WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#29338 closed defect (bug) (fixed)

Awkward jump when clicking text tab after scrolling visual editor

Reported by: gcorne Owned by: helen
Milestone: 4.0 Priority: normal
Severity: normal Version: 4.0
Component: Editor Keywords: has-patch dev-reviewed
Focuses: ui Cc:

Description

After scrolling the visual editor, there is an awkward jump when clicking the text tab.

See https://cloudup.com/cEY-y18v5_N for a demonstration

Attachments (11)

29338.diff (369 bytes) - added by stephdau 6 years ago.
Proof of concept for comment:8
29338.2.patch (983 bytes) - added by azaozz 6 years ago.
29338.3.patch (1.2 KB) - added by azaozz 6 years ago.
29338.4.patch (1.4 KB) - added by azaozz 6 years ago.
29338.patch (691 bytes) - added by iseulde 6 years ago.
29338.5.patch (770 bytes) - added by iseulde 6 years ago.
29338.6.patch (742 bytes) - added by iseulde 6 years ago.
29338.7.patch (1.5 KB) - added by iseulde 6 years ago.
29338.8.patch (1.6 KB) - added by iseulde 6 years ago.
over-scroll.png (119.3 KB) - added by azaozz 6 years ago.
29338.9.patch (1.4 KB) - added by azaozz 6 years ago.

Download all attachments as: .zip

Change History (46)

This ticket was mentioned in IRC in #wordpress-dev by stephdau. View the logs.


6 years ago

#2 @stephdau
6 years ago

I don't have code specifics yet, but testing seems to lead me to believe it's directly tied to reaching the bottom-most of the page while in visual mode.

One can notice a drastic size difference between the visual and text editors when switching from one to the other while not yet at the bottom of the page. The only context in which I can reproduce Greg's bug is after reaching the bottom edge of the page.

See https://cloudup.com/cPfihqdfbV2

#3 @stephdau
6 years ago

When the bug happens, we can also see that "calculations" meant to keep the sidebar off the footer are also failing.

https://cloudup.com/c7UYGLHoAp9

#4 @stephdau
6 years ago

  1. Switching from the visual editor to the text editor triggers textEditorResize().
  2. The text editor is never as tall as the visual one, with the same content, since same but unstyled.
  3. We resize the editor to be the height of the non-WYSIWYG text content (see editor status bar moving up.
  4. But we don't (or fail to) recalculate where the top/bottom of the pinned sidebar should be in relation to the current editor's size (I think).

EG: switching hiddenHeight for textEditorHeight at https://core.trac.wordpress.org/browser/trunk/src/wp-admin/js/editor-expand.js?rev=29656#L144 stops the Awkward jump from happening, but the text editor ends with a slew of whitespace at its bottom that the user cannot access (totalling a height equal to the discrepancy between the visual and text editors heights). So not quite a solution.

Another hack to "make the problem go away) is to force a scrollTop(0) when switching to the text editor (aka, in mceHide()), which recalculates the editor and sidebar positions, and make the issue moot. Still not great.

Still currently at a loss for an actual fix. :)
Seems to need another clever // don't get over the footer for the sidebar, upon switching/resize.

Last edited 6 years ago by stephdau (previous) (diff)

#5 @azaozz
6 years ago

Was thinking about this too: perhaps we can scroll up as many px as the difference between Visual to Text. This should take care of the sidebar position too.

#6 @stephdau
6 years ago

Good idea, will try.

#7 @stephdau
6 years ago

Better, but not quite: https://cloudup.com/cxJXT5WG4Ar
Which is surprising, because I thought it'd be enough to at least trigger a proper recalculation of the UX element's position (at least the sidebar in relation to the footer).

#8 @stephdau
6 years ago

So, what's "disturbing" with my suggestion to scroll to the top of the page is that it brings back the title and slug fields back in view. Another solution could be to scroll to the "top of the editor", which can be defined by the bottom of the slug area. This should be invisible to the user, since the editor's toolbar is static, and should meet the height and position requirements of the sidebar.

@stephdau
6 years ago

Proof of concept for comment:8

#9 follow-up: @stephdau
6 years ago

attachment:29338.diff is a rough proof of concept (hard-coded position instead of dynamic) of what I suggested in comment:8.

See behavior in https://cloudup.com/cW8i7vbaPWR video.

Last edited 6 years ago by stephdau (previous) (diff)

#10 @stephdau
6 years ago

PS: because what it does is scroll up, the sidebar positioning case that's triggered does make the sidebar pin at the top, not the bottom, which would be ideal, but going against the grain of what needs to happen when actually scrolling up on purpose.

@azaozz
6 years ago

#11 in reply to: ↑ 9 @azaozz
6 years ago

Replying to stephdau:

Yep, calculating the height difference and scrolling by that much seems to work well too. To fix the sidebar, we need to do the same trick as when toggling postboxes: scroll in one direction, adjust, scroll in the other direction and adjust again. 29338.2.patch seems to work well here.

@azaozz
6 years ago

#12 @azaozz
6 years ago

