Make it possible to locally prevent event propagation #9749
Conversation
|
Would be nice to get some 👀 on this I presume @roryabraham. Should we look for an internal or external engineer to help? |
This was just a POC stemming from this slack thread. I never really intended to have this merged or take on the issue myself. Was just trying to help |
| window.removeEventListener('keydown', bindHandlerToKeydownEvent); | ||
| window.addEventListener('keydown', bindHandlerToKeydownEvent); |
There was a problem hiding this comment.
I believe this code is not required. Could you explain why did you add this?
There was a problem hiding this comment.
I don't recall right now, but found that it was necessary in my testing, which was pretty thorough for the login page.
There was a problem hiding this comment.
I think the idea is that we establish these event handlers in the bubbling phase so that further down in the DOM we can prevent them from being triggered in the capture phase.
If you have a capture-phase handler at the document root, then there's no way to prevent it from executing first
| onKeyDownCapture={(e) => { | ||
| e.stopPropagation(); | ||
| Session.clearSignInData(); | ||
| }} |
There was a problem hiding this comment.
I am not very confident about it as there is only single button on this page . What if we have multiples. We have to explicitly add this block of code to every such component?
eg:
There was a problem hiding this comment.
I guess so but I don't have ideas on this. Rory will have better ideas on this.
There was a problem hiding this comment.
yeah, I think you would have to add this code to every such component OR create a wrapper component like FormPressable that has it built-in and use it for all pressables within forms
There was a problem hiding this comment.
Can we do the reverse? Create a form wrapper component and add onKeyDownCapture and check if the target is INPUT, TEXTAREA, SELECT and trigger the submit function. I believe this will fix the issue without any sideeffects.
There was a problem hiding this comment.
There was a problem hiding this comment.
@roryabraham Are these changes you are expecting here?
There was a problem hiding this comment.
Yeah that POC looks good to me 👍
Details
React 17 changed where events handlers are set up from
documentto the root node (see changes to EventDelegation).Before, we solved this by making global event handlers attached to the
documentuse the capturing phase, so they always execute first. However, this makes it impossible for a local event handler to execute before the global one and prevent propagation to the parent handler, because thedocumentin capturing phase will always be the first to receive any event. This PR changes that, so that we just set up event listeners in the bubbling phase (as is a more common practice). We now attach global event handlers to the same place React will (i.e:rootNode) instead of to the parentdocument.Fixed Issues
$ (related to) #7918
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
Mobile Web
Desktop
iOS
Android