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

Backport WordPress 5.2.1 fixes #15650

Merged
merged 11 commits into from
May 16, 2019
Merged

Backport WordPress 5.2.1 fixes #15650

merged 11 commits into from
May 16, 2019

Conversation

youknowriad and others added 4 commits May 15, 2019 12:26
* Rich Text: Set applied format as active in applyFormats

* Rich Text: Update toggleFormat tests per activeFormats revision

* Rich Text: Replace existing active format in applied

* Rich Text: Remove active format in all cases for removeFormat

* Rich Text: Update toggleFormat tests per removeFormats revision

* Testing: Update E2E test case to verify link display regression

* Rich Text: Verify by test of activeFormats format replacement
@ellatrix
Copy link
Member

I think everything is added now.

@ellatrix
Copy link
Member

Let's see if the tests pass.

@aduth
Copy link
Member

aduth commented May 15, 2019

A recurring error I see in the failing build is this one:

TypeError: Cannot read property 'isContentEditable' of undefined

I can reproduce it as well, when trying to ArrowUp / ArrowDown between two paragraphs.

There are changes to both writing-flow/index.js and dom.js which are prime candidates for being culprit to these errors, though the specific changes don't immediately strike me as problematic.

@aduth
Copy link
Member

aduth commented May 16, 2019

A recurring error I see in the failing build is this one:

TypeError: Cannot read property 'isContentEditable' of undefined

This error is because we should have kept the target argument being passed to computeCaretRect in the cherry-picked 4e45835 . In master, this was later removed in a9a3907 , but since that change is not reflected here, I think we should keep the argument. The other option is to cherry-pick a9a3907 as well, or at least the relevant logic changes for computeCaretRect.

In 72539b8, I chose to simply reintroduce the target argument. This could be squashed/fixup'd into 4e45835.

@aduth
Copy link
Member

aduth commented May 16, 2019

There's a build failure caused by the console warnings tracked at Trac#47183 and fixed in master at #15502 . The changes in that pull request affect only tests, and should be safe to pull in here. I'll cherry-pick the merge commit 17c3571 to allow the build to pass.

* Testing: Skip test failures for font decoding warnings

* Testing: Fix Classic block Add Media toolbar button click
@@ -483,5 +483,4 @@ Undocumented declaration.

<!-- END TOKEN(Autogenerated API docs) -->


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen this around locally as well sometimes. Why does this happen?

Copy link
Member

@oandregal oandregal May 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably related #15626 I'm investigating it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#15679 should fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I actually had to add empty lines to the previous npm release (because of the failure we had) to force learna to detect these as changed. This just reverts those unnecessary changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I actually had to add empty lines to the previous npm release (because of the failure we had) to force learna to detect these as changed. This just reverts those unnecessary changes.

Can someone clarify for me: Do these changes matter? Is there anything further which needs to be done in this branch for them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, nothing left to do here.

@ellatrix
Copy link
Member

Thanks for looking at the test failures, @aduth! I seems there are still two left involving metaboxes.

@youknowriad
Copy link
Contributor Author

The failure seems related to autop. It seems that the "p" tag is not added in the TinyMCE metabox.
That said, it happens only when we install popular plugins and the same test pass without these plugins.

I wonder if it's fine to just ignore this failure?

@aduth
Copy link
Member

aduth commented May 16, 2019

I've tracked the build failure to Advanced Custom Fields, specifically these two lines:

https://github.com/AdvancedCustomFields/acf/blob/079c93e029457eacbe91cc8508aa4b81fbc4777d/assets/js/acf-input.js#L12403-L12404

It's not clear to me yet why (a) this fails specifically on this branch and not others and (b) why or if ACF should be mutating the wp.editor global in this way.

cc @elliotcondon

Worth highlighting that the field being used in the test is not one intended to be managed by ACF, but rather its own custom metabox field:

https://github.com/WordPress/gutenberg/blob/master/packages/e2e-tests/plugins/wp-editor-metabox.php
https://github.com/WordPress/gutenberg/blob/master/packages/e2e-tests/specs/plugins/wp-editor-meta-box.test.js

@aduth
Copy link
Member

aduth commented May 16, 2019

It's not clear to me yet why (a) this fails specifically on this branch and not others

In master, the specific test case was changed to be skipped due to intermittent failures, perhaps related to the same plugin incompatibility (though not necessarily so, since it was both intermittent, and with a different error message). The relevant pull request was #15211, tracked for reintroduction at #15538 .

I will cherry-pick the merge commit 93c5ad3 from #15211 to sync with expected behavior from master. This still means there is a legitimate failure to be addressed, though it is not specific to this branch.

@youknowriad
Copy link
Contributor Author

I'm going to test quickly the fixes cherry-picked here and see if everything works as expected.

@aduth
Copy link
Member

aduth commented May 16, 2019

Noting: We probably want to be dropping the commit f004741 before merging. It was included to force Travis to run the tests. Then again, it wouldn't be a terrible thing to have the commit merged into the wp/5.2 branch.

@aduth
Copy link
Member

aduth commented May 16, 2019

I will cherry-pick the merge commit 93c5ad3 from #15211 to sync with expected behavior from master. This still means there is a legitimate failure to be addressed, though it is not specific to this branch.

To verify, I applied local changes to unskip the test in the master branch, and with POPULAR_PLUGINS flag, an identical error was produced when running the end-to-end test case.

@youknowriad
Copy link
Contributor Author

Can't approve my own PR. I tested everything (except the google doc thing where it's hard to replicate). Everything seems to work as intended.

@aduth
Copy link
Member

aduth commented May 16, 2019

Relevant Trac ticket: https://core.trac.wordpress.org/ticket/47284

@youknowriad youknowriad merged commit e28b0d8 into wp/5.2 May 16, 2019
@youknowriad youknowriad deleted the backport/wp-5.2.1-fixes branch May 16, 2019 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants