Skip to content

fix(drawing): Fix cannot focus drawing annotations on IE11#634

Merged
mergify[bot] merged 2 commits intobox:masterfrom
mingzexiao6:fix-drawing-focus
Nov 13, 2020
Merged

fix(drawing): Fix cannot focus drawing annotations on IE11#634
mergify[bot] merged 2 commits intobox:masterfrom
mingzexiao6:fix-drawing-focus

Conversation

@mingzexiao6
Copy link
Contributor

No description provided.

@mingzexiao6 mingzexiao6 requested a review from a team as a code owner November 13, 2020 04:56
event.currentTarget.focus();
} else {
// IE11 does not support focus function; call handleFocus directly
handleFocus();
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, does this reduce the number of SET_ACTIVE_ANNOTATION_ID actions triggered?

Should this be done for HighlightTarget as well?

Copy link
Contributor Author

@mingzexiao6 mingzexiao6 Nov 13, 2020

Choose a reason for hiding this comment

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

It does reduce the number of actions by 1 compared to HighlightTargt. But the purpose of this PR is to fix that onSelect will not be called if event.currentTarget.focus doesn't exist in IE and it errors out. It also removes the console error.

Copy link
Contributor

Choose a reason for hiding this comment

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

You dared to open ie11 dev tools 😁

event.currentTarget.focus(); // Buttons do not receive focus in Firefox and Safari on MacOS; triggers handleFocus

onSelect(annotationId);
if (event.currentTarget.focus) {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: could be more compact with something like:

const focusFn = event.currentTarget.focus || handleFocus;
focusFn();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about (event.currentTarget.focus || handleFocus)()? Haha actually I think the current way is easy to read and easy to add comments.

@mergify mergify bot merged commit 30a61ae into box:master Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants