New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sidebar: Avoid focus loss on active tab change #10917

Merged
merged 11 commits into from Oct 26, 2018

Conversation

@aduth
Member

aduth commented Oct 22, 2018

Partially addresses: #8079

This pull request seeks to resolve an issue of focus loss when changing active sidebar tabs for the editor settings sidebar ("Document", "Block").

Currently, this pull request includes all but the fix itself. Notably, included is an end-to-end test case covering the expected behavior, and a global test event handler protecting against unexpected focus loss.

Update: It has been implemented with 72569ab.

Implementation notes:

Focus loss occurs because each specific sidebar renders the entirety of the .edit-post-wrapper element. For editor settings, this also includes the tab header as well.

The bug occurs because React is unable to reconcile the two element trees, and is therefore forced to unmount and remount an entirely new sidebar wrapper.

An initial attempt here was to invert rendering, having each specific sidebar populating only the Fill of the sidebar component, whereas the Slot would be responsible for rendering the .edit-post-wrapper wrapper element. This has the added advantage of fixing an issue with the application of withFocusReturn, as currently this is broken since it's applied to the Fill (non-DOM) element. An issue with this approach is in respecting the aria-label application provided by each specific sidebar.

Testing instructions:

Verify that no focus loss occurs when changing tabs in the sidebar.

  1. Navigate to Posts > Add New
  2. Click "Document"
  3. Press Tab
  4. Press Spacebar
  5. Note that focus remains on the "Block" tab after activation

Ensure end-to-end tests pass:

npm run test-e2e
@gziolo

This comment has been minimized.

Member

gziolo commented Oct 23, 2018

Thanks @aduth, this is very helpful. I will try to find some time today to start working on a fix.

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 24, 2018

I consolidate both sidebars into one component with 678819 and it solves the issue. It reuses the same Fill with the updated implementation so it is able to re-render (instead of unmount and mount) even when the active tab changes.

@gziolo gziolo requested a review from vindl Oct 24, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 24, 2018

@vindl - just letting you know that this changes a bit the logic inside the edit-post's Layout component.

@gziolo gziolo requested a review from WordPress/gutenberg-core Oct 24, 2018

@gziolo gziolo added this to the 4.2 milestone Oct 24, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 25, 2018

@afercia this should be ready to verify 👍

E2e tests fail but this is unrelated. I bet focus loss check should be enabled only for tests which use keyboard navigation, not to all of them.

@afercia

This comment has been minimized.

Contributor

afercia commented Oct 25, 2018

@gziolo thanks. I'm not seeing the sidebar at all. When I click the Settings button, I see the is-sidebar-open CSS class gets applied but nothing is rendered, no sidebar. What I'm doing wrong?

* Binds to the document on page load which throws an error if a `focusout`
* event occurs without a related target (i.e. focus loss).
*/
export function observeFocusLoss() {

This comment has been minimized.

@gziolo

gziolo Oct 25, 2018

Member

@aduth - it seems to fail too many test suites. I moved it to utils and applied only for the sidebar. Let's open a follow-up and this assertion where it fits.

This comment has been minimized.

@gziolo

gziolo Oct 26, 2018

Member

I also reverted changes applied to setup-test-framework.js with 46a078f.

There is this issue that afterEach removes all page listener but afterAll needs them, too.

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 25, 2018

@afercia it was my mistake, it should be fixed with d5ff172#diff-b006c664acf96e390063ef30c215b267R60 - I did a typo when refactoring code at the end of the day ...

@afercia

This comment has been minimized.

Contributor

afercia commented Oct 25, 2018

@gziolo LGTM 👍 thanks!

@tofumatt

Ran the test steps locally and it worked a treat. The code makes sense and the tests passed, so :shipit:

>
<SettingsHeader sidebarName={ SIDEBAR_NAME } />
<Panel>
<PanelBody className="edit-post-block-sidebar__panel">

This comment has been minimized.

@aduth

aduth Oct 25, 2018

Member

There's still an occurrence of this now-removed class name in tests:

await page.focus( '.edit-post-block-sidebar__panel .components-range-control__number[aria-label="Columns"]' );

Tests are currently running, but if they're passing that would be equally concerning.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 25, 2018

Update: I ran the tests on master by mistake but I verified the fix manually on the branch, sorry. Running them on the right branch now 😓

@tofumatt

Locally I'm getting a failure on:

>   ● splitting and merging blocks › should remove at most one paragraph in forward direction

Sorry, I thought I clicked "submit review" hours ago but I guess not, I'm just the worst on this PR 😓

From the looks the snapshot is wrong so something is off here and needs fixing.

@@ -32,7 +32,7 @@ describe( 'Navigating the block hierarchy', () => {
await columnsBlockMenuItem.click();
// Tweak the columns count.
await page.focus( '.edit-post-block-sidebar__panel .components-range-control__number[aria-label="Columns"]' );
await page.focus( '.edit-post-settings-sidebar__panel-block .components-range-control__number[aria-label="Columns"]' );

This comment has been minimized.

@gziolo

gziolo Oct 26, 2018

Member

Good catch, thanks 👍

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 26, 2018

● splitting and merging blocks › should remove at most one paragraph in forward direction

@tofumatt - it works properly for me locally and on Travis

screen shot 2018-10-26 at 11 41 16

@gziolo gziolo requested a review from tofumatt Oct 26, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 26, 2018

Travis is finally happy! 🎉

@aduth

This comment has been minimized.

Member

aduth commented Oct 26, 2018

In spirit I give my approval, though despite @gziolo doing the bulk of the work, I can't review a pull request I'd opened 😄

@youknowriad

despite @aduth doing the bulk of review work, he can't review the pull request he opened 😄so I'm approving :P

Test failure resolved #10917 (comment)

@aduth aduth merged commit e8de101 into master Oct 26, 2018

2 checks passed

codecov/project 48.42% (-0.48%) compared to ac81cf7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the fix/focus-loss-sidebar-tab branch Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment