Closed keithamus closed 11 months ago
Name | Link |
---|---|
Latest commit | 9eef4ea1a95cce9bc364ccaa0871f4bf7dbc0195 |
Latest deploy log | https://app.netlify.com/sites/popover-polyfill/deploys/657195cb7c04b90008f27e61 |
Deploy Preview | https://deploy-preview-160--popover-polyfill.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Description
This fixes https://github.com/oddbird/popover-polyfill/issues/139. Currently the demo page crashes in Firefox (without popovers enabled) if you click the "Click to toggle shadowed popover" button.
Looking through a crash log, we can see this is because Firefox tries to handle the popover using their popover logic, but the logic isn't guarded to check the feature flag, and it tries to get the element by its ID, which is empty. The empty id check fails for some reason, and we end up with a crash:
mTarget->postHandleEvent(aVisitor);
postHandleEvent
callsHandlePopoverTargetAction();
HandlePopoverTargetAction
isn't adequately guarded so callsGetEffectivePopoverTargetElement();
GetAttrAssociatedElement(nsGkAtoms::popovertarget);
docOrShadowRoot->GetElementById(valueAtom);
aElementId
is anAtom
, but it should be the empty atom, and it should hitMOZ_UNLIKELY(aElementId == nsGkAtoms::_empty)
and return anullptr
, but for some reason it doesn't (it's not the empty atom?)mIdentifierMap.GetEntry(aElementId)
So, this is definitely a bug in Firefox. Exceptions like this shouldn't happen, I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1868740 and I'll work on a patch to fix it.
In the meantime, however, we have an opportunity to fix it ourselves, because
postHandleEvent()
will only be called if the event's default wasn't prevented. Consequently, simply addingevent.preventDefault()
in our JS fixes this issue.Steps to test/reproduce
Show me
N/A