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

Classic block: Add scroll to last edit position #23544

Merged
merged 7 commits into from
Jul 2, 2020

Conversation

glendaviesnz
Copy link
Contributor

@glendaviesnz glendaviesnz commented Jun 28, 2020

Temporary patch for #14134

Description

Adds a scollTo to the last bookmarked edit position when returning from onBlur caused by link and image edit dialogs

How has this been tested?

Just tested manually so far

Screenshots

Before:

After:

Types of changes

Adds a scrollIntoView call in order to bring selected node into view

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@glendaviesnz glendaviesnz added [Type] Bug An existing feature does not function as intended [Block] Classic Affects the Classic Editor Block labels Jun 28, 2020
@glendaviesnz glendaviesnz self-assigned this Jun 28, 2020
@github-actions
Copy link

github-actions bot commented Jun 28, 2020

Size Change: -404 B (0%)

Total Size: 1.13 MB

Filename Size Change
build/a11y/index.js 1.14 kB +1 B
build/api-fetch/index.js 3.4 kB -1 B
build/autop/index.js 2.82 kB +1 B
build/block-editor/index.js 109 kB +46 B (0%)
build/block-editor/style-rtl.css 10.7 kB -2 B (0%)
build/block-editor/style.css 10.7 kB -3 B (0%)
build/block-library/editor-rtl.css 7.41 kB -186 B (2%)
build/block-library/editor.css 7.41 kB -187 B (2%)
build/block-library/index.js 129 kB -133 B (0%)
build/block-library/style-rtl.css 8.04 kB +5 B (0%)
build/block-library/style.css 8.05 kB +5 B (0%)
build/block-serialization-default-parser/index.js 1.88 kB +1 B
build/blocks/index.js 48.2 kB +3 B (0%)
build/components/index.js 198 kB -23 B (0%)
build/components/style-rtl.css 15.9 kB +6 B (0%)
build/components/style.css 15.9 kB +5 B (0%)
build/compose/index.js 9.65 kB -1 B
build/core-data/index.js 11.4 kB -1 B
build/data-controls/index.js 1.29 kB +1 B
build/data/index.js 8.44 kB +6 B (0%)
build/date/index.js 5.47 kB +3 B (0%)
build/edit-navigation/index.js 9.88 kB +16 B (0%)
build/edit-post/index.js 303 kB -13 B (0%)
build/edit-post/style-rtl.css 5.51 kB +5 B (0%)
build/edit-post/style.css 5.51 kB +5 B (0%)
build/edit-site/index.js 16.6 kB -10 B (0%)
build/edit-site/style-rtl.css 3 kB -34 B (1%)
build/edit-site/style.css 3 kB -36 B (1%)
build/edit-widgets/index.js 9.32 kB -4 B (0%)
build/editor/index.js 44.8 kB +88 B (0%)
build/editor/style-rtl.css 3.85 kB +9 B (0%)
build/editor/style.css 3.86 kB +10 B (0%)
build/element/index.js 4.65 kB -1 B
build/i18n/index.js 3.56 kB -1 B
build/is-shallow-equal/index.js 710 B -1 B
build/keyboard-shortcuts/index.js 2.52 kB +5 B (0%)
build/keycodes/index.js 1.94 kB +1 B
build/list-reusable-blocks/index.js 3.12 kB -1 B
build/notices/index.js 1.79 kB -2 B (0%)
build/nux/index.js 3.4 kB -1 B
build/nux/style-rtl.css 671 B +8 B (1%)
build/nux/style.css 668 B +8 B (1%)
build/plugins/index.js 2.56 kB -1 B
build/rich-text/index.js 14 kB +1 B
build/server-side-render/index.js 2.67 kB -2 B (0%)
build/shortcode/index.js 1.69 kB -1 B
build/token-list/index.js 1.28 kB +1 B
build/wordcount/index.js 1.17 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/annotations/index.js 3.62 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.39 kB 0 B
build/block-directory/style-rtl.css 941 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-library/theme-rtl.css 730 B 0 B
build/block-library/theme.css 732 B 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.19 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-widgets/style-rtl.css 2.42 kB 0 B
build/edit-widgets/style.css 2.42 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/list-reusable-blocks/style-rtl.css 450 B 0 B
build/list-reusable-blocks/style.css 451 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B

