Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Safari Linking: Check for selection range count before calling getRangeAt #15576

Merged
merged 1 commit into from May 11, 2019

Conversation

@gwwar
Copy link
Contributor

commented May 10, 2019

Calling window.getSelection.getRangeAt(0) is unsafe without first checking for selection.rangeCount > 0. In certain situations this can cause unexpected block errors like the gif below. This is most easily reproducible in Safari, but will be inconsistent.

To attempt to reproduce:

  1. Add a new link
  2. Focus on another paragraph
  3. Focus on the original paragraph, or on the link directly

See also related: Automattic/wp-calypso#32965 and #11209 and potentially #15491

Before:
May-10-2019 14-49-33

See https://stackoverflow.com/questions/22935320/uncaught-indexsizeerror-failed-to-execute-getrangeat-on-selection-0-is-not/23699875 for a short example of the window.getSelection.getRangeAt(0) issue.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gwwar gwwar self-assigned this May 10, 2019

@gwwar gwwar changed the title Check for selection range count before calling getRangeAt Fix Safari Linking: Check for selection range count before calling getRangeAt May 10, 2019

@vesaraiskila

This comment has been minimized.

Copy link

commented May 11, 2019

I was able to reproduce this bug using precisely the steps described with Safari on iMac.

@ellatrix ellatrix merged commit 9c5bc04 into master May 11, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@ellatrix ellatrix deleted the fix/safari-links branch May 11, 2019

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 13, 2019

@aduth

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Doing a codebase search for window.getSelection().getRangeAt( 0 ), I find another instance:

const range = window.getSelection().getRangeAt( 0 );

Would it be fair to say that if window.getSelection().getRangeAt( 0 ) is unsafe without prior consideration of selection.rangeCount, that this one too should be updated?

@gwwar

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Would it be fair to say that if window.getSelection().getRangeAt( 0 ) is unsafe without prior consideration of selection.rangeCount, that this one too should be updated?

Yes, any instances should be updated. I wonder if we can add a custom lint rule here?

@aduth

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Yes, any instances should be updated. I wonder if we can add a custom lint rule here?

The naive check might be simple enough:

https://astexplorer.net/#/gist/26984b64750ba301178a38680fbce17f/66c7af6eb40fa09446d81d33f4adf3697eb13b47

But it fails at:

window.getSelection().rangeCount > 0 ? window.getSelection().getRangeAt( 0 ) : null;

...unadvisable anyways, re: repeated window.getSelection() vs. variable assignment.

const selection = window.getSelection();
selection.getRangeAt( 0 );

...probably the less likely case if doing an unguarded check anyways, and a "better than nothing" allowable false negative.

@aduth

This comment has been minimized.

Copy link
Member

commented May 13, 2019

An issue for the remaining item / possible ESLint rule: #15606

@vesaraiskila

This comment has been minimized.

Copy link

commented May 17, 2019

Sorry, when will a fix be available? In the next WordPress release?

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@vesaraiskila yes, WP 5.2.1

@vesaraiskila

This comment has been minimized.

Copy link

commented May 17, 2019

Thanks @youknowriad Do you know when it will be released? Our site should upgrade to it automatically.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Sometime next week probably. There's no precise date yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.