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

Block Editor: Partition attributes updates to avoid conflating meta and blocks attributes #15781

Merged
merged 1 commit into from
May 27, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented May 22, 2019

Fixes #15739

This pull request seeks to resolve an issue where setting a new meta attribute value in a block can wrongfully displace the text caret selection.

Implementation Notes:

I consider this to be an incomplete but likely sufficient resolution to #15739. The issue occurs because a render of the block's edit occurs synchronously as a result of updateBlockAttributes. At this point in time the meta attributes have not yet updated, so the old value is used (more at #15739 (comment)). Arguably, it is not correct (or at least ineffective) to be calling updateBlockAttributes with new meta values, since the selector to generate a block's attributes will always ignore these (source). To that point, the changes here will avoid calling updateBlockAttributes with meta value updates, and avoid calling it altogether if the only attributes updates are meta value updates. The solution works because updateBlockAttributes is avoided, and thus the problematic intermediate render does not occur.

It is technically incomplete in that if a block were to call setAttributes with a mix of block attributes and meta attributes, a similar issue may occur. It is quite unlikely that a block would be implemented this way.

In any case, I'd like to see some improvements to this sort of "external" sourcing, which would be a larger task than I propose to resolve here. I will aim to submit a formal proposal for how it might work, informed on #15739 and previous issues #4989 and #2759. An inline code comment has been added to reflect this future work.

Testing Instructions:

An existing end-to-end test case has been adapted to reveal the failure from master resolved here. Verify that it passes:

npm run test-e2e packages/e2e-tests/specs/plugins/meta-attribute-block.test.js

Repeat Steps to Reproduce from #15739

@aduth aduth added [Type] Bug An existing feature does not function as intended [Package] Block editor /packages/block-editor labels May 22, 2019
@aduth aduth requested a review from youknowriad May 22, 2019 15:42
},
{}
);

Copy link
Contributor

Choose a reason for hiding this comment

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

The behavior of all this setAttributes function seems like something that should be absorbed in the updateBlockAttributes action instead.

As you said, we should ship the bug fix and consider these consolidations separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

The behavior of all this setAttributes function seems like something that should be absorbed in the updateBlockAttributes action instead.

As you said, we should ship the bug fix and consider these consolidations separately.

Yes, I think so too. One point of clarification / affirmation is that: I don't think the changes here dig us any further into a hole, and in-fact serve more as an implementation which ought to have existed in the first place.

@youknowriad youknowriad added this to the 5.8 (Gutenberg) milestone May 27, 2019
@youknowriad youknowriad merged commit cb127c9 into master May 27, 2019
@youknowriad youknowriad deleted the fix/15739-meta-attribute-update branch May 27, 2019 08:10
@youknowriad youknowriad added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label May 27, 2019
youknowriad pushed a commit that referenced this pull request May 29, 2019
@jorgefilipecosta jorgefilipecosta removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cursor jumps to end in TextControl with attribute source 'meta' after each keystroke
3 participants