Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LPS-122449 Remove usages of dom.delegate #466

Closed

Conversation

kresimir-coko
Copy link
Collaborator

@kresimir-coko kresimir-coko commented Nov 2, 2020

I've started removing usages of dom.delegate.

In the meanwhile I realised that the implementation of delegate isn't fully functional, at least from my investigations, so I've pushed a commit that extends the utility to find an ancestor that matches the provided selector.

I've also provided some examples of usage with the pattern tested working, except a couple of them - ones that have selectors for html tags or children, e.g. '.someClassName a'

@wincent
Copy link

wincent commented Nov 2, 2020

Another thing I discovered is that the usage commonly has event.delegateTarget and .removeListener() which I removed from a couple usages so let me know if I did it properly, I managed to test both and they seem to be working fine.

MDN's page on Event.srcElement

Deprecated

This feature is no longer recommended. Though some browsers might still support it, it may have already been removed from the relevant web standards, may be in the process of being dropped, or may only be kept for compatibility purposes. Avoid using it, and update existing code if possible; see the compatibility table at the bottom of this page to guide your decision. Be aware that this feature may cease to work at any time.

@kresimir-coko
Copy link
Collaborator Author

I've pushed another batch of changes, and updated the list of those I didn't manage to test due to not being able to find them in the UI.

I've hit a problem with one of them, upload_multiple_file_entries_resources.jsp, where after adding delegate, and doing the thing that triggers the event the page breaks, and it cannot find Liferay, AUI, here's how it looks.
image

@kresimir-coko
Copy link
Collaborator Author

I've pushed more changes here, but there's a blocker that I explained in Slack.

@kresimir-coko
Copy link
Collaborator Author

kresimir-coko commented Nov 5, 2020

I've pushed a couple commits, replacing old ones. One of the commits expands the delegate utility, whereas the other commit contains some of the replacements in JSPs, let me know if I missed something.

P.S. is there a way to globally run formatSource?

@kresimir-coko
Copy link
Collaborator Author

I've pushed new commits, removing the old ones, the commits are split into 2, one for JSPs and one for JS files.

I've tested a couple of patterns used in JSPs and one (and only) pattern used in JS files.

@kresimir-coko kresimir-coko marked this pull request as ready for review November 9, 2020 14:36
@wincent
Copy link

wincent commented Nov 10, 2020

GitHub is showing conflicts on this one @kresimir-coko, so looks like it needs a rebase.

@kresimir-coko
Copy link
Collaborator Author

I see @wincent thanks for the ping, I'll take a look.

@kresimir-coko
Copy link
Collaborator Author

Hey @wincent I've pushed commits with removed conflicts!

@kresimir-coko kresimir-coko changed the title [Draft] LPS-122449 Remove usages of dom.delegate LPS-122449 Remove usages of dom.delegate Nov 10, 2020
@wincent
Copy link

wincent commented Nov 10, 2020

ci:test:sf

@wincent
Copy link

wincent commented Nov 10, 2020

ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

❌ ci:test:sf - 0 out of 1 jobs passed in 22 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 9226b10831eed5db236b208be0c6e85c8aafbe62

Sender Branch:

Branch Name: LPS-122449
Branch GIT ID: 6a65c90c5e1323dda8e1caf6535456087eb84276

