Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(Popup): Focus the last focused element outside Popup on ESC #861

Merged
merged 6 commits into from
Feb 8, 2019

Conversation

sophieH29
Copy link
Contributor

@sophieH29 sophieH29 commented Feb 7, 2019

This PR fixes the issue when a complex component is used as a trigger for Popup, but actually, the inner element of that component triggers the Popup. So when press ESC on Popup, the focus should go to the actual focusable element which triggered the Popup, not the whole component itself.

In the next example, MenuItem is a trigger for the Popup and this component has this structure

<li> - wrapper
   <a></a> // focusable element which actually triggers Popup
</li>
const items = [
  {
    key: 'editorials',
    content: 'Editorials',
  },
  {
    key: 'review',
    content: 'Reviews',
  },
  {
    key: 'events',
    content: 'Upcoming Events',
  },
].map(item => render => render(item, renderIten))

const renderIten = (MenuItem, props) =>
  <Popup
    content={{ content: <Button content="Click me" /> }}
    trigger={<MenuItem {...props}/>}
  />

const MenuExample = () => <Menu defaultActiveIndex={0} items={items} />

So when press ESC, MenuItem's wrapper <li> element will receive focus, instead of <a>, what breaks focus management on the page.

Closes #831

@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

Merging #861 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #861   +/-   ##
=======================================
  Coverage   93.54%   93.54%           
=======================================
  Files          21       21           
  Lines         728      728           
  Branches       69       69           
=======================================
  Hits          681      681           
  Misses         47       47

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4ca3af...f17ad95. Read the comment docs.

* Save DOM element which had focus before Popup opens.
* Can be either trigger DOM element itself or the element inside it.
*/
private updateTriggerFocusableDomElement() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to introduce a new method for a single use?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense, don't want to mess up tryOpen method and still remain better reading

Co-Authored-By: sophieH29 <8460706+sophieH29@users.noreply.github.com>
@sophieH29 sophieH29 merged commit fc0d558 into master Feb 8, 2019
@sophieH29 sophieH29 deleted the fix/popup-triggered-by-menuitem-focuses-wrapper branch February 8, 2019 10:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popup triggered by MenuItem is focusing the wrapper instead of the root when user presses ESC
3 participants