Skip to content
This repository has been archived by the owner on Feb 6, 2023. It is now read-only.

Added tests and fixed IE IndexSizeError trying to get a range from a selection when there is not one #2271

Closed

Conversation

LaurenWyatt610
Copy link
Contributor

Summary
Various browsers will throw errors if selection.getRangeAt(0) is called when there is no range. To fix this, a check was added to make sure a range exists.

This change was originally suggested here a few years ago. I opted for opening a new PR and adding tests here since that PR seemed stale.

Selection and Range did not work in the test environment, so I had to stub them. I only added functionality for what was needed in the tests I added, but more can be added in the future very easily.

@mrkev
Copy link
Contributor

mrkev commented Dec 4, 2019

This is awesome! Thanks for adding the tests too. I'll try to get this merged.

Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@mrkev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@gdtrice
Copy link

gdtrice commented Dec 4, 2019

Nice!

@LaurenWyatt610
Copy link
Contributor Author

@mrkev Is there anything else I need to do (resolving conflicts and making changes based on whatever the internal test failures/warnings are)? Or is that handed by Facebook now since this PR has been "imported"

@mrkev
Copy link
Contributor

mrkev commented Dec 16, 2019

Hey @lrnwytt, holiday season has lots of things fighting for my attention, thanks for bringing this up again, I want to get this in 👍

Facebook web exists as a monorepo. Had a quick look at the test failure— it seems that the way this repo is set up to sync with our www monorepo means that adding files on Github doesn't add them to our dependency resolution system, so the tests can't actually find RangeApi and SelectionApi when we run them internally.

Let's just embed the RangeApi and SelectionApi classes in the file that uses them. It will make it a little more verbose but it will save a lot of hoop-jumping, and honestly if they're just used here I think it's the most worth approach.

The lint failure is actually a warning because the test is not @flow strict-local and the requires are not sorted alphabetically— this won't block landing and I'm fine landing it as is.

Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@mrkev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@mrkev
Copy link
Contributor

mrkev commented Dec 17, 2019

Sweet thanks!

@LaurenWyatt610
Copy link
Contributor Author

Thanks for working through this with me! We just got this logged as a bug and it's so great to see how easy the outsider PR process is.
Have a great holiday!

@facebook-github-bot
Copy link

@mrkev merged this pull request in aa55de2.

@mrkev
Copy link
Contributor

mrkev commented Dec 20, 2019

@lrnwytt thanks, this was a very well-done PR which made it easy. Hope you have a great holiday too! (:

mmissey pushed a commit to mmissey/draft-js that referenced this pull request Mar 24, 2020
…selection when there is not one (facebookarchive#2271)

Summary:
**Summary**
Various browsers will throw errors if `selection.getRangeAt(0)` is called when there is no range. To fix this, a check was added to make sure a range exists.

This change was originally suggested [here](facebookarchive#1571) a few years ago. I opted for opening a new PR and adding tests here since that PR seemed stale.

Selection and Range did not work in the test environment, so I had to stub them. I only added functionality for what was needed in the tests I added, but more can be added in the future very easily.
Pull Request resolved: facebookarchive#2271

Differential Revision: D18807105

Pulled By: mrkev

fbshipit-source-id: 0e3b833b8a3267b9a5f17b262b6a0442b6ae5e3d
vilemj-Viclick pushed a commit to kontent-ai/draft-js that referenced this pull request Jul 16, 2020
…selection when there is not one (facebookarchive#2271)

Summary:
**Summary**
Various browsers will throw errors if `selection.getRangeAt(0)` is called when there is no range. To fix this, a check was added to make sure a range exists.

This change was originally suggested [here](facebookarchive#1571) a few years ago. I opted for opening a new PR and adding tests here since that PR seemed stale.

Selection and Range did not work in the test environment, so I had to stub them. I only added functionality for what was needed in the tests I added, but more can be added in the future very easily.
Pull Request resolved: facebookarchive#2271

Differential Revision: D18807105

Pulled By: mrkev

fbshipit-source-id: 0e3b833b8a3267b9a5f17b262b6a0442b6ae5e3d
alicayan008 pushed a commit to alicayan008/draft-js that referenced this pull request Jul 4, 2023
…selection when there is not one (#2271)

Summary:
**Summary**
Various browsers will throw errors if `selection.getRangeAt(0)` is called when there is no range. To fix this, a check was added to make sure a range exists.

This change was originally suggested [here](facebookarchive/draft-js#1571) a few years ago. I opted for opening a new PR and adding tests here since that PR seemed stale.

Selection and Range did not work in the test environment, so I had to stub them. I only added functionality for what was needed in the tests I added, but more can be added in the future very easily.
Pull Request resolved: facebookarchive/draft-js#2271

Differential Revision: D18807105

Pulled By: mrkev

fbshipit-source-id: 0e3b833b8a3267b9a5f17b262b6a0442b6ae5e3d
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants