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

Fix: Custom color formatter may end up "bouncing" #20612

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Mar 3, 2020

Fixes: https://core.trac.wordpress.org/ticket/49568

How has this been tested?

I added a paragraph with some text.
I opened the formater menu and applied a custom text color to the paragraph using the mouse.
I manually applied the "red" color (I verified that the popover did not start moving randomly around the screen).
I selected another part of the text in the paragraph I applied a custom color and verified the popovers positions for both parts of the paragraph with colors are correct close to the part of text they refer to.

@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Mar 3, 2020

Size Change: -22 B (0%)

Total Size: 865 kB

Filename Size Change
build/format-library/index.js 7.58 kB -22 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.5 kB 0 B
build/block-editor/style.css 10.5 kB 0 B
build/block-library/editor-rtl.css 7.36 kB 0 B
build/block-library/editor.css 7.36 kB 0 B
build/block-library/index.js 116 kB 0 B
build/block-library/style-rtl.css 7.5 kB 0 B
build/block-library/style.css 7.51 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.6 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.6 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 90.9 kB 0 B
build/edit-post/style-rtl.css 8.53 kB 0 B
build/edit-post/style.css 8.53 kB 0 B
build/edit-site/index.js 4.63 kB 0 B
build/edit-site/style-rtl.css 2.51 kB 0 B
build/edit-site/style.css 2.51 kB 0 B
build/edit-widgets/index.js 4.42 kB 0 B
build/edit-widgets/style-rtl.css 2.59 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 325 B 0 B
build/editor/editor-styles.css 327 B 0 B
build/editor/index.js 44.6 kB 0 B
build/editor/style-rtl.css 3.98 kB 0 B
build/editor/style.css 3.98 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.48 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.02 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

}, [ isActive, addingColor, value.start, value.end ] );
}, [] );
Comment on lines -68 to +71

This comment has been minimized.

Copy link
@aduth

aduth Mar 6, 2020

Member

Which of these is changing to cause the cache to be bust and recompute the anchor?

This comment has been minimized.

Copy link
@aduth

aduth Mar 6, 2020

Member

Kinda seems like the anchor calculation should be testing whether it's within the popover, and aborting if so.

This comment has been minimized.

Copy link
@aduth

aduth Mar 6, 2020

Member

Seems like these values shouldn't be changing in the first place, which is something we should be addressing higher up than in this component itself. Notably, I observe that value.start and value.end each change to undefined at some points, which I don't think would ever be expected while this component is mounted.

@aduth
aduth approved these changes Mar 6, 2020
}, [ isActive, addingColor, value.start, value.end ] );
}, [] );

This comment has been minimized.

Copy link
@aduth

aduth Mar 6, 2020

Member

Seems like these values shouldn't be changing in the first place, which is something we should be addressing higher up than in this component itself. Notably, I observe that value.start and value.end each change to undefined at some points, which I don't think would ever be expected while this component is mounted.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Mar 6, 2020

Hi @aduth, what this PR does is making sure anchorRect is never recomputed. I think it is a safe change because there is no relevant action one could take that may require the popover to change position without first closing the popover (e.g: one can not change the start and end while the popover is open).
It seems depending one on value.start, value.end, would be more "natural" but as you noted these values change unexpectedly so this change is probably a safer bet to cherry-pick into 5.4.

@jorgefilipecosta jorgefilipecosta changed the title Fix: Custom color formatter is "bouncing" when a color is manually changed. Fix: Custom color formatter may end up "bouncing" Mar 9, 2020
@jorgefilipecosta jorgefilipecosta merged commit 1347a8c into master Mar 9, 2020
3 of 4 checks passed
3 of 4 checks passed
build
Details
pull-request-automation
Details
pull-request-automation
Details
Travis CI - Pull Request Build Failed
Details
WordPress 5.4 Must Have automation moved this from Needs Review to Done Mar 9, 2020
@jorgefilipecosta jorgefilipecosta deleted the fix/custom-color-formatter-is-bouncing-when-a-color-is-manually-changed branch Mar 9, 2020
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.