Unexpected focus lost when switching editor tabs

[ocean90]

Probably introduced by #41962/#42029.

Steps to reproduce:

• Go to the edit screen of a post
• Activate the text tab
• Switch back to visual tab

Video: ​https://cloudup.com/iP2mf9W4QOS

Expected behaviour: The editor doesn't get the focus and no auto-scrolling happens.

[ocean90]

@biskobe Is this something you can look into?

[afercia]

Seems to me this is part of the changes to keep the selection when switching editor modes, see [41630].

However, moving focus programmatically is not a good practice from an accessibility perspective. Same for scrolling the whole page.

Moving focus programmatically is always an assumption and may not be what users really want. It generally assumes one workflow, which may be the desired workflow or not. Scrolling the whole page shouldn't happen. Maybe the content within the editor should scroll, if necessary, to reveal the selection, if any.

I'd say when there's no selection or the editor had no focus, nothing of all this should happen.

Seems to me when the "auto-save-draft" notice appears, the scrolling happens also on first page load, which is very confusing.

[afercia]

P.S. seems to me all the new functions introduced for this feature miss a @since notation in their docblocks.

[arush]

Unexpectedly switching contexts like this is a huge deal. For screen reader users, the page will need to be re-rendered in the virtual buffer whenever scrolling happens. For users with cognitive disabilities, it can be a simple matter of losing one's place and having to re-find it. Is there any way the release can be held back until this is fixed?

[afercia]

Additional note about documentation standards: many multiline inline comments introduced in [41630] use a double asterisk, e.g.:

/**
 * If the selection is on a shortcode with Live View, TinyMCE creates a bogus markup,
 * which we have to account for.
 */

This doesn't meet the documentation standards. Multiline comments should use just a single asterisk. The double asterisk is only for docblocks.

[adamsilverstein]

I posted a patch reverting r41783 here - https://core.trac.wordpress.org/attachment/ticket/42531/42531.diff - in my testing this resolves the issue.

[afercia]

Note: one more coding standard that isn't met across [41630]:

​https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#declaring-variables-with-var

Each function should begin with a single comma-delimited var statement that declares any local variables necessary

[afercia]

@adamsilverstein with the patch from #42531 I still see the focus being moved and the pafe scrolling so that wouldn't solve the usability and accessibility issue here.

[adamsilverstein]

Ok thanks for testing. I’ll take a closer look.

[SergeyBiryukov]

Replying to afercia:

P.S. seems to me all the new functions introduced for this feature miss a @since notation in their docblocks.

Summarized the docblock/standards comments in #42505.

[pento]

The existing behaviour prior to 4.9 is:

• When switching to the Text view, the <textarea> is not focussed.
• When switching to the Visual view, the editor is focussed, with the cursor placed at the start of the content.

The new behaviour in trunk is for switching to either to focus the relevant editor box, at the point that was previously selected. This behaviour can be disabled by changing the wp_keep_scroll_position TinyMCE setting to false, the tiny_mce_before_init filter is the best option for doing that globally.

So, 42530.diff​ is mostly a tweak to the scrolling behaviour:

• When switching to the Text view, wait until after the Visual editor element has been hidden before focussing the <textarea>.
• When switching to the Visual view, only scroll if the selection is not on the current screen.

[pento]

In 42175:

Editor: When switching Editor tabs, don't scroll unnecessarily.

• When switching to the Text view, wait until after the Visual editor element has been hidden, before focussing the <textarea>.
• When switching to the Visual view, only scroll if the cursor is not visible on the current screen.

Fixes #42530.

[pento]

In 42176:

Editor: When switching Editor tabs, don't scroll unnecessarily.

• When switching to the Text view, wait until after the Visual editor element has been hidden, before focussing the <textarea>.
• When switching to the Visual view, only scroll if the cursor is not visible on the current screen.

Merge of 52175 to the 4.9 branch.

Fixes #42530.

[afercia]

Replying to pento:

The existing behaviour prior to 4.9 is:

• When switching to the Text view, the <textarea> is not focussed.
• When switching to the Visual view, the editor is focussed, with the cursor placed at the start of the content.

This is not entirely true. In 4.8.3 when switching to VIsual mode, the editor is not focused. In 4.7 it is. Worth noting this "auto focus" is a TinyMCE behavior, not something WordPress does. It was discussed at length because it's highly problematic for a11y, see for example #30490. I'd agree it should be discussed separately as it seems something changed in TinyMCE and the current behavior after [42176] matches the behavior in 4.7. Will open a new ticket.

