[Popover ] Fixed incorrect element being focused when closed #2255

AndrewMusgrave · 2019-10-07T21:30:13Z

WHY are these changes introduced?

WHAT is this pull request doing?

When the popover is closed it'll focus the next focusable element. If a next focusable element is not present the activator will be focused
Create two focus utilities
Add tests for utilities
Add missing focus tests

How to 🎩

With another focusable element

import React, {useState, useCallback} from 'react';
import {Popover, Button, ActionList} from '../src';

export function Playground() {
  const [popoverActive, setPopoverActive] = useState(true);

  const togglePopoverActive = useCallback(
    () => setPopoverActive((popoverActive) => !popoverActive),
    [],
  );

  const activator = (
    <Button onClick={togglePopoverActive} disclosure>
      More actions
    </Button>
  );

  return (
    <div style={{height: '250px'}}>
      <Popover
        active={popoverActive}
        activator={activator}
        onClose={togglePopoverActive}
      >
        <ActionList items={[{content: 'Import'}, {content: 'Export'}]} />
      </Popover>
      <button>next focusable target</button>
    </div>
  );
}

Without another focusable element

import React, {useState, useCallback} from 'react';
import {Popover, Button, ActionList} from '../src';

export function Playground() {
  const [popoverActive, setPopoverActive] = useState(true);

  const togglePopoverActive = useCallback(
    () => setPopoverActive((popoverActive) => !popoverActive),
    [],
  );

  const activator = (
    <Button onClick={togglePopoverActive} disclosure>
      More actions
    </Button>
  );

  return (
    <div style={{height: '250px'}}>
      <Popover
        active={popoverActive}
        activator={activator}
        onClose={togglePopoverActive}
      >
        <ActionList items={[{content: 'Import'}, {content: 'Export'}]} />
      </Popover>
    </div>
  );
}

TODO

Add changelog

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

github-actions · 2019-10-07T21:32:56Z

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified	10
Files potentially affected	95

Details

All files potentially affected (total: 95)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Popover/Popover.tsx (total: 23)

Files potentially affected (total: 23)

🧩 src/components/Popover/tests/Popover.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Portal/Portal.tsx (total: 33)

Files potentially affected (total: 33)

🧩 src/components/Portal/tests/Portal.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/shared.ts (total: 93)

Files potentially affected (total: 93)

🧩 src/utilities/focus.ts (total: 65)

Files potentially affected (total: 65)

🧩 src/utilities/is-element-in-viewport.ts (total: 66)

Files potentially affected (total: 66)

🧩 src/utilities/tests/focus.test.ts (total: 0)

Files potentially affected (total: 0)

🧩 src/utilities/tests/is-element-in-viewport.test.ts (total: 0)

Files potentially affected (total: 0)

This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

AndrewMusgrave · 2019-10-07T22:35:04Z

src/utilities/focus.ts