29338.3.patch also checks if the difference in heights would mean scrolling all the way to the top of the page and limits it to scrolling to the top of the editor. Fixes this in cases where the Text editor is really short and the Visual is really tall, for example: a gallery post.

#13 @azaozz
6 years ago

  • Milestone changed from Awaiting Review to 4.0

@azaozz
6 years ago

#14 @azaozz
6 years ago

In 29338.4.patch: also add some delay before calculating heights as in some cases the pre-switch heights are returned.

#15 @helen
6 years ago

  • Keywords has-patch commit added

Looks good - the slight delay is noticeable if you're looking for it, but it's a lot better than having the editor jump off the screen.

#16 @iseulde
6 years ago

I still notice some weird end positions... When I have a post full of views and switch to text mode form the bottom, I end up with the text content scrolled just one line away from the top and the toolbar out of sight (the text editor is too small to have the fixed toolbar).

I still think it'd be better to just scroll to the top of the editor whenever the top toolbar is pinned, but the current solution is already way better than it was. :)

@iseulde
6 years ago

#17 @iseulde
6 years ago

How about this?

@iseulde
6 years ago

This ticket was mentioned in IRC in #wordpress-dev by azaozz. View the logs.


6 years ago

@iseulde
6 years ago

#19 follow-up: @azaozz
6 years ago

29338.6.patch looks good here. Changes the behavior a bit, now switching Visual => Text will always scroll to the editor top.

#20 in reply to: ↑ 19 @adamsilverstein
6 years ago

Replying to azaozz:

29338.6.patch looks good here. Changes the behavior a bit, now switching Visual => Text will always scroll to the editor top.

29338.6.patch looks really good, nice!

#21 @iseulde
6 years ago

I think 29338.6.patch is the way to go. Always scrolling to the top when switching mode makes mosts sense to me, it's impossible to scroll exactly to the corresponding line in the text editor anyway. Switching modes is disorientation in itself.

#22 @nacin
6 years ago

In 3.9, each editor's current scroll location is remembered. I think scrolling to the top might make it really annoying to do certain things like switching multiple times in a row to adjust something, for example.

#23 @iseulde
6 years ago

So you want to remember the position for each editor and switch to that position if possible, and otherwise to the top? Sounds good to me too. I'll try making another patch. Can't remember that in 3.9 though.

#24 @iseulde
6 years ago

Okay, right, in 3.9 the browser remembers the scroll position for the textarea and TinyMCE focuses on the last caret position... Trying to make a patch now...

@iseulde
6 years ago

@iseulde
6 years ago

#25 @iseulde
6 years ago

  • Keywords needs-testing added; commit removed

New patch: just remember the position of the mode and set that position back after switching back.

#26 @markjaquith
6 years ago

It's gonna be a little weird if you switch, scroll, then switch back. but I think most switches are either quick "I want to insert some custom HTML here and then go back to writing" or long-lasting switches. Saving the scroll states individually works great for quick diversions into another mode, and doesn't really matter for long-lasting switches.

I would defer to @avryl on the safety of this code change. In my testing, I didn't have any functional problems.

#27 @helen
6 years ago

  • Keywords commit added; needs-testing removed

There's more redraw here than before with the patch, but it is less jarring than having things jump off the screen. I also agree with Mark that the scroll state saving works out well for the most common use cases.

#28 @nacin
6 years ago

  • Keywords dev-reviewed added

Would like azaozz to review this, but it can land for the soak.

#29 @helen
6 years ago

  • Owner set to helen
  • Resolution set to fixed
  • Status changed from new to closed

In 29693:

Editor scrolling: Remember and restore scroll position for visual and text modes.

This prevents the editor from jumping off screen when the rendered visual content is significantly taller than plain text, e.g. when there are views present.

This is also a close restoration of 3.9 behavior, some of which was browser-specific.

props avryl.
fixes #29338.

@azaozz
6 years ago

#30 @iseulde
6 years ago

  • Keywords needs-patch added; has-patch commit dev-reviewed removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Needs checks to see if the old position is within the editor's range.

@azaozz
6 years ago

#31 @azaozz
6 years ago

29338.9.patch reverts 29338.8.patch and applies 29338.6.patch. This always scrolls the Text editor to the top and prevents all other edge cases.

#32 @iseulde
6 years ago

  • Keywords has-patch added; needs-patch removed

Looks good! :)

#33 follow-up: @helen
6 years ago

  • Keywords dev-reviewed added

Go for it.

For reference, this does lose the text editor scroll position, which was browser-specific behavior (I never saw it in Firefox, but it worked in Chrome). Still supports the use case of flipping quickly into text mode to insert some HTML and going back to visual, which does retain scroll position, quite well.

#34 @azaozz
6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 29698:

Editor expand: revert back to scrolling to the editor top when switching Visual to Text, fixes #29338

#35 in reply to: ↑ 33 @azaozz
6 years ago

Replying to helen:

For reference, this does lose the text editor scroll position, which was browser-specific behavior (I never saw it in Firefox, but it worked in Chrome).

Yeah, that would be nice to have. Created #29492 for an enhancement in 4.1.

Note: See TracTickets for help on using tickets.