However,

I'm sorry but the current fix for the scrolling behavior doesn't seem a proper fix to me. Here's what happens for me, tested on 2 different machines and with different browsers: to reproduce

• in the edit post Screen Options, enable all the metaboxes
• uncheck "Enable full-height editor and distraction-free functionality."
• edit a post that is long enough (it happens also with shorter posts but a longer one makes reproducing easier)

In my case, the edited post is made of 1256 words:

Switch to Text mode:

Select a word at the bottom of the post, then switch back to Visual mode:

The editor page is now completely scrolled at the bottom, and the editor area is out of view. I guess this is not the best experience WordPress wants for users.

Additionally, to keep the selection, some "placeholder" content gets inserted in the post. As a consequence, when switching mode, the beforeunload confirmation dialog to warn users there are unsaved changes kicks in even if the user hasn't made any change to the post content. To reproduce:

• edit any post
• switch the editor to Text mode
• switch back to Visual mode
• navigate away from the page
• the confirmation dialog appears, warning there are unsaved changes

Of course, this would be very confusing for users since there were no intentional changes to the post content.

Overall, I still think this new feature hasn't been tested enough and at this point of the release it should be just reverted. To recap:

• the weird scrolling behaviors highlighted above significantly worsen the user experience
• some bugs were discovered at the last minute, see also #42531 and there could be more. It just needs more testing, and I'd recommend to revert and postpone to 4.9.1
• the code introduced for this feature doesn't meet the coding standards. Honestly, I'm a bit surprised to hear that the current standards are a minor concern because "eslint is coming soon". This is not a great argumentation. ESLint could be a thing in a few weeks, or in six months, or a year. In the meantime, any new code introduced in core should stick to the current coding standards.
• the code introduced for this feature doesn't meet the documentation standards.

[pento]

42530.2.diff​ fixes the scrolling issue, that's as far as I got for my evening.

The beforeunload message is likely caused by the changes in [42175], given the lack of reports of this prior to now.

[westonruter]

For my part I cannot reproduce the scrolling issue. I can, however, reproduce the beforeunload issue.

[westonruter]

Nevertheless, I'm also seeing beforeunload warnings when switching between the Text and Visual tabs in 4.8.3 and 4.7. So I don't think this is a new regression.

[afercia]

Replying to westonruter:

I'm also seeing beforeunload warnings when switching between the Text and Visual tabs in 4.8.3 and 4.7. So I don't think this is a new regression.

Cannot reproduce in 4.8.3 or 4.7.

[pento]

In 42530.3.diff​:

When switching back to the Visual editor, focusHTMLBookmarkInVisualEditor() removes the bookmark elements correctly from TinyMCE, but doesn't remove them from the <textarea>. Unfortunately, autosave doesn't get the content from TinyMCE, it goes directly to the <textarea>, so it gets the HTML with the bookmark elements still in there.

Calling editor.save() forces TinyMCE to write the content without the bookmark elements to the <textarea>, so that autosave doesn't see those elements.

[westonruter]

In 42183:

Editor: Improve scrolling behavior and prevent autosave logic from causing dirty state when just switching between Visual and Text tabs.

Props pento.
See #41962, #42029.
Fixes #42530 for trunk.

[westonruter]

In 42184:

Editor: Improve scrolling behavior and prevent autosave logic from causing dirty state when just switching between Visual and Text tabs.

Props pento.
See #41962, #42029.
Fixes #42530 for 4.9.

[afercia]

The fix above doesn't seem to work properly, and it behaves inconsistently across browsers. To reproduce:

• Uncheck "Enable full-height editor and distraction-free functionality." in the Screen options
• using Chrome, edit a long post starting in the "Visual mode"
• select a word that occurs at about the middle of the post content:

• switch to "Text mode": no scrolling happens and the selected word stays out of view:

• switch back to "Visual mode"
• the selected word is almost completely hidden at the top of the content visible area:

Using Firefox instead, it behaves a bit better: at least the selected word is visible when switching from VIsual to text:

Reopening for visibility. See also #42553.

[westonruter]

Replying to afercia:

The fix above doesn't seem to work properly, and it behaves inconsistently across browsers. To reproduce:

We'll follow this up in #42561 during 4.9.1.