+  let nextElement = node.nextElementSibling;
+
+  while (nextElement) {
+    if (nextElement.matches(FOCUSABLE_SELECTOR)) return nextElement;


I'm going with the matches approach that rather a tabIndex so we can ignore disabled elements

AndrewMusgrave · 2019-10-07T22:36:00Z

src/utilities/focus.ts

+  while (nextElement) {
+    if (nextElement.matches(FOCUSABLE_SELECTOR)) return nextElement;
+
+    if (nextElement instanceof HTMLElement) {


findFirstFocusableNode require HTMLElement rather than Element. Although nextElementSibling won't return text nodes so I'm not sure we'll ever get an Element

AndrewMusgrave · 2019-10-07T22:36:43Z

src/utilities/focus.ts

+
+export function focusNextFocusableNode(node: HTMLElement) {
+  const nextFocusable = nextFocusableNode(node);
+  if (nextFocusable && nextFocusable instanceof HTMLElement) {


The Element interface doesn't contain focus so we need this check

AndrewMusgrave · 2019-10-07T22:37:15Z

src/utilities/focus.ts

+  const nextFocusable = nextFocusableNode(node);
+  if (nextFocusable && nextFocusable instanceof HTMLElement) {
+    nextFocusable.focus();
+    return true;


Added a boolean return rather than void so we can determine if an element was focused or not

dleroux · 2019-10-17T12:21:50Z

src/utilities/focus.ts

+export function nextFocusableNode(
+  node: HTMLElement,
+): HTMLElement | Element | null {
+  let nextElement = node.nextElementSibling;


This logic will only work if the nextFocusableElement same tree level or lower correct? The following would not pass

<React.Fragment> <div><Popover active activator={<div />} onClose={noop} /></div> <button id={id} /> </React.Fragment>

I think it's fine since the activator will get focus in the end. I can't think of an inexpensive way to do this either. What do you think?

dpersing

I'm not sure if this is a new issue or not (since our previous examples didn't have additional elements to manage focus around, but I found while testing this that focus is does not appear to be moved to the popover on iOS when the button is activated. When using VoiceOver with Safari or Chrome on iOS, I had to swipe to the next focusable target button before I could swipe to the popover contents. Can we confirm if this was an existing issue, or if it's introduced by this update? If it's not new, I'll open a new issue, which we should address as soon as possible, since it impacts popover usage across mobile.

I wonder if we can also take a look at web to see if anyone is overriding our current focus management logic for popovers, to make sure this update won't cause conflicts?

AndrewMusgrave · 2019-10-22T19:26:30Z

I'm not sure if this is a new issue or not (since our previous examples didn't have additional elements to manage focus around, but I found while testing this that focus is does not appear to be moved to the popover on iOS when the button is activated. When using VoiceOver with Safari or Chrome on iOS, I had to swipe to the next focusable target button before I could swipe to the popover contents. Can we confirm if this was an existing issue, or if it's introduced by this update? If it's not new, I'll open a new issue, which we should address as soon as possible, since it impacts popover usage across mobile.

@dleroux and I spent some time on iOS voiceover and it appears it's an existing issue. Oddly enough we still have the correct focus order, however it seems voice was following the dom order, and not respecting the currently focused element. Is this many a voiceover bug?

I wonder if we can also take a look at web to see if anyone is overriding our current focus management logic for popovers, to make sure this update won't cause conflicts?

It would be pretty difficult to check all the popovers since there're hundreds, maybe thousands 😬 however I tested home, and a few other pages as well and everything looked 👍

dpersing · 2019-10-31T16:23:23Z

Created issue #2387 to capture the issue with VoiceOver on iOS.

dleroux · 2019-11-04T13:07:54Z

Is this ready to review again @AndrewMusgrave?

dpersing · 2019-11-04T15:53:27Z

@dleroux Per discussion in Slack last week, yes!

dleroux

🎩 and works great.

Interestingly enough as I was tabbing through the popover I would have expected it to trap focus but I'm guessing that is not the case? @dpersing

dleroux · 2019-11-05T13:42:38Z

src/utilities/focus.ts

+  node: HTMLElement,
+  filter?: Filter,
+): HTMLElement | Element | null {
+  const allFocusableElements = [


dleroux · 2019-11-05T13:59:21Z

src/utilities/tests/focus.test.ts

+  });
+});
+
+function domSetup(


This is great, not asking to change it, but I wonder if it wouldn't have been simpler outline the dom being tested with each test. It makes it a little tricky to review and it's almost like domSetup() needs its own tests.

Addressed

AndrewMusgrave force-pushed the popover-focus-next branch 2 times, most recently from 1717d92 to 6d89db1 Compare October 7, 2019 22:15

AndrewMusgrave commented Oct 7, 2019

View reviewed changes

AndrewMusgrave force-pushed the popover-focus-next branch from 6d89db1 to e89834c Compare October 7, 2019 22:40

AndrewMusgrave requested review from mirualves, dpersing and dleroux October 7, 2019 22:45

AndrewMusgrave marked this pull request as ready for review October 7, 2019 22:45

dleroux reviewed Oct 17, 2019

View reviewed changes

dleroux self-requested a review October 17, 2019 12:39

dpersing previously requested changes Oct 17, 2019

View reviewed changes

AndrewMusgrave force-pushed the popover-focus-next branch 3 times, most recently from 0ba6a36 to 4b255bb Compare October 22, 2019 18:53

AndrewMusgrave requested a review from dpersing October 22, 2019 19:27

dpersing mentioned this pull request Oct 31, 2019

VoiceOver focus management not working on iOS with popover #2387

Closed

dleroux mentioned this pull request Nov 5, 2019

[Popover] Convert Popover to functional component #2386

Merged

6 tasks

dleroux approved these changes Nov 5, 2019

View reviewed changes

AndrewMusgrave added 2 commits November 7, 2019 14:23

Fixed Popover focusing incorrect element when closed

e1d2c2d

changelog

9894f68

AndrewMusgrave force-pushed the popover-focus-next branch from 4b255bb to 9894f68 Compare November 7, 2019 19:24

AndrewMusgrave merged commit b1127fa into master Nov 7, 2019

AndrewMusgrave deleted the popover-focus-next branch November 7, 2019 19:31

PabloVallejo temporarily deployed to production November 12, 2019 14:48 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Popover ] Fixed incorrect element being focused when closed #2255

[Popover ] Fixed incorrect element being focused when closed #2255

AndrewMusgrave commented Oct 7, 2019 •

edited

Loading

github-actions bot commented Oct 7, 2019 •

edited

Loading

Files potentially affected (total: 0)

Files potentially affected (total: 23)

Files potentially affected (total: 0)

Files potentially affected (total: 33)

Files potentially affected (total: 0)

Files potentially affected (total: 93)

Files potentially affected (total: 65)

Files potentially affected (total: 66)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

AndrewMusgrave Oct 7, 2019

AndrewMusgrave Oct 7, 2019

AndrewMusgrave Oct 7, 2019

AndrewMusgrave Oct 7, 2019

dleroux Oct 17, 2019

dpersing left a comment

AndrewMusgrave commented Oct 22, 2019

dpersing commented Oct 31, 2019

dleroux commented Nov 4, 2019

dpersing commented Nov 4, 2019

dleroux left a comment

dleroux Nov 5, 2019

dleroux Nov 5, 2019

[Popover ] Fixed incorrect element being focused when closed #2255

[Popover ] Fixed incorrect element being focused when closed #2255

Conversation

AndrewMusgrave commented Oct 7, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

TODO

🎩 checklist

github-actions bot commented Oct 7, 2019 • edited Loading

Details

Files potentially affected (total: 0)

Files potentially affected (total: 23)

Files potentially affected (total: 0)

Files potentially affected (total: 33)

Files potentially affected (total: 0)

Files potentially affected (total: 93)

Files potentially affected (total: 65)

Files potentially affected (total: 66)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

AndrewMusgrave Oct 7, 2019

Choose a reason for hiding this comment

AndrewMusgrave Oct 7, 2019

Choose a reason for hiding this comment

AndrewMusgrave Oct 7, 2019

Choose a reason for hiding this comment

AndrewMusgrave Oct 7, 2019

Choose a reason for hiding this comment

dleroux Oct 17, 2019

Choose a reason for hiding this comment

dpersing left a comment

Choose a reason for hiding this comment

AndrewMusgrave commented Oct 22, 2019

dpersing commented Oct 31, 2019

dleroux commented Nov 4, 2019

dpersing commented Nov 4, 2019

dleroux left a comment

Choose a reason for hiding this comment

dleroux Nov 5, 2019

Choose a reason for hiding this comment

dleroux Nov 5, 2019

Choose a reason for hiding this comment

AndrewMusgrave commented Oct 7, 2019 •

edited

Loading

github-actions bot commented Oct 7, 2019 •

edited

Loading