WordPress.org

Make WordPress Core

Opened 11 days ago

Closed 9 days ago

#42530 closed defect (bug) (fixed)

Unexpected focus lost when switching editor tabs

Reported by: ocean90 Owned by: pento
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.9
Component: Editor Keywords: has-patch dev-reviewed commit
Focuses: accessibility, administration Cc:

Description

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.

Attachments (3)

42530.diff (1.0 KB) - added by pento 10 days ago.
42530.2.diff (1.2 KB) - added by pento 10 days ago.
42530.3.diff (1.6 KB) - added by pento 10 days ago.

Download all attachments as: .zip

Change History (37)

#1 @ocean90
11 days ago

@biskobe Is this something you can look into?

#2 @afercia
11 days ago

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.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


11 days ago

#4 follow-up: @afercia
11 days ago

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

#5 @arush
11 days ago

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?

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


11 days ago

#7 @afercia
11 days ago

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.

#8 @adamsilverstein
11 days ago

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

#9 @afercia
11 days ago

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

This ticket was mentioned in Slack in #core by westonruter. View the logs.


11 days ago

#11 @afercia
11 days ago

@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.

#12 @adamsilverstein
11 days ago

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

#13 in reply to: ↑ 4 @SergeyBiryukov
11 days ago

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.

#14 @westonruter
10 days ago

  • Owner set to pento
  • Status changed from new to assigned

@pento
10 days ago

#15 follow-up: @pento
10 days ago

  • Keywords has-patch needs-testing added; 2nd-opinion needs-patch removed

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.

This ticket was mentioned in Slack in #core by pento. View the logs.


10 days ago

#17 @westonruter
10 days ago

  • Keywords dev-reviewed commit added; needs-testing removed

#18 @pento
10 days ago

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

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.

#19 @pento
10 days ago

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.

#20 in reply to: ↑ 15 @afercia
10 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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:

https://cldup.com/Z0I204LjFN.jpg

Switch to Text mode:

https://cldup.com/S2YA4I_6Us.jpg

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

https://cldup.com/oTTYBQhbKs.jpg

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

https://cldup.com/0cXJ_3Rctn.jpg

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.

This ticket was mentioned in Slack in #core by afercia. View the logs.


10 days ago

@pento
10 days ago

#22 @pento
10 days ago

  • Keywords needs-testing added; dev-reviewed commit removed

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.

This ticket was mentioned in Slack in #core-committers by melchoyce. View the logs.


10 days ago

#24 @westonruter
10 days ago

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

#25 follow-up: @westonruter
10 days ago

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.

#26 in reply to: ↑ 25 @afercia
10 days ago

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
10 days ago

#27 @pento
10 days ago

  • Keywords dev-feedback commit added; needs-testing removed

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.

This ticket was mentioned in Slack in #core by pento. View the logs.


10 days ago

#29 @westonruter
10 days ago

  • Keywords dev-reviewed added; dev-feedback removed

#30 @westonruter
10 days ago

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

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.

#31 @westonruter
10 days ago

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.

#32 follow-up: @afercia
9 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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:

https://cldup.com/tPgw-6wpIk.png

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

https://cldup.com/XovMqyzv1V.png

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

https://cldup.com/gitpyBHrYM.png

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

https://cldup.com/x4UiZEBviz.png

Reopening for visibility. See also #42553.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


9 days ago

#34 in reply to: ↑ 32 @westonruter
9 days ago

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

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.

Note: See TracTickets for help on using tickets.