Drop some top-level events from the list #11912
Conversation
| topEmptied: 'emptied', | ||
| topEncrypted: 'encrypted', | ||
| topEnded: 'ended', | ||
| topError: 'error', |
There was a problem hiding this comment.
This one also fires for JS errors and images (not just media) but AFAIK it doesn't truly bubble either way.
There was a problem hiding this comment.
Correct. It does not fire bubble for images, iframes, videos, or audio tags.
But it does capture. We might be able to attach them at the top level using capture to avoid eagerly attaching a listener to each element.
|
Tagged you folks for review but it's nothing urgent :-) Have good holidays. |
"react-dom.production.min.js (UMD_PROD)": {
- "size": 93699,
- "gzip": 30755
+ "size": 93218,
+ "gzip": 30555
}, |
| trapBubbledEvent(event, mediaEvents[event], domElement); | ||
| for (const event in mediaEventTypes) { | ||
| if (mediaEventTypes.hasOwnProperty(event)) { | ||
| trapBubbledEvent(event, mediaEventTypes[event], domElement); |
There was a problem hiding this comment.
trapBubbledEvent is kind of a confusing name now since it's attaching the listener directly to the event target in some cases.
There was a problem hiding this comment.
Agreed. It's more like "attach this event locally to emulate bubbling"
gaearon
force-pushed
the
drop-top-events
branch
from
86776d4 to
671ecae
Compare
gaearon
force-pushed
the
drop-top-events
branch
from
671ecae to
2dbf22e
Compare
|
Anybody wants to help test this in browsers? 😄 |
|
Sorry, should have said something. I was going to create a fixture for this. But I can get on some browser testing stuff too. |
|
Nice, thanks! I'll leave this to you then. |
gaearon
requested review from
nhunzaker
and removed request for
nhunzaker and
jquense
|
Just checking back. We merged the media event fixtures. Next step is to rebase those into this branch and run the tests real quick. I was planning on doing that, but no worries if someone else can get to it before me. |
|
Rebased master and verified this in the DOM fixtures. The dog approves:
|
|
Thank you! |
| @@ -18,7 +18,7 @@ import { | |||
| import SyntheticEvent from 'events/SyntheticEvent'; | |||
| import invariant from 'fbjs/lib/invariant'; | |||
|
|
|||
| import BrowserEventConstants from '../events/BrowserEventConstants'; | |||
| import {topLevelTypes} from '../events/BrowserEventConstants'; | |||
There was a problem hiding this comment.
Oops I think this might not be enough now? We need to do the same for mediaEventTypes or we'll "lose" some events?
There was a problem hiding this comment.
Ah man. I think you're right. Darn.
* Drop some top-level events from the list * Put both whitelists in one file
Since they never bubble it doesn't seem necessary to keep them in the top-level list?
We already don't put other events like
submitthere for the same reason.I haven't verified this works in practice, but I'd expect it to.
Or at the very least we should clarify the comment that explains the criteria for inclusion.