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

Throw correct exceptions from collapse/selectAllChildren on detached doctypes #86

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from

Conversation

ayg
Copy link
Contributor

@ayg ayg commented Apr 25, 2017

According to the previous spec, trying to collapse to or select a
doctype that is not in the current document would not throw. But
Chrome, Firefox, and Edge all do throw. (WebKit doesn't seem to throw
for doctypes at all.) The wpt test also tests that browsers do throw in
this case.

…doctypes

According to the previous spec, trying to collapse to or select a
doctype that is not in the current document would not throw.  But
Chrome, Firefox, and Edge all do throw.  (WebKit doesn't seem to throw
for doctypes at all.)  The wpt test also tests that browsers do throw in
this case.
@ayg ayg force-pushed the selectAllChildren-exceptions branch from 2a053cc to d9b03f9 Compare April 25, 2017 14:38
@ayg ayg changed the title Throw correct exceptions from selectAllChildren on detached doctypes Throw correct exceptions from collapse/selectAllChildren on detached doctypes Apr 25, 2017
@ayg
Copy link
Contributor Author

ayg commented Apr 25, 2017

Updated to reflect the fact that this affects collapse() too.

@ayg
Copy link
Contributor Author

ayg commented Apr 25, 2017

@masayuki-nakano points out that this introduces an inconsistency with extend() and setBaseAndExtent(), which do not throw in this case in Chrome. I think we should throw in all cases, because it matches the current behavior of Edge and Firefox and probably the old behavior of Chrome.

@ayg
Copy link
Contributor Author

ayg commented Apr 25, 2017

I wrote a test case:

  • collapse to detached doctype: Chrome, Firefox, and Edge throw; WebKit doesn't
  • extend to detached doctype: Firefox and Edge throw; Chrome and WebKit don't
  • extend to out-of-range index: Firefox, Edge, and WebKit throw; Chrome doesn't
  • selectAllChildren on detached doctype: Chrome, Firefox, and Edge throw; WebKit doesn't
  • setBaseAndExtent on detached doctype: Chrome, Firefox, and Edge throw; WebKit doesn't
  • setBaseAndExtent on out-of-range index: Chrome and Firefox throw; Edge and WebKit don't

So in most cases three out of four throw, and in the rest it's 50/50, so I think we should just throw in all cases here.

@ayg
Copy link
Contributor Author

ayg commented Apr 25, 2017

I should add that usually when browsers disagree, we have a strong preference for not throwing, but I think maybe this is different because half of the not-throwing behavior is due to the fact that WebKit doesn't ever throw when setting a range endpoint to a doctype. This is clearly not required by web compatibility. If you leave out WebKit from the doctype cases, behavior is 100% in favor of throwing in most cases. But on second thought, it's always easier not to throw, so maybe the spec should say never to throw. @rniwa, what do you think?

This issue blocks Mozilla's implementation, which I guess therefore won't get done until August.

@rniwa
Copy link
Contributor

rniwa commented May 9, 2017

I can be convinced about throwing for doctype but I don't think we'd like to change the behavior for out-of-range index since we've seen web contents and App-specific web content that do use such index and expect WebKit to do the "right thing".

@ayg
Copy link
Contributor Author

ayg commented Aug 3, 2017

@rniwa By out-of-range index you mean for setBaseAndExtent specifically? For all the others it looks like WebKit already throws, according to my earlier testing.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 13, 2017
…asayuki

This matches the spec and Chrome, and seems to bring us closer to Edge
and WebKit as well.  It also matches our own behavior for addRange(),
which was changed in bug 1341137.

For collapse and selectAllChildren, we match the tests and browsers, but
the spec is incorrect at the time of this writing:
w3c/selection-api#86

The removeAllRanges test hadn't been updated for the spec change.

MozReview-Commit-ID: DTK8283k5IP

--HG--
extra : rebase_source : 54701e7136c33ebce651d5f74c3dc1d8b944f9a3
luke-chang pushed a commit to luke-chang/gecko that referenced this pull request Aug 14, 2017
…asayuki

This matches the spec and Chrome, and seems to bring us closer to Edge
and WebKit as well.  It also matches our own behavior for addRange(),
which was changed in bug 1341137.

For collapse and selectAllChildren, we match the tests and browsers, but
the spec is incorrect at the time of this writing:
w3c/selection-api#86

The removeAllRanges test hadn't been updated for the spec change.

MozReview-Commit-ID: DTK8283k5IP
JerryShih pushed a commit to JerryShih/gecko-dev that referenced this pull request Aug 14, 2017
…asayuki

This matches the spec and Chrome, and seems to bring us closer to Edge
and WebKit as well.  It also matches our own behavior for addRange(),
which was changed in bug 1341137.

