Fix a regression that caused us to listen to extra events at the top #12878
Conversation
I find it a bit easier to follow than many comparison conditions.
They are being assigned below anyway. This is likely a copypasta from the FOCUS/BLUR special case (which *does* need those assignments).
Their logic is identical.
|
ReactDOM: size: 0.0%, gzip: -0.0% Details of bundled changes.Comparing: ad27845...53d0d36 react-dom
Generated by 🚫 dangerJS |
|
Hmm. I started looking into special cases and it looks like |
The case I added was wrong (just like including this event in the top level list was always wrong). In fact it never bubbles, even for <img>. And since we don't special case it in the <img> event attachment logic when we create it, we never supported <img onLoadStart> at all. We could fix it. But Chrome doesn't support it either: https://bugs.chromium.org/p/chromium/issues/detail?id=458851. Nobody asked us for it yet. And supporting it would require attaching an extra listener to every <img>. So maybe we don't need it? Let's document the existing state of things.
| formRef.current.dispatchEvent( | ||
| new Event('submit', { | ||
| // https://developer.mozilla.org/en-US/docs/Web/Events/submit | ||
| bubbles: true, |
There was a problem hiding this comment.
Might be nice to have a bubbles-false test for this too if we don't.
There was a problem hiding this comment.
When would it be false?
There was a problem hiding this comment.
Old browsers I think. That's why we listen on the element at all.
| bubbles: true, | ||
| }), | ||
| ); | ||
| expect(handleSubmit).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
This failed before, right?
There was a problem hiding this comment.
Yes, it failed before your fix
| // Some of them bubble so we don't want them to fire twice. | ||
| break; | ||
| default: | ||
| // By default, listen on the top level to all non-media events. |
There was a problem hiding this comment.
Maybe add:
// Media events don't bubble so adding the listener wouldn't do anything.
| break; | ||
| default: | ||
| // By default, listen on the top level to all non-media events. | ||
| const isNonMediaEvent = mediaEventTypes.indexOf(dependency) === -1; |
There was a problem hiding this comment.
IMO this would be easier to read as
isMediaEvent
!== -1
if (!isMediaEvent)
There was a problem hiding this comment.
Force-pushed the version without the commit that changed this
I think this should fix the regression introduced in #12629 and that @sophiebits has patched over in #12877.
The first three commits are small refactors (I can remove them but I found the final form easier to read with these changes). The last one fixes the logic.
I'll need to work on some way to test this and verify it’s correct. Here’s the methodology with which I arrived at this solution:
Take a list of all DOM event types in master
For each type, record whether it exists in either of two mappings that were deleted in #12629:
topLevelTypesormediaEventTypes. Some types were new in master (pointer events) which I treat as if they were intopLevelTypes.After doing this categorization, I ended up with four lists:
NOT top AND NOT media
TOP_INVALIDTOP_RESETTOP_SUBMITNOT top AND media
TOP_ABORTTOP_CAN_PLAYTOP_CAN_PLAY_THROUGHTOP_DURATION_CHANGETOP_EMPTIEDTOP_ENCRYPTEDTOP_ENDEDTOP_ERRORTOP_LOADED_DATATOP_LOADED_METADATATOP_PAUSETOP_PLAYTOP_PLAYINGTOP_PROGRESSTOP_RATE_CHANGETOP_SEEKEDTOP_SEEKINGTOP_STALLEDTOP_SUSPENDTOP_TIME_UPDATETOP_VOLUME_CHANGETOP_WAITINGtop AND media
TOP_LOAD_STARTtop AND NOT media
TOP_ANIMATION_ENDTOP_ANIMATION_ITERATIONTOP_ANIMATION_STARTTOP_BLURTOP_CANCELTOP_CHANGETOP_CLICKTOP_CLOSETOP_COMPOSITION_ENDTOP_COMPOSITION_STARTTOP_COMPOSITION_UPDATETOP_CONTEXT_MENUTOP_COPYTOP_CUTTOP_DOUBLE_CLICKTOP_DRAGTOP_DRAG_ENDTOP_DRAG_ENTERTOP_DRAG_EXITTOP_DRAG_LEAVETOP_DRAG_OVERTOP_DRAG_STARTTOP_DROPTOP_FOCUSTOP_GOT_POINTER_CAPTURETOP_INPUTTOP_KEY_DOWNTOP_KEY_PRESSTOP_KEY_UPTOP_LOADTOP_LOST_POINTER_CAPTURETOP_MOUSE_DOWNTOP_MOUSE_MOVETOP_MOUSE_OUTTOP_MOUSE_OVERTOP_MOUSE_UPTOP_PASTETOP_POINTER_CANCELTOP_POINTER_DOWNTOP_POINTER_ENTERTOP_POINTER_LEAVETOP_POINTER_MOVETOP_POINTER_OUTTOP_POINTER_OVERTOP_POINTER_UPTOP_SCROLLTOP_SELECTION_CHANGETOP_TEXT_INPUTTOP_TOGGLETOP_TOUCH_CANCELTOP_TOUCH_ENDTOP_TOUCH_MOVETOP_TOUCH_STARTTOP_TRANSITION_ENDTOP_WHEELFrom these lists, it follows that:
TOP_LOAD_STARTis an exception to the previous rule: even though it’s a media event, we listen to it on the top level.TOP_INVALID,TOP_RESET, andTOP_SUBMIT) that are neither in the media nor the top whitelist. So we should exclude them too.That’s reflected in my switch structure:
Note that I’m introducing an
indexOflookup there. I think it’s fine because this code only runs once per event, andindexOfis very fast on small arrays (mediaEventTypeshas 23 elements). So I wouldn’t expect this to regress anything. But I can turn it into an object lookup if needed.