Edit Post: Remove menu toggling on checkbox, radio buttons

[aduth]

This pull request seeks to avoid closing the More Options menu when selecting one of its checkbox or radio items (specifically, the options under "View" and "Editor" subheadings).

I could not find an associated issue, though one may exist.

Per WAI-ARIA Authoring Practices 1.1 3.15 Menu or Menu bar:

• Space:
  • (Optional): When focus is on a menuitemcheckbox, changes the state without closing the menu.
  • (Optional): When focus is on a menuitemradio that is not checked, without closing the menu, checks the focused menuitemradio and unchecks any other checked menuitemradio element in the same group.

(Emphasis mine)

https://www.w3.org/TR/wai-aria-practices/#menu

Testing Instructions:

Note: The current behavior is still not ideal. Focus is lost due to rendering behavior of MenuItem, specifically in that it renders a new element of a different type (Button vs. IconButton) depending on whether the menu item is checked (source). This needs to be addressed.

In the meantime: The menu should at least no longer close when pressing these buttons, if either by keyboard activation or a direct mouse click.

[aduth]

Another related issue surfaced by this behavior change (I think explaining some of the E2E failures): Similar to the unmounting and remounting of the menu items causing a focus loss, there's a re-mounting of the title component which causes its autoFocus to forcibly take effect again, drawing focus out of the menu as soon as the mode is changed.

For both this and the menu item, ideally there's no remounting which occurs. The title is a bit more of a difficult issue to solve, since both VisualEditor and TextEditor components render their own title as part of their virtual hierarchy. Since the title is present regardless of mode, it makes me wonder if it ought not be pulled out as common separate into the general layout. That said, there may be a reason I've not yet discovered for it being implemented the way it was.

For the behavior of the autofocus itself (previously #9608, #3828), it seems like it should only ever take effect once per page session. Thus, regardless of whether the component was to be remounted, the fact that autoFocus kicks any time after the initial page load is not desirable.

[aduth]

Since the title is present regardless of mode, it makes me wonder if it ought not be pulled out as common separate into the general layout. That said, there may be a reason I've not yet discovered for it being implemented the way it was.

I explored this further. On the JavaScript side, it's very possible, and generally speaking I think the consolidation could prove to be a valuable refactoring, aligning the two modes. In practice, however, I struggled with it due to existing styles which are very much tailored to positioning the titles within specific characteristics of each mode (cc @jasmussen).

Work-in-progress patch: https://gist.github.com/aduth/aa2ebb9470d5ccb32aa466fb0841dce0

Since I expect that to be a larger effort, for the purpose of this pull request it may be most effective to tackle directly the issue that autoFocus occurs more than just on the initial page load.

[jasmussen]

Thanks for the ping, I will take a deeper look first thing Monday.

Specifically to the discussions around the title, though, I would overall recommend not optimizing too much towards the current implementation. In the not too long term, I would hope that we can turn it into a block that can be removed, and overall simply have an additional redundant UI for changing the title. One set of suggestions are being discussed in #11553 (comment), but another option is to add this redundant UI in the sidebar in context of the permalink box, and in the pre-publish flow as a field there.

[jasmussen]

This branch:

Master:

What a wonderful PR. Outside of a code sanity check by someone better than me, this is a great PR and a much improved experience. 👍 👍

[gziolo]

There is e2e test failure:

FAIL packages/e2e-tests/specs/editor-modes.test.js (10.205s)
  ● Editing modes (visual/HTML) › the code editor should unselect blocks and disable the inserter
    expect(received).not.toBeNull()
    Received: null
      109 | 		await page.click( '.edit-post-sidebar__panel-tab[data-label="Block"]' );
      110 | 		const noBlocksElement = await page.$( '.block-editor-block-inspector__no-blocks' );
    > 111 | 		expect( noBlocksElement ).not.toBeNull();
          | 		                              ^
      112 | 
      113 | 		// The inserter is disabled
      114 | 		const disabledInserter = await page.$( '.block-editor-inserter > button:disabled' );
      at Object.toBeNull (specs/editor-modes.test.js:111:33)
      at tryCatch (../../node_modules/regenerator-runtime/runtime.js:62:40)
      at Generator.invoke [as _invoke] (../../node_modules/regenerator-runtime/runtime.js:288:22)
      at Generator.prototype.(anonymous function) [as next] (../../node_modules/regenerator-runtime/runtime.js:114:21)
      at asyncGeneratorStep (specs/editor-modes.test.js:9:103)
      at _next (specs/editor-modes.test.js:11:194)```

I'm sure we need to tweak it after we changed the behavior of More Menu items.

@jasmussen, there was also a suggestion that we should not close More Menu when opening modals. This would let keyboard users navigate back to where they started when opening a modal. I did some testing and we would only need to improve handling around x (close button) which forces both the modal and More Menu to close. Using esc keyboard shortcut closes only the modal and moves focus back to the menu item which opened the modal.

[jasmussen]

@jasmussen, there was also a suggestion that we should not close More Menu when opening modals. This would let keyboard users navigate back to where they started when opening a modal. I did some testing and we would only need to improve handling around x (close button) which forces both the modal and More Menu to close. Using esc keyboard shortcut closes only the modal and moves focus back to the menu item which opened the modal.

This sounds very good to me, and sounds accurate, since when you escape the modal you get back to where you initiated the modal, which in my understanding is correct. But I'll let the "needs accessibility feedback" label stick around in case anyone wants to confirm.

[afercia]

Quickly discussed during today's accessibility bug-scrub. The proposed change makes perfectly sense. Also the improvements to the post title autofocus and rendering behavior of MenuItem would be great.

[gziolo]

I fixed the failing test (I hope Travis is happy as well). It all looks great, I really like this change as it makes More Menu easier to use overall, not only for keyboard users. 🥇

[aduth]

Apparently I overlooked the guideline immediately preceding the one I'd quoted, where it specifies the Enter behavior as unique from Space in how it makes no distinction of the menu item role, and recommends to always close the menu.

The full text:

• Enter:
  • When focus is on a menuitem that has a submenu, opens the submenu and places focus on its first item.
  • Otherwise, activates the item and closes the menu.
• Space:
  • (Optional): When focus is on a menuitemcheckbox, changes the state without closing the menu.
  • (Optional): When focus is on a menuitemradio that is not checked, without closing the menu, checks the focused menuitemradio and unchecks any other checked menuitemradio element in the same group.
  • (Optional): When focus is on a menuitem that has a submenu, opens the submenu and places focus on its first item.
  • (Optional): When focus is on a menuitem that does not have a submenu, activates the menuitem and closes the menu.

Seems it could be an improvement worth exploring further if it better aligns with expected interactions.

[aduth]

Follow-ups for the issues described in #14456 (comment) :

• Edit Post: More Options menu toggle buttons cause focus loss when toggled #14849 Edit Post: More Options menu toggle buttons cause focus loss when toggled
• Edit Post: Focus wrongly transitions to post title when toggling Visual/Code  #14850 Edit Post: Focus wrongly transitions to post title when toggling Visual/Code