For collapse and selectAllChildren, we match the tests and browsers, but
the spec is incorrect at the time of this writing:
w3c/selection-api#86

The removeAllRanges test hadn't been updated for the spec change.

MozReview-Commit-ID: DTK8283k5IP
aethanyc pushed a commit to aethanyc/gecko-dev that referenced this pull request Aug 15, 2017
…asayuki

This matches the spec and Chrome, and seems to bring us closer to Edge
and WebKit as well.  It also matches our own behavior for addRange(),
which was changed in bug 1341137.

For collapse and selectAllChildren, we match the tests and browsers, but
the spec is incorrect at the time of this writing:
w3c/selection-api#86

The removeAllRanges test hadn't been updated for the spec change.

MozReview-Commit-ID: DTK8283k5IP
jgraham pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 31, 2017
This matches the spec and Chrome, and seems to bring us closer to Edge
and WebKit as well.  It also matches our own behavior for addRange(),
which was changed in bug 1341137.

For collapse and selectAllChildren, we match the tests and browsers, but
the spec is incorrect at the time of this writing:
w3c/selection-api#86

The removeAllRanges test hadn't been updated for the spec change.

MozReview-Commit-ID: DTK8283k5IP

Upstreamed from https://bugzilla.mozilla.org/show_bug.cgi?id=1359397 [ci skip]
rachelandrew pushed a commit to rachelandrew/web-platform-tests that referenced this pull request Nov 8, 2017
This matches the spec and Chrome, and seems to bring us closer to Edge
and WebKit as well.  It also matches our own behavior for addRange(),
which was changed in bug 1341137.

For collapse and selectAllChildren, we match the tests and browsers, but
the spec is incorrect at the time of this writing:
w3c/selection-api#86

The removeAllRanges test hadn't been updated for the spec change.

MozReview-Commit-ID: DTK8283k5IP

Upstreamed from https://bugzilla.mozilla.org/show_bug.cgi?id=1359397 [ci skip]
jakearchibald pushed a commit to jakearchibald/web-platform-tests that referenced this pull request Nov 16, 2017
This matches the spec and Chrome, and seems to bring us closer to Edge
and WebKit as well.  It also matches our own behavior for addRange(),
which was changed in bug 1341137.

For collapse and selectAllChildren, we match the tests and browsers, but
the spec is incorrect at the time of this writing:
w3c/selection-api#86

The removeAllRanges test hadn't been updated for the spec change.

MozReview-Commit-ID: DTK8283k5IP

Upstreamed from https://bugzilla.mozilla.org/show_bug.cgi?id=1359397 [ci skip]
@marcoscaceres
Copy link
Member

@rniwa seems there was only one small issue for @ayg to address here (as webkit already throws in various places that @ayg tested)? Can we move this forward?

@marcoscaceres marcoscaceres requested a review from rniwa May 23, 2019 07:05
@rniwa
Copy link
Contributor

rniwa commented May 23, 2019

@marcoscaceres : if someone can fix the PR, then yes. I don't think we can marge the PR as is due to the aforementioned issue.

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…asayuki

This matches the spec and Chrome, and seems to bring us closer to Edge
and WebKit as well.  It also matches our own behavior for addRange(),
which was changed in bug 1341137.

For collapse and selectAllChildren, we match the tests and browsers, but
the spec is incorrect at the time of this writing:
w3c/selection-api#86

The removeAllRanges test hadn't been updated for the spec change.

MozReview-Commit-ID: DTK8283k5IP

UltraBlame original commit: 0bda6393453ef6ca289a37aa723f87f91160c66f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
…asayuki

This matches the spec and Chrome, and seems to bring us closer to Edge
and WebKit as well.  It also matches our own behavior for addRange(),
which was changed in bug 1341137.

For collapse and selectAllChildren, we match the tests and browsers, but
the spec is incorrect at the time of this writing:
w3c/selection-api#86

The removeAllRanges test hadn't been updated for the spec change.

MozReview-Commit-ID: DTK8283k5IP

UltraBlame original commit: 0bda6393453ef6ca289a37aa723f87f91160c66f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…asayuki

This matches the spec and Chrome, and seems to bring us closer to Edge
and WebKit as well.  It also matches our own behavior for addRange(),
which was changed in bug 1341137.

For collapse and selectAllChildren, we match the tests and browsers, but
the spec is incorrect at the time of this writing:
w3c/selection-api#86

The removeAllRanges test hadn't been updated for the spec change.

MozReview-Commit-ID: DTK8283k5IP

UltraBlame original commit: 0bda6393453ef6ca289a37aa723f87f91160c66f
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