Skip to content
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

fix(overflow-menu): check if function exists before calling #6707

Merged
merged 1 commit into from
Aug 24, 2020

Conversation

tw15egan
Copy link
Collaborator

Closes #6705

Check if target.matches exists before invoking it

Changelog

Changed

  • Check if target.matches exists before invoking it

Testing / Reviewing

Using Firefox, open OverflowMenu, and open the menu. Then click anywhere outside the browser, return to the browser, and click anywhere inside to close the menu. There should not be an error.

@dabrad26
Copy link
Member

I see similar target.matches in

  • pagination-nav
  • init-component-by-search
  • init-component-by-event
  • init-component-by-launcher
  • ui-shell/product-switcher
  • dropdown
  • data-table

Maybe should have this check for all the cases. IDK what init-component is for, but I assume the others are similar to what overflow was doing.

Copy link
Member

@dabrad26 dabrad26 left a comment

Choose a reason for hiding this comment

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

IDK of a reason matches would be set to anything but function. But normally for libraries I go for overly cautious :/

packages/react/src/components/OverflowMenu/OverflowMenu.js Outdated Show resolved Hide resolved
@tw15egan tw15egan force-pushed the overflow-firefox-fix branch from 172a4fe to cf46d8b Compare August 21, 2020 17:09
@tw15egan
Copy link
Collaborator Author

I agree, made it match function specifically. Not seeing any issues with those other components, I think the main reason this one was throwing was due to the positioning logic of the FloatingMenu.

Thanks again for catching that and taking a look 👍

@netlify
Copy link

netlify bot commented Aug 21, 2020

Deploy preview for carbon-elements ready!

Built with commit 172a4fe

https://deploy-preview-6707--carbon-elements.netlify.app

@dabrad26
Copy link
Member

Thanks! I am a bit concerned with an event getting triggered in Firefox and then bubbling it in on viewport focus (with no target); that def seems like wrong behavior.

I hope its a bug in Firefox and they fix it. But if not I fear it could show as more issue later for other events that rely on target (even if not error can cause the expected behaviors to get impacted).

One concern I saw was it seemed when clicking in Firefox dev tools around the events backed up and would then hit the app over and over once viewport got focus (causing freezing). Seems very odd... Will keep an eye on Firefox's bug reports and open if I don't see others talking about it.

@netlify
Copy link

netlify bot commented Aug 21, 2020

Deploy preview for carbon-components-react ready!

Built with commit 172a4fe

https://deploy-preview-6707--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Aug 21, 2020

Deploy preview for carbon-elements ready!

Built with commit cf46d8b

https://deploy-preview-6707--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Aug 21, 2020

Deploy preview for carbon-components-react ready!

Built with commit cf46d8b

https://deploy-preview-6707--carbon-components-react.netlify.app

@kodiakhq kodiakhq bot merged commit 019b5e7 into carbon-design-system:master Aug 24, 2020
@tw15egan tw15egan deleted the overflow-firefox-fix branch April 28, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overflow using renderIcon throws error in Firefox on click outside
4 participants