Skip to content

fix: Ensure document level mousedown listener is last#668

Merged
ConradJChan merged 6 commits intobox:masterfrom
ConradJChan:relocate-mousedown-logic
Jan 15, 2021
Merged

fix: Ensure document level mousedown listener is last#668
ConradJChan merged 6 commits intobox:masterfrom
ConradJChan:relocate-mousedown-logic

Conversation

@ConradJChan
Copy link
Contributor

@ConradJChan ConradJChan commented Jan 13, 2021

Issue

With the logic added to BaseAnnotator to setActiveId to null when mousedown event is encountered this introduced a regression where if you click on the same annotation a second time, the annotation would no longer become active

Solution

Due to the way React handles events, an event listener added to document before the React code mounts will always be invoked first. The approach here is to attach the event listener to document after the React code mounts, or more accurately, after the React code that matters mounts, i.e. *List components that have mousedown logic, in order to take advantage of being able to prevent event propagation when appropriate.

TODO:

  • unit tests
  • cross browser testing

@ConradJChan ConradJChan force-pushed the relocate-mousedown-logic branch from 6f2cff7 to 2a19ae8 Compare January 14, 2021 00:48
@ConradJChan ConradJChan marked this pull request as ready for review January 14, 2021 17:20
@ConradJChan ConradJChan requested a review from a team as a code owner January 14, 2021 17:20
store: Store;
};

export default class MouseeventManager extends BaseManager {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a whole manager for this or can we follow the HighlightListener convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to go with the HighlightListener convention first, but it kind of needs the BaseManager logic of inserting a div to mount the react component so I ended up duplicating most of the BaseManager

@ConradJChan ConradJChan force-pushed the relocate-mousedown-logic branch from 2a19ae8 to d6f2819 Compare January 15, 2021 00:23
@ConradJChan ConradJChan force-pushed the relocate-mousedown-logic branch from cc56bd4 to a2fb242 Compare January 15, 2021 21:25
mingzexiao6
mingzexiao6 previously approved these changes Jan 15, 2021
@ConradJChan ConradJChan merged commit a6648d1 into box:master Jan 15, 2021
@ConradJChan ConradJChan deleted the relocate-mousedown-logic branch January 15, 2021 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants