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

Plugin: Backport bug fixes from Gutenberg 5.5.0 to wp/trunk (WordPress 5.2 RC) #14987

Merged
merged 22 commits into from
Apr 16, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 15, 2019

This pull request seeks to backport the following bug-fix pull requests from Gutenberg 5.5.0 to the wp/trunk branch, to be published as part of a subsequent packages release and patched into core ahead of this Wednesday's WordPress 5.2 release candidate.

Process Notes:

Roughly following steps from:

https://github.com/WordPress/gutenberg/blob/master/docs/contributors/release.md#minor-wordpress-releases

I wrote a small Node script to generate cherry-pick commands to ensure each pull request was applied in the order of their merge date:

https://gist.github.com/aduth/901b1604dc7a9ba520db0a06deadec05

And a variant to generate the above list of pull requests and corresponding authors:

https://gist.github.com/aduth/65edee20b936a21e086dcc6f60bbb3e8

There were a few issues encountered in the process of applying cherry-picked commits:

Conflicts:

Not Included:

Testing Instructions:

Verify fix of impacted pull requests.

Ensure tests pass.

@aduth aduth added the Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts label Apr 15, 2019
@aduth aduth requested a review from youknowriad April 15, 2019 14:59
@youknowriad
Copy link
Contributor

Can we enable the e2e tests on these branches?

@youknowriad
Copy link
Contributor