0 out of 1jobs PASSED
For more details click here.
     [exec] apps/journal/journal-content-web/src/main/resources/META-INF/resources/configuration.jsp: BAD
     [exec] apps/journal/journal-web/src/main/resources/META-INF/resources/view_more_menu_items.jsp: BAD
     [exec] apps/knowledge-base/knowledge-base-web/src/main/resources/META-INF/resources/admin/common/history.jsp: BAD
     [exec] apps/layout/layout-admin-web/src/main/resources/META-INF/resources/select_master_layout.jsp: BAD
     [exec] apps/layout/layout-admin-web/src/main/resources/META-INF/resources/select_style_book.jsp: BAD
     [exec] apps/layout/layout-taglib/src/main/resources/META-INF/resources/layout_classed_model_usages_view/page.jsp: BAD
     [exec] apps/product-navigation/product-navigation-product-menu-web/src/main/resources/META-INF/resources/portlet/pages_tree.jsp: BAD
     [exec] apps/wiki/wiki-web/src/main/resources/META-INF/resources/wiki/page_iterator.jsp: BAD
     [exec] Prettier checked 54 files, 13 files have problems
     [exec] 
     [exec] 1 of 3 jobs failed
     [exec] 
     [exec] error Command failed with exit code 1.
     [exec] info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
     [exec] 
     [exec] > Task :packageRunCheckFormat FAILED
     [exec] Gradle build finished at 2020-11-10 12:52:14.891.
     [exec] 
     [exec] FAILURE: Build failed with an exception.
     [exec] 
     [exec] * What went wrong:
     [exec] Execution failed for task ':packageRunCheckFormat'.
     [exec] > Process 'command '/opt/dev/projects/github/liferay-portal/build/node/bin/node'' finished with non-zero exit value 1
     [exec] 
     [exec] * Try:
     [exec] Run with --info or --debug option to get more log output. Run with --scan to get full insights.
     [exec] 
     [exec] * Exception is:
     [exec] org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':packageRunCheckFormat'.
     [exec] 	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.lambda\$executeIfValid\$1(ExecuteActionsTaskExecuter.java:208)
     [exec] 	at org.gradle.internal.Try\$Failure.ifSuccessfulOrElse(Try.java:263)
     [exec] 	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeIfValid(ExecuteActionsTaskExecuter.java:206)
     [exec] 	at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.execute(ExecuteActionsTaskExecuter.java:187)

@liferay-continuous-integration
Copy link
Collaborator

@kresimir-coko
Copy link
Collaborator Author

Hey @wincent I'm trying to see which files/modules have SF errors in them, but I don't know the login details for the test platform:
image

@jbalsas
Copy link

jbalsas commented Nov 10, 2020

Hey @kresimir-coko, just expand the comment above:

Screen Shot 2020-11-10 at 13 54 40

And locally run ant format-source from the portal-impl folder to see the full scan.

@kresimir-coko
Copy link
Collaborator Author

And locally run ant format-source from the portal-impl folder to see the full scan.

Didn't know we had this going for us, awesome, ran it and pushed a commit fixing those SF problems.

@wincent
Copy link

wincent commented Nov 10, 2020

Hey @wincent I'm trying to see which files/modules have SF errors in them, but I don't know the login details for the test platform:

At least some of them can be seen in the GitHub comment:

     [exec] apps/journal/journal-content-web/src/main/resources/META-INF/resources/configuration.jsp: BAD
     [exec] apps/journal/journal-web/src/main/resources/META-INF/resources/view_more_menu_items.jsp: BAD
     [exec] apps/knowledge-base/knowledge-base-web/src/main/resources/META-INF/resources/admin/common/history.jsp: BAD
     [exec] apps/layout/layout-admin-web/src/main/resources/META-INF/resources/select_master_layout.jsp: BAD
     [exec] apps/layout/layout-admin-web/src/main/resources/META-INF/resources/select_style_book.jsp: BAD
     [exec] apps/layout/layout-taglib/src/main/resources/META-INF/resources/layout_classed_model_usages_view/page.jsp: BAD
     [exec] apps/product-navigation/product-navigation-product-menu-web/src/main/resources/META-INF/resources/portlet/pages_tree.jsp: BAD
     [exec] apps/wiki/wiki-web/src/main/resources/META-INF/resources/wiki/page_iterator.jsp: BAD
     [exec] Prettier checked 54 files, 13 files have problems

but in any case, the login details are your Liferay email address name (ie. up to but not including the @) and your Liferay email password.

@wincent
Copy link

wincent commented Nov 10, 2020

ci:test:sf

@wincent
Copy link

wincent commented Nov 13, 2020

You cannot perform that action because you are not a member of the Liferay organization.

youre-fired

@kresimir-coko
Copy link
Collaborator Author

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 9 out of 9 jobs passed

❌ ci:test:relevant - 19 out of 30 jobs passed in 2 hours 53 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 06e707df4454e36051558b8c5deaf7600497f9ab

Copied in Private Modules Branch:

Branch Name: master-private
Branch GIT ID: 8d22a791300569a6c92e7b9152e6ae1c6bdc863a

ci:test:stable - 9 out of 9 jobs PASSED
9 Successful Jobs:
ci:test:relevant - 19 out of 30 jobs PASSED

11 Failed Jobs:

19 Successful Jobs:
For more details click here.

Failures unique to this pull:

  1. test-portal-acceptance-pullrequest-batch(master)/modules-integration-mysql57-jdk8/0
    Job Results:

    18 Tests Passed.
    1 Test Failed.

    1. AXIS_VARIABLE=3,label_exp=!master #444378
           [exec] * What went wrong:
           [exec] Could not resolve all artifacts for configuration 'classpath'.
           [exec] > Could not resolve org.springframework.boot:spring-boot-gradle-plugin:1.5.7.RELEASE.
           [exec]   Required by:
           [exec]       unspecified:unspecified:unspecified > com.liferay:com.liferay.gradle.plugins.defaults:6.1.404
           [exec]    > Could not resolve org.springframework.boot:spring-boot-gradle-plugin:1.5.7.RELEASE.
           [exec]       > Could not parse POM https://repository-cdn.liferay.com/nexus/content/groups/public/org/springframework/boot/spring-boot-gradle-plugin/1.5.7.RELEASE/spring-boot-gradle-plugin-1.5.7.RELEASE.pom
           [exec]          > Could not resolve org.springframework.boot:spring-boot-tools:1.5.7.RELEASE.
           [exec]             > Could not resolve org.springframework.boot:spring-boot-tools:1.5.7.RELEASE.
           [exec]                > Could not parse POM https://repository-cdn.liferay.com/nexus/content/groups/public/org/springframework/boot/spring-boot-tools/1.5.7.RELEASE/spring-boot-tools-1.5.7.RELEASE.pom
           [exec]                   > Could not resolve org.springframework.boot:spring-boot-parent:1.5.7.RELEASE.
           [exec]                      > Could not resolve org.springframework.boot:spring-boot-parent:1.5.7.RELEASE.
           [exec]                         > Could not parse POM https://repository-cdn.liferay.com/nexus/content/groups/public/org/springframework/boot/spring-boot-parent/1.5.7.RELEASE/spring-boot-parent-1.5.7.RELEASE.pom
           [exec]                            > Could not resolve org.springframework.boot:spring-boot-dependencies:1.5.7.RELEASE.
           [exec]                               > Could not resolve org.springframework.boot:spring-boot-dependencies:1.5.7.RELEASE.
           [exec]                                  > Could not parse POM https://repository-cdn.liferay.com/nexus/content/groups/public/org/springframework/boot/spring-boot-dependencies/1.5.7.RELEASE/spring-boot-dependencies-1.5.7.RELEASE.pom
           [exec]                                     > Could not resolve com.fasterxml.jackson:jackson-bom:2.8.10.
           [exec]                                        > Could not resolve com.fasterxml.jackson:jackson-bom:2.8.10.
           [exec]                                           > Could not get resource 'https://repository-cdn.liferay.com/nexus/content/groups/public/com/fasterxml/jackson/jackson-bom/2.8.10/jackson-bom-2.8.10.pom'.
    2. AXIS_VARIABLE=19,label_exp=!master #444378
      1. SearchRequestBuilderTest.testAddPostFilterQueryPart
        org.junit.ComparisonFailure: \{"from":0,"size":10000,"query":\{"bool":\{"must":[\{"bool":\{"must":[\{"bool":\{"should":[\{"bool":\{"must":[\{"match":\{"comments":\{"query":"omega"\}\}\}],"should":[\{"match_phrase":\{"comments":\{"query":"omega","slop":50\}\}\},\{"match_phrase":\{"comments":\{"query":"omega","boost":2.0\}\}\}]\}\},\{"bool":\{"must":[\{"match":\{"content":\{"query":"omega"\}\}\}],"should":[\{"match_phrase":\{"content":\{"query":"omega","slop":50\}\}\},\{"match_phrase":\{"content":\{"query":"omega","boost":2.0\}\}\}]\}\},\{"bool":\{"must":[\{"match":\{"description":\{"query":"omega"\}\}\}],"should":[\{"match_phrase":\{"description":\{"query":"omega","slop":50\}\}\},\{"match_phrase":\{"description":\{"query":"omega","boost":2.0\}\}\}]\}\},\{"bool":\{"should":[\{"wildcard":\{"properties":\{"wildcard":"*omega*"\}\}\}]\}\},\{"bool":\{"must":[\{"bool":\{"should":[\{"match":\{"title":\{"query":"omega"\}\}\},\{"match_phrase_prefix":\{"title":\{"query":"omega"\}\}\}]\}\}],"should":[\{"match_phrase":\{"title":\{"query":"omega","boost":2.0\}\}\}]\}\},\{"bool":\{"should":[\{"match_phrase":\{"url":\{"query":"omega"\}\}\},\{"bool":\{"should":[\{"wildcard":\{"url":\{"wildcard":"*omega*"\}\}\}]\}\},\{"match_phrase_prefix":\{"url":\{"query":"omega","max_expansions":300\}\}\}]\}\},\{"bool":\{"should":[\{"wildcard":\{"userName":\{"wildcard":"*omega*"\}\}\}]\}\},\{"bool":\{"must":[\{"match":\{"assetCategoryTitles_en_US":\{"query":"omega"\}\}\}],"should":[\{"match_phrase":\{"assetCategoryTitles_en_US":\{"query":"omega","slop":50\}\}\},\{"match_phrase":\{"assetCategoryTitles_en_US":\{"query":"omega","boost":2.0\}\}\}]\}\},\{"bool":\{"must":[\{"bo...
  2. ...

@liferay-continuous-integration
Copy link
Collaborator

@kresimir-coko
Copy link
Collaborator Author

Hey @john-co I've fixed the problem in the Related section you mentioned, leftover CI errors are probably not related to this PR, can you confirm?

@john-co
Copy link

john-co commented Nov 16, 2020

I can reproduce this issue when running the poshi test locally. I think what's happening is that the text field is still being focused after the test clicks on close the side panel. And while the text field is still being focused, the test clicks on the add/plus button to open the side panel again but is showing the edit field side panel instead of the add Elements tab. Maybe this is something Forms team can confirm as a test fix or regression to fix? @cvanut

cc/ @cvanut, can you please take a look? The failing tests look like they're all Forms tests.

@john-co
Copy link

john-co commented Nov 16, 2020

pinged Cleyton on slack to advise.

@john-co
Copy link

john-co commented Nov 17, 2020

Cleyton says he will take a look tomorrow.

@jbalsas
Copy link

jbalsas commented Nov 17, 2020

Hey @kresimir-coko, could you rebase this in the meantime?

In the future, it's best to split these tasks in different commits so you can discard whatever area is giving you trouble to avoid the whole thing getting blocked over an issue in one specific area.

If you want, you could actually rework this PR to split the changes in Forms from the rest and send 2 different PRs to see if you can get something going.

@kresimir-coko
Copy link
Collaborator Author

@jbalsas Sure, I'm fixing conflicts as we speak, so I'll split Forms away and send 2 separate PRs

@cvanut
Copy link

cvanut commented Nov 17, 2020

Just started reviewing :)

:octocat: Sent from GH.

@john-co
Copy link

john-co commented Nov 17, 2020

Here's the differences I'm seeing when running poshi test from this PR test bundle:
Peek 2020-11-17 12-08

vs. a bundle from master:
Expected

After clicking the Close (x) icon from the side panel, the Text Field is still highlighted (border focused). And then when the test clicks on the Plus button again, the side panel is supposed to show Elements tab to add another element, but instead has Basics tab which edits the Text Field.

You can see from the video from the master bundle that the Text Field is not highligted (border focused). And then when it clicks on Plus button, it correctly shows the side panel with Elements tab in order to add another element.

There's no difference between Chrome 86 and 65. I was able to reproduce the issue on both chrome versions running the poshi test locally. However, I cannot reproduce the issue manually. With the current tools we have I can think of including a refresh page, or navigating back to the page as a good hard reset for a test fix workaround. Could this be resolved on the poshi side @kenjiheigel? Maybe there's some dependent behavior with dom.delegate?

@john-co
Copy link

john-co commented Nov 17, 2020

I'm unable to reproduce this behavior manually with my theory about the Text Field being selected...so my guess is the ideal fix would come from poshi side. Otherwise, we'd have to investigate a workaround test fix:
Unreproducible

@jbalsas
Copy link

jbalsas commented Nov 20, 2020

Going to close this since we're apparently trying to split it into smaller PRs like #466 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants