Classic Editor: the distraction-free button breaks keyboard navigation

[afercia]

This appears to be a long standing bug in the Classic Editor. To reproduce:

• edit a post in the Classic Editor
• make sure in the Screen Options > "Enable full-height editor and distraction-free functionality" is not checked
• click in the editor content
• press Alt+F10 to move focus to the TinyMCE toolbar
• navigate through the buttons either with the Tab key or the arrow keys
• navigating forwards on the first buttons row: you're stuck on the Toggle Toolbar button
• navigating backwards on the second buttons row: you're stuck on the Strikethrough button

Go in the Screen Options again and check "Enable full-height editor and distraction-free functionality":

• the "distraction-free" button appears in the toolbar top right
• repeat the steps above: navigation through the buttons now works correctly
• however, DOM order and visual/tab order mismatch

TinyMCE implements its own mechanism to navigate through the buttons, meaning that's not the native browsers behavior. Turns the "distraction-free" button is hidden with display: none and the TinyMCE navigation mechanism doesn't like hidden buttons.

Additionally, the "distraction-free" button is placed in the markup before the "Toggle Toolbar" button but visually it's the last one in the top right. For accessibility, DOM order and visual order must match when the navigation sequence "affects meaning or operation".

To solve the broken keyboard navigation, I'd propose to just keep the button always visible:

When the button is disabled, TinyMCE already handles what is necessary: the button is greyed-out, it does nothing, and has an aria-disabled attribute. This matches the standard style and behavior of all the other disabled buttons in TinyMCE (see Undo/Redo in the screenshot below):

[afercia]

46640.diff​ :

• keeps the button visible
• makes the DOM order match the visual order

Also, while looking into this, I've noticed this couple lines related to backwards compatibility for the old distraction free mode:
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/script-loader.php?rev=44986&marks=927-928#L926

// Back-compat for old DFW. To-do: remove at the end of 2016.
$scripts->add( 'wp-fullscreen-stub', "/wp-admin/js/wp-fullscreen-stub$suffix.js", array(), false, 1 );

Maybe worth considering for removal. /Cc @azaozz

[afercia]

Moving to 5.3 as per today's bug-scrub, with the hope it can be considered for 5.2 even if it's in beta 1.

[d4z_c0nf]

We've spent some time today, along with @afercia, @realloc and other Italian contributors, at the wctrn2019 contributor day analyzing this patch in order to reconstruct the original intent.

We'd agree to not alter the visual order of the editor toolbar buttons yet ensuring the correct keyboard navigation, even if that led to what can be considered a not elegant PHP code.

[afercia]

46640.2.diff​ tries to simplify things a bit.

Looking at the original code, the wp_adv button is always added to the toolbar. Let's just add it to the default array of buttons and then:

• when not mobile:
  • when free-distraction mode is available (which I guess it's the most common case) just add the dfw button at the last position
  • when it's not available, remove the dfw button, then add fullscreen and add again dfw as last button

This way, the order of the buttons is the one we need and the availability of the buttons is the intended one. Also, added minor CSS adjustments to make the dfw button correctly aligned with the other buttons.

@azaozz when you have a chance, I'd greatly appreciate your review. I'd like to move this to 5.2 as it's a long standing bug that would be nice to fix soon :)

[afercia]

Note: For anyone willing to test with the distraction-free tool disabled, just add the following to your theme's functions.php:

add_filter( 'wp_editor_expand', '__return_false' );

[azaozz]

@afercia thanks for the patches! :)

I think 46640.diff handles this better. The current logic there is:

1. If on mobile device, do not add either the dfw or the fullscreen button.
2. If not on mobile device, add dfw if it is in the settings, otherwise add fullscreen.
3. Always add the wp_adv button to the end.

46640.diff reorders the buttons so the "visual" order matches the DOM order, i.e. wp_adv before dfw or after fullscreen depending on which is used.

What I'm uncomfortable with is not hiding the dfw button (when DFW mode is disabled) but instead letting MCE to set it to disabled. Then the users will see an always disabled button (with tooltip, shortcut, etc.) and most will have no idea how to enable it. That also makes the behaviour inconsistent with the Text tab where the dfw button is hidden.

I agree, it's a bit "hacky" to hide that button with css. This was done to maintain the behaviour in both Visual and Text tabs when turning DFW on/off "dynamically" by clicking the "Enable full-height editor..." checkbox in the screen options.

For 5.2 I'm thinking we should fix at least the DOM ordering problem. It may be possible to add and remove the dfw button "dynamically" instead of hide it with css. Can look at this again but if I remember right last time we checked, it had some issues in TinyMCE.

The other option is use only the Text tab dfw button (yep, there are two of them) and completely remove the one in TinyMCE. Then we will still need to show it "visually" on the MCE toolbar despite that it's at completely different place in the DOM.

[afercia]

@azaozz thanks for looking into this.

The main issue is the broken keyboard navigation. The button hidden with display: none breaks basic accessibility: users who use the keyboard or technologies that emulate the keyboard can hardly navigate the toolbar buttons. Fixing only the order won't help so much if users can't use the toolbar to start with :)

What I'm uncomfortable with is not hiding the dfw button (when DFW mode is disabled) but instead letting MCE to set it to disabled. Then the users will see an always disabled button (with tooltip, shortcut, etc.)

After all, this is the default behavior in TinyMCE.

and most will have no idea how to enable it. That also makes the behaviour inconsistent with the Text tab where the dfw button is hidden.

I tend to see this more as a design issue. However, breaking keyboard accessibility to address a design issue is far from ideal.

If there's a better way to fix keyboard navigation, I'm all for it :)

Unless I've messed up something, 46640.diff​ and 46640.2.diff​ do basically the same thing. They both unhide the button. In 46640.2.diff​ I've just tried to avoid nested if/else.

[azaozz]

Replying to afercia:

The main issue is the broken keyboard navigation.

Yeah I see. Tried to fix this in 46640.3.diff. Disabling the button through TinyMCE's API seems to exclude it from the (js controlled) navigation there. Please test :)

46640.diff​ and 46640.2.diff​ do basically the same thing.

Right, but the PHP changes in 46640.diff are a bit more readable imho, can easily see which button is after which :)

In 46640.3.diff:

• Based on the previous diffs.
• Show and hide the dfw button through the editor API.

[afercia]

Oh yes 46640.3.diff​ is the best of both worlds: fixes keyboard navigation and preserves the intended UI. Tested, work nicely. Thanks @azaozz !

Any objections to move this to 5.2, which means committing very soon? 😄

the PHP changes in 46640.diff are a bit more readable

Yep, it was a readability vs. stylistic thing. If the nested if/else is not a concern, I have no strong preferences thanks!

[azaozz]

Replying to afercia:

Yeah, seems to work pretty well here too. Lets add this :)

[azaozz]

In 45172:

TinyMCE: Fix keyboard navigation when the dfw button is present but hidden. Ensure that button is added last in the DOM to match where it appears visually.

Props afercia, azaozz.
Fixes #46640.