(you shouldn't have named the branch wp/* because these are protected :)

@youknowriad
Copy link
Contributor

I'm still seeing a border in the post-title in the code editor which is supposed to be fixed by this #14771 (might be a markup conflict)

@youknowriad
Copy link
Contributor

I'm seeing an issue where the columns block is not functional. I don't see any "column" block inserted.

@jasmussen
Copy link
Contributor

Are you looking at the markup? Because until we have better parent selection, selecting the column with the mouse has intentionally been disabled.

@youknowriad
Copy link
Contributor

@jasmussen not really, just inserting a columns block and trying to insert content is broken.

@youknowriad
Copy link
Contributor

So I tested almost all the fixes here, everything seems fine. The only concerning issue is the columns block.

@jasmussen
Copy link
Contributor

#14876 for posterity.

@aduth
Copy link
Member Author

aduth commented Apr 15, 2019

I'm seeing an issue where the columns block is not functional. I don't see any "column" block inserted.

By the noted conflict, there were some substantial updates not included yet in trunk from #13899. Unclear if or how much #14876 might rely on those changes.

Occurring to me now that Travis builds aren't running here. I'd have imagined some end-to-end failures would be reflected in the issues you're discovering. Might need to be run manually in local environment instead.

@aduth
Copy link
Member Author

aduth commented Apr 15, 2019

From some initial debugging, the issues with columns block appear to stem from #14003 . Specifically, the initialization of the columns block in inserting individual Column blocks is considered as invalid:

https://github.com/WordPress/gutenberg/pull/14003/files#diff-90854747cb48848ced1cc6f1700d8b66R397

Unclear yet why this works in master and not in this branch.

@aduth
Copy link
Member Author

aduth commented Apr 15, 2019

Oh, I see now. It relies on #14291, which was never introduced to wp/trunk because it was considered a feature, but is necessary for Columns to work correctly when including the changes from #14003 (it substitutes the implementation of the mapDispatchToProps for InnerBlocks to use custom logic in place of insertBlocks).

I might suggest then one of two options for moving forward:

cc @jorgefilipecosta

@aduth
Copy link
Member Author

aduth commented Apr 16, 2019

(you shouldn't have named the branch wp/* because these are protected :)

Is there any way to change it now? Is it documented somewhere, or should I follow-up with a task for that?

@aduth
Copy link
Member Author

aduth commented Apr 16, 2019

I might suggest then one of two options for moving forward:

In some initial testing of dropping the commit from #14003, it does resolve the behavior of the columns not being added, but there appears to be some other issue with being able to select the blocks themselves (perhaps related to #14876, as the symptom is very much of the sort impacted by pointer-events styling).

@youknowriad
Copy link
Contributor

Is there any way to change it now? Is it documented somewhere, or should I follow-up with a task for that?

I think once we merge this PR, we should disable the protection, remove the branch and enable the protection again. This is not documented, but we have the same with release/* branches. Maybe we could document protected branches.

@youknowriad
Copy link
Contributor

Can we try backporting #14291 as well?

@aduth aduth force-pushed the wp/trunk-5-2-0-rc-1 branch from d75b533 to 8c82af1 Compare April 16, 2019 12:35
@aduth
Copy link
Member Author

aduth commented Apr 16, 2019

I've pushed a rebased branch which includes the following:

† Aside: This class name does not seem conventional.

I think once we merge this PR, we should disable the protection, remove the branch and enable the protection again. This is not documented, but we have the same with release/* branches. Maybe we could document protected branches.

I ran into issues force-pushing the rebase due to the protection rules. I updated them to two separate rules wp/?.? and wp/trunk to allow me to push, which could perhaps be a general improvement anyways. Otherwise, I can restore the original rule after.

@jasmussen
Copy link
Contributor

I'd like a second opinion from @jasmussen in case one or the other of (a) the lowered specificity by the removed selector or (b) the selector .block-core-columns was needed in some way to contextualize the subsequent .block-editor-block-list__layout.

I wish I had found out exactly what made the old stopgap fix (to prevent the selection of the column block) break. To the best of my knowledge, what made it break was that the CSS selector was incredibly specific and used adjacent sibling selectors, and then a separate refactor of either the block list or the columns block added an additional element to the hierarchy — could be a fragment or anything — making it break. But I'm not sure.

The new fix is much simpler, less specific, and incidentally should affect pointer events less so than the old one due to the reduced specificity. From looking at the code, it looks correctly ported. But the way to test is to insert a columns block, and be sure to test both an archives block, and a reusable block as nested elements inside the columns block. Both archives and reusable blocks use the component-disabled component which also disables pointer events, those should remain disabled despite what the columns block does.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

I noticed there are some unit test fails in this branch:

  ● state › blocks() › replace inner blocks › can replace a child block

    expect(received).toEqual(expected)

    Difference:

    - Expected
    + Received

    @@ -16,11 +16,12 @@
            "clientId": "chicken",
            "isValid": true,
            "name": "core/test-parent-block",
          },
        },
    -   "isPersistentChange": Anything,
    +   "isIgnoredChange": false,
    +   "isPersistentChange": true,

It seems the test case is not aware of isIgnoredChange that was meanwhile added.

@aduth
Copy link
Member Author

aduth commented Apr 16, 2019

@jorgefilipecosta Good catch. Noted in the original comment, I had some conflict with these tests in 8722f49 (#14916). At the time I wasn't bringing in changes from #14291. I'll try cherry-picking the commit again.

It also reminds me to run all tests again locally, since as noted above, Travis is not running against this branch.

@jorgefilipecosta
Copy link
Member

I think the root cause of the test failure is that I should have used toMatchObject to allow future additions of keys in the reducer not relevant to the test case. I can submit a PR with these changes to tests that can be merged to master and cherry-picked to this branch if the problem persists.

aduth and others added 10 commits April 16, 2019 09:26
* Block Editor: Avoid creating new state reference on unchanging isPersistentChange

* Block Editor: Consider received blocks state change as ignored

* fixup a35fa82

* Block Editor: Regenerate documentation
When you focus a color swatch, the circle around the check is offset.

Removing the border that this PR removes, fixes that, and appears to not have any side-effects.

I'm not sure how it regressed.
Fixes #14853.

This removes the box shadow on the inserter button that appears to the left of the default block appender (or to the right on mobile).

This brings the hover state in line with the hover state for the sibling inserter.
* Adjust clashing shortcuts used for character input

* Update modal

* adds docs correction to correspond, adds inlined script to remove production dead code
* Button Block: fix space insertion

* Typo

* Allow rule for preview
#14938)

* Block Editor: Pass spread props from URLPopover to Popover

Avoids need to explicitly support onClickOutside as deprecated pass-through prop, instead leveraging pass-through nature of spread props.

* Format Library: Assign Popover getAnchorRect to update selection position

* Format Library: Compute memoized anchor from caret point or active element

* Components: Add anchorRect prop to Popover component

* Format Library: Provide direct reference to Popover anchorRect

* Format Library: Consider next element sibling for initial caret placement

Co-Authored-By: Ella van Durpe <[email protected]>

* Components: Mark PositionedAtSelection as unstable

* Components: Remove getAnchorRect mention from Popover README
* Fix spacer NaN warning

* Set the min height of spacer block

* Reset the customHeight if it is invalid

* Add local state in spacer

* Update input field's value when using drag handle
@aduth
Copy link
Member Author

aduth commented Apr 16, 2019

@jorgefilipecosta Good catch. Noted in the original comment, I had some conflict with these tests in 8722f49 (#14916). At the time I wasn't bringing in changes from #14291. I'll try cherry-picking the commit again.

I rebased locally and substituted the original 8722f49 for where in the branch I'd previously made revisions in 215f7f2 to account for conflicts. The commit applied cleanly (unlike before) and all unit tests passed.

I'm going to run through the end-to-end tests locally, but will push the rebased branch afterward.

@aduth aduth force-pushed the wp/trunk-5-2-0-rc-1 branch from 8c82af1 to 660fbaa Compare April 16, 2019 14:23
@aduth
Copy link
Member Author

aduth commented Apr 16, 2019

Pushed the rebased branch. I had some initial trouble with the end-to-end tests related to the fact that #14243 hadn't been fixed in wp/trunk. Temporarily bringing it into the branch helped demonstrate that all tests are passing.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This is looking good in my manual tests.

@aduth
Copy link
Member Author

aduth commented Apr 16, 2019

I'm still seeing a border in the post-title in the code editor which is supposed to be fixed by this #14771 (might be a markup conflict)

I could not reproduce this. In Firefox, I see that the border is thicker on the left side than it is in Chrome (screenshot), but I see this in master as well.

@aduth
Copy link
Member Author

aduth commented Apr 17, 2019

Publish results:

Successfully published:
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
 - @wordpress/[email protected]
lerna success published 11 packages

Trac ticket: https://core.trac.wordpress.org/ticket/46951

nylen pushed a commit to nylen/wordpress-develop-svn that referenced this pull request Apr 22, 2019
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]

Props iseulde, kjellr, aduth, get_dave, talldanwp, jorgefilipecosta, afercia, nosolosw, jasmussen, netweb .

See WordPress/gutenberg#14987 .
See WordPress/gutenberg#15020 .
Fixes #46951 .


git-svn-id: https://develop.svn.wordpress.org/trunk@45255 602fd350-edb4-49c9-b593-d223f7449a82
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Apr 22, 2019
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]

Props iseulde, kjellr, aduth, get_dave, talldanwp, jorgefilipecosta, afercia, nosolosw, jasmussen, netweb .

See WordPress/gutenberg#14987 .
See WordPress/gutenberg#15020 .
Fixes #46951 .


git-svn-id: https://develop.svn.wordpress.org/trunk@45255 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Apr 22, 2019
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]

Props iseulde, kjellr, aduth, get_dave, talldanwp, jorgefilipecosta, afercia, nosolosw, jasmussen, netweb .

See WordPress/gutenberg#14987 .
See WordPress/gutenberg#15020 .
Fixes #46951 .

Built from https://develop.svn.wordpress.org/trunk@45255


git-svn-id: http://core.svn.wordpress.org/trunk@45064 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Apr 22, 2019
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]

Props iseulde, kjellr, aduth, get_dave, talldanwp, jorgefilipecosta, afercia, nosolosw, jasmussen, netweb .

See WordPress/gutenberg#14987 .
See WordPress/gutenberg#15020 .
Fixes #46951 .

Built from https://develop.svn.wordpress.org/trunk@45255


git-svn-id: https://core.svn.wordpress.org/trunk@45064 1a063a9b-81f0-0310-95a4-ce76da25c4cd
miya0001 pushed a commit to cjk4wp/wordpress that referenced this pull request Jul 4, 2019
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]
- @wordpress/[email protected]

Props iseulde, kjellr, aduth, get_dave, talldanwp, jorgefilipecosta, afercia, nosolosw, jasmussen, netweb .

See WordPress/gutenberg#14987 .
See WordPress/gutenberg#15020 .
Fixes #46951 .


git-svn-id: http://develop.svn.wordpress.org/trunk@45255 602fd350-edb4-49c9-b593-d223f7449a82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Plugin Issues or PRs related to Gutenberg Plugin management related efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.