compressed-size-action

@glendaviesnz glendaviesnz force-pushed the fix/classic-block-scroll-top-bug branch from d856fe5 to d319662 Compare June 29, 2020 03:32
@aaronrobertshaw
Copy link
Contributor

This tests ok for me. After adding a link or image the new element is scrolled into view.

As discussed internally, this still feels a little disorientating if the scroll position is being changed from when you made the edit. It is better than having to manually scroll back though. If we have the ability to save the scroll position at the point in time the edit is made and can scroll back to that, it might be a smoother experience.

@Copons
Copy link
Contributor

Copons commented Jun 29, 2020

I've noticed that the issue doesn't happen on Firefox (77.0.1, on macOS 10.15.5), so this fix would actually introduce the disorientating scroll change that @aaronrobertshaw noticed.

The suggestion of keeping track of the original scroll position is much more involved, but I agree that it would also result in a much cleaner experience.

@glendaviesnz
Copy link
Contributor Author

I've noticed that the issue doesn't happen on Firefox (77.0.1, on macOS 10.15.5), so this fix would actually introduce the disorientating scroll change

@Copons - seems to be a strange little chrome bug causing this that I can't figure a way around, so have added a check to only run if node is out of viewport, so it should now only run on Chrome with the link dialog on longer blocks - so at least minimises the disorientation 😄

@aaronrobertshaw
Copy link
Contributor

@glendaviesnz During our internal chat you mentioned that scrollTop was always returning 0. If tracking the original scroll position doesn't turn out to be possible, perhaps we might be able to lock the body scroll and then release after the Chrome quirk fires?

@pento
Copy link
Member

pento commented Jun 30, 2020

I've been doing some more testing on this, I think I've got to the root cause.

So, the classic block renders TinyMCE inline: rather than being a scrolling iframe, the TinyMCE area is the height of the content it contains. While scrolling looks like it's just being done to the classic block, it's actually the entire block list which is scrolling.

Now, when the link popup closes, it calls editor.focus() (where editor is the TinyMCE object, not the block editor). This ultimately calls focus() on the DOM element of the TinyMCE area, which Chrome evidently interprets as an instruction to make the top of that DOM element visible.

The easiest fix may actually be in Core, rather than in Gutenberg: changing the editor.focus() call to be editor.focus( true ) tells TinyMCE to set this as the active editor, but to skip the DOM focus() call.

@azaozz, @ellatrix: Can you give your thoughts on this issue, and possible fix?

@aaronrobertshaw
Copy link
Contributor

so have added a check to only run if node is out of viewport, so it should now only run on Chrome with the link dialog on longer blocks - so at least minimises the disorientation 😄

I've tested this PR works as described across Chrome, Firefox, Safari and Edge.

That said, it looks like @pento is onto something regarding the root cause. My vote would be for addressing the cause directly.

@glendaviesnz
Copy link
Contributor Author

glendaviesnz commented Jun 30, 2020

Thanks @pento, makes sense to fix this in core, but given the time that might take to filter back through I have modified this PR to better track the scroll position and correct it in instances where it differs on return of focus. I think we should include this temporary fix until the problem is resolved in core.

@aaronrobertshaw
Copy link
Contributor

I've re-tested this and it works well.

@glendaviesnz
Copy link
Contributor Author

👋 @talldan, @ntsekouras, @ellatrix - we are trying to clear a few of the more annoying Classic block bugs ahead of the deprecation of the Calypso classic editor ... so if there was any chance of getting a sign off on this one in the next few days that would be much appreciated - many thanks 😃

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

Thanks! I tested it and looks good!

We can include this workaround for now but I think an issue/fix should be made at Core though, since the root cause is pinpointed by @pento.

@glendaviesnz
Copy link
Contributor Author

thanks again @ntsekouras - have added a note about the core fix to the original issue

@glendaviesnz glendaviesnz merged commit 480dc0d into master Jul 2, 2020
@glendaviesnz glendaviesnz deleted the fix/classic-block-scroll-top-bug branch July 2, 2020 22:40
@github-actions github-actions bot added this to the Gutenberg 8.5 milestone Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Classic Affects the Classic Editor Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants