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

selectionStart/selectionEnd behavior needs to be specified #2424

Closed
bzbarsky opened this issue Mar 8, 2017 · 29 comments
Closed

selectionStart/selectionEnd behavior needs to be specified #2424

bzbarsky opened this issue Mar 8, 2017 · 29 comments
Assignees
Labels
interop Implementations are not interoperable with each other topic: forms

Comments

@bzbarsky
Copy link
Contributor

bzbarsky commented Mar 8, 2017

Right now the spec is pretty much silent on how this stuff works, and as a result browsers have all sorts of behavior differences, both internally and across browsers. Some examples:

  1. In Chrome, var x = document.createElement(tag); x.defaultValue = "abc"; alert(x.selectionStart); will alert 0 if tag is "input" but 3 if tag is "textarea".

  2. In Safari, var x = document.createElement("textarea"); x.defaultValue = "abc"; alert(x.selectionStart); alerts 0. So do var x = document.createElement("textarea"); document.body.appendChild(x); x.defaultValue = "abc"; alert(x.selectionStart); and var x = document.createElement("textarea"); x.selectionStart = x.selectionStart; x.defaultValue = "abc"; alert(x.selectionStart);. But var x = document.createElement("textarea"); document.body.appendChild(x); x.selectionStart = x.selectionStart; x.defaultValue = "abc"; alert(x.selectionStart); alerts 3.

  3. In Gecko... there are various inconsistencies, esp between textarea vs input, display:none vs non-display-none, etc. I'm fixing some of this stuff right now, so not sure it's worth going into the details, which are changing.

OK, what does the spec actually say? It says that setting the "value" IDL attribute of an input that is in "value" mode will move the text entry cursor (if there is one!) to the end of the text.

It also says that setting the "value" IDL attribute of textarea should do the same thing.

It says that UAs must "follow platform conventions to determine their initial state" for the text entry cursor and selection.

That's it. There's no indication of how this initial state should get modified when the value or default value changes...

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Mar 8, 2017

So here is a proposal for specific behavior:

  1. The initial state of the text entry cursor is at the beginning of the text control. The web depends on this behavior, looks like; see https://bugzilla.mozilla.org/show_bug.cgi?id=1337392

  2. Setting .value moves the text entry cursor to the end.

  3. Anything else that modifies a value just ensures that the text entry cursor is at some position in the range [0, newlength], clamping it to end as needed.

This is what I tentatively plan to implement in Gecko.

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Mar 8, 2017

@cdumez @domenic @travisleithead

Note that I haven't had a chance to test Edge, so maybe it's got sane behavior....

@domenic
Copy link
Member

domenic commented Mar 8, 2017

/cc @tkent-google

Anything else that modifies a value just ensures that the text entry cursor is at some position in the range [0, newlength], clamping it to end as needed.

To be clear, this means leaving the text entry cursor alone, unless it would be outside the [0, newlength] range, in which case it gets clamped?

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Mar 8, 2017

Yes, exactly. Probably the same thing for selection too.

So basically, adjust selectionStart/selectionEnd to min(currentValue,newLength).

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Mar 8, 2017

One other problem: it's not clear to me what should happen when the control is focused, or maybe "has been focused". In that case, setting defaultValue seems to put the cursor at the end of the value in browsers? Not sure which parts here are interoperable, exactly.

@tkent-google
Copy link
Contributor

tkent-google commented Mar 9, 2017

I recently filed related issues:
#2411
#2412

  1. In Chrome, var x = document.createElement(tag); x.defaultValue = "abc"; alert(x.selectionStart); will alert 0 if tag is "input" but 3 if tag is "textarea".

This behavior is definitely a bug. Chrome doesn't update selection values for input/defaultValue change at all. selectionStart and selectionEnd can be out of range. Oops.

  1. The initial state of the text entry cursor is at the beginning of the text control. The web depends on this behavior, looks like;

I agree if this is already interoperable.

  1. Setting .value moves the text entry cursor to the end.

Agree. This is defined and interoperable.

  1. Anything else that modifies a value just ensures that the text entry cursor is at some position in the range [0, newlength], clamping it to end as needed.

I think in many cases modifying a value sets the cursor at the end of the text. At least updating textarea/@DefaultValue sets it to the end in all major browsers.

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Mar 9, 2017

I agree if this is already interoperable.

It's complicated. In particular there's ok interop for text controls that come from the parser, I think. But if you try to create one from script, you have to do the equivalent of setting .defaultValue, and suddenly implementations are all over the map per above.

Agree. This is defined and interoperable.

Though #2412 is proposing changing it a bit, right?

I think in many cases modifying a value sets the cursor at the end of the text.

Right, setting .value needs to do that per current spec. The question is about value changes that don't set .value but do change the value it returns.

Most obviously, modifying the value via setRangeText or the user typing or whatnot obviously don't move the cursor. It's not clear how .defaultValue sets should behave. What about reset() calls? Direct mutations to the .nodeValue of a child of a textarea? Might be worth testing what UAs do in practice...

At least updating textarea/@DefaultValue sets it to the end in all major browsers

That's only sometimes true in Firefox. I'm pretty sure it's false if the textarea is display:none, for example, or not attached to a document...

It's definitely false in Safari, unless I messed something up testing. See my first comment in this issue.

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Mar 9, 2017

At least updating textarea/@DefaultValue sets it to the end in all major browsers

As noted in https://bugzilla.mozilla.org/show_bug.cgi?id=1345293#c0 this is not necessarily true even in Chrome, depending on what "updating" means....

@bzbarsky
Copy link
Contributor Author

@domenic domenic self-assigned this Mar 14, 2017
@domenic
Copy link
Member

domenic commented Mar 14, 2017

I've posted #2437 and web-platform-tests/wpt#5147 around the first of these bugs, #2412, as that seems rather separable. But this and #2411 seem worth solving together; #2411 is largely a subset of the larger question posed here about what happens if the underlying value is changed, somehow.

@bzbarsky's plan in #2424 (comment) still seems sound as a way forward, modulo the change in #2437 to not set selection to the end if the value is the same. However the big question for me is how to spec it. We could either have something vague, like "whenever the value changes, run these steps", or we could try to find all the places that update the value. We'll need to do the latter for tests anyway. So far what I've got are:

  • Setting the defaultValue attribute, sometimes (depending on the dirty value flag IIRC)
  • Setting the value attribute
  • Setting textContent on textareas, sometimes (depending on dirty value flag).
  • Otherwise modifying child nodes of textareas in a way that would cause textContent to change (if dirty vale flag is set)
  • setRangeText()
  • The reset algorithm
  • Users typing

I guess from this perspective a vague "whenever the value changes" makes more sense.

For textareas, there's also a difference between changing the element's value and its raw value. It seems like we only care about changes to value; e.g. if you set the raw value to "hell\r\no", then set it to "hell\no", that should not change selection. That's the assumption I made in #2437 at least.

@bzbarsky
Copy link
Contributor Author

I guess from this perspective a vague "whenever the value changes" makes more sense.

You missed a few. Off the top of my head:

  • Browser form autofill.
  • Clipboard operations (paste, cut).
  • IME commit.
  • Form state restoration on history traversal.
  • Cloning steps, for the new clone.
  • Initial element creation.

You'd think some of these would be covered by your list, but at least Blink has different behavior for foo.innerHTML = "<input value='something'>" and var i = document.createElement("input"); i.setAttribute("value", "something"); foo.appendChild(i);: the former puts the cursor at the start, and the latter puts it at the end...

For textareas, there's also a difference between changing the element's value and its raw value.

Right. I'm not sure how these interact with selection in practice. Is the selection in the raw value or in the value?

@bzbarsky
Copy link
Contributor Author

Oh, and I think it's obvious that paste/cut should probably not modify selection/cursor stuff in any ways other than the obvious one.... But at this point it would merely sadden me, not surprise me, to discover that we have no interop there either...

@domenic
Copy link
Member

domenic commented Mar 15, 2017

You missed a few. Off the top of my head:

Sounds good. I'll try to add those to the test cases.

Right. I'm not sure how these interact with selection in practice. Is the selection in the raw value or in the value?

I imagine it should be on something with normalized line breaks (so value or API value) since you can't select the \r part of \r\n. I guess API value is the best fit there, hmm. But for determining changes either version with normalized line breaks works, see comment thread at #2437 (comment)

@bzbarsky
Copy link
Contributor Author

But for determining changes either version with normalized line breaks works

Don't they give different answers for the "did the value change?" question?

In an ideal world, our definition of "value did not change" should probably be such that doing foo.value = foo.value is treated as "value did not change".

@domenic
Copy link
Member

domenic commented Mar 15, 2017

Don't they give different answers for the "did the value change?" question?

I can't see how. They are just two different ways of normalizing away line breaks. As far as I can see apiValue1 === apiValue2 iff value1 === value2. An example input where this is not true would be appreciated.

@bzbarsky
Copy link
Contributor Author

As far as I can see apiValue1 === apiValue2 iff value1 === value2.

No, that's not true. Here's a testcase:

<!DOCTYPE html>
<form action="http://software.hixie.ch/utilities/cgi/test-tools/echo">
  <textarea name="one" cols="7" wrap="hard">some text</textarea>
  <textarea name="two" cols="7" wrap="hard">some
text</textarea>
  <hr>
  <script>
    var areas = document.querySelectorAll("textarea");
    document.writeln("API values match: ", areas[0].value == areas[1].value);
  </script>
  <hr>
  <input type="submit" value="submit me to see the 'value's">
</form>

The API values are different: one is "some text" and one is "some\ntext". The value ... well, the spec doesn't actually define what the value has to be in this case, though it has certain restrictions on it. In practice, in Firefox, both textareas have "some\r\ntext" as the value. Chrome and Safari seem to not support the "cols" attribute at all. But they allow CSS applied to the textarea to affect its value. So in Chrome and Safari, if I throw <style>textarea { font-family: monospace; width: 50px }</style> into the testcase the value becomes "some\r\ntext" for both textareas. Interop, sigh.

Of course the only spec requirement here is that the value not have more than 7 chars in a row without a \r\n between them, and that the name="two" textarea must have a \r\n after the 'e' of "some". So as far as I can tell "some\r\nt\r\ne\r\nx\r\nt" would be a perfectly valid value for the second control, or for the first one. "some te\r\nxt" would be a perfectly valid value for the first control, but not the second....

Of course the other direction works too: if I have two textareas, one with wrap="hard" and one without, they can have the same API value but different value.

@bzbarsky
Copy link
Contributor Author

I guess if you restrict to a single textarea, such that you can assume a particular value of the wrap attribute, and you assume that the "UA-defined algorithm" in https://html.spec.whatwg.org/multipage/forms.html#textarea-wrapping-transformation step 2 is a function of only the input string (which it's not required to be!), then value1 == value2 would imply that apiValue1 == apiValue2. But the converse would not necessarily hold.

@domenic
Copy link
Member

domenic commented Mar 15, 2017

Thanks. I was indeed thinking one text area, but I guess even then it's not so simple. I think we should compare API values then.

@bzbarsky
Copy link
Contributor Author

Comparing API values makes the most sense to me.

@domenic
Copy link
Member

domenic commented Jun 21, 2017

A related issue that comes very close to the above discussion but I don't think we ever explicitly touched on was noted by @bzbarsky in #2770: when setting the value to something smaller than the currently-selected range (e.g. setting the value to "ab" when selectionStart or selectionEnd are 3) the behavior is not interoperable.

Also, dropping a reminder to myself in this thread to file bugs on browsers for #2770 at the same time as filing bugs for however we fix this. I think filing separate bugs will be a bit annoying since anyone fixing the two bugs will end up touching very similar code so we shouldn't make them do it twice.

jonleighton added a commit to jonleighton/servo that referenced this issue Feb 15, 2018
The TextInput::assert_ok_selection() method is meant to ensure that we
are not getting into a state where a selection refers to a location in
the control's contents which doesn't exist.

However, before this change we could have a situation where the
internals of the TextInput are changed by another part of the code,
without using its public API. This could lead to us having an invalid
selection.

I did manage to trigger such a situation (see the test added in this
commit) although it is quite contrived. There may be others that I
didn't think of, and it's also possible that future changes could
introduce new cases. (Including ones which trigger panics, if indexing
is used on the assumption that the selection indices are always valid.)

The current HTML specification doesn't explicitly say that
selectionStart/End must remain within the length of the content, but
that does seems to be the consensus reached in a discussion of this:

whatwg/html#2424

The test case I've added here is currently undefined in the spec which
is why I've added it in tests/wpt/mozilla.
jonleighton added a commit to jonleighton/servo that referenced this issue Feb 15, 2018
The TextInput::assert_ok_selection() method is meant to ensure that we
are not getting into a state where a selection refers to a location in
the control's contents which doesn't exist.

However, before this change we could have a situation where the
internals of the TextInput are changed by another part of the code,
without using its public API. This could lead to us having an invalid
selection.

I did manage to trigger such a situation (see the test added in this
commit) although it is quite contrived. There may be others that I
didn't think of, and it's also possible that future changes could
introduce new cases. (Including ones which trigger panics, if indexing
is used on the assumption that the selection indices are always valid.)

The current HTML specification doesn't explicitly say that
selectionStart/End must remain within the length of the content, but
that does seems to be the consensus reached in a discussion of this:

whatwg/html#2424

The test case I've added here is currently undefined in the spec which
is why I've added it in tests/wpt/mozilla.
@emanchado
Copy link

I might be able to help a bit here, but I'd personally prefer to see the spec patch first.

I've never written tests for a standard so the more context and explicit instructions I have, the better :-) Should all those tests go into selection-start-end.html? A new file? However I think makes sense?

jonleighton added a commit to jonleighton/servo that referenced this issue Feb 15, 2018
The TextInput::assert_ok_selection() method is meant to ensure that we
are not getting into a state where a selection refers to a location in
the control's contents which doesn't exist.

However, before this change we could have a situation where the
internals of the TextInput are changed by another part of the code,
without using its public API. This could lead to us having an invalid
selection.

I did manage to trigger such a situation (see the test added in this
commit) although it is quite contrived. There may be others that I
didn't think of, and it's also possible that future changes could
introduce new cases. (Including ones which trigger panics, if indexing
is used on the assumption that the selection indices are always valid.)

The current HTML specification doesn't explicitly say that
selectionStart/End must remain within the length of the content, but
that does seems to be the consensus reached in a discussion of this:

whatwg/html#2424

The test case I've added here is currently undefined in the spec which
is why I've added it in tests/wpt/mozilla.
jonleighton added a commit to jonleighton/servo that referenced this issue Feb 16, 2018
The TextInput::assert_ok_selection() method is meant to ensure that we
are not getting into a state where a selection refers to a location in
the control's contents which doesn't exist.

However, before this change we could have a situation where the
internals of the TextInput are changed by another part of the code,
without using its public API. This could lead to us having an invalid
selection.

I did manage to trigger such a situation (see the test added in this
commit) although it is quite contrived. There may be others that I
didn't think of, and it's also possible that future changes could
introduce new cases. (Including ones which trigger panics, if indexing
is used on the assumption that the selection indices are always valid.)

The current HTML specification doesn't explicitly say that
selectionStart/End must remain within the length of the content, but
that does seems to be the consensus reached in a discussion of this:

whatwg/html#2424

The test case I've added here is currently undefined in the spec which
is why I've added it in tests/wpt/mozilla.
@annevk
Copy link
Member

annevk commented Feb 16, 2018

@emanchado (!) I strongly suspect that how you think makes sense will be just fine, but once you submit some you can ask for review on your PR and iterate from there.

domenic added a commit that referenced this issue Feb 17, 2018
Per the test case included as an example in the spec, this is more in
line with browser behavior, and makes more sense. A similar conclusion
was reached on related matters in
#2424, and also in
#2437.
domenic added a commit that referenced this issue Feb 17, 2018
@emanchado
Copy link

Ok, I have started looking into this (see emanchado/servo@181b375) but I have a bunch of questions:

  • In which repo should the tests go? I found out about the tests (and this issue) through Servo, so I was initially working on https://github.com/servo/servo, but I wonder if I should be sending the PR to https://github.com/w3c/web-platform-tests instead.
  • Is setRangeText the right way to modify the text? Setting a new value will destroy the selection, so this is the way I've found (used in other tests, too) that seems to work. I wonder if I should be testing other ways in addition to this one.
  • How to set the cursor position, is setSelectionRange (with the same beginning/end) the right way?
  • How to detect that the browser doesn't support empty selection, and how do I set the cursor position without setSelectionRange in those cases? Or is it still the same?
  • Is it expected that the two commented-out tests fail? First one, second one.

@jdm
Copy link
Member

jdm commented Feb 18, 2018

Working directly with the upstream repository would be preferable to upstreaming them from Servo's copy.

@jonleighton
Copy link

Is setRangeText the right way to modify the text?

What we're trying to specify/test here is what happens to the selection bounds when the underlying value of a field changes under various "edge case" scenarios. What happens to the selection bounds when setRangeText is called is already well-defined (ditto for setting value), so I wouldn't expect to see it in the new tests. The edge cases are other things like when invoking the reset algorithm causes a value change, or changing the type attribute of an input causes the value sanitization algorithm to change the field's value.

There are more edge cases listed in the links in @domenic's comment above:

In particular, we'll want to cover the cases in #2424 (comment) (including the reset algorithm, mentioned in #3468), plus those mentioned a comment later in #2424 (comment) where possible, plus type change/value sanitization as mentioned in #2424 (comment), plus the difference between value and API value for text areas as demonstrated in #2424 (comment).

How to set the cursor position, is setSelectionRange (with the same beginning/end) the right way?

I think that's fine. (Alternatively you could set selectionStart and selectionEnd separately.)

How to detect that the browser doesn't support empty selection, and how do I set the cursor position without setSelectionRange in those cases? Or is it still the same?

I don't think you can detect this, or need to. selectionStart is always either the start of the selection, or the text entry cursor position, and selectionEnd is always either the end of the selection, or the text entry cursor position.

bors-servo pushed a commit to servo/servo that referenced this issue Feb 22, 2018
Disallow mutating the internals of TextInput

The TextInput::assert_ok_selection() method is meant to ensure that we
are not getting into a state where a selection refers to a location in
the control's contents which doesn't exist.

However, before this change we could have a situation where the
internals of the TextInput are changed by another part of the code,
without using its public API. This could lead to us having an invalid
selection.

I did manage to trigger such a situation (see the test added in this
commit) although it is quite contrived. There may be others that I
didn't think of, and it's also possible that future changes could
introduce new cases. (Including ones which trigger panics, if indexing
is used on the assumption that the selection indices are always valid.)

The current HTML specification doesn't explicitly say that
selectionStart/End must remain within the length of the content, but
that does seems to be the consensus reached in a discussion of this:

whatwg/html#2424

The test case I've added here is currently undefined in the spec which
is why I've added it in tests/wpt/mozilla.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20051)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 23, 2018
…rom jonleighton:prevent-invalid-selections); r=jdm

The TextInput::assert_ok_selection() method is meant to ensure that we
are not getting into a state where a selection refers to a location in
the control's contents which doesn't exist.

However, before this change we could have a situation where the
internals of the TextInput are changed by another part of the code,
without using its public API. This could lead to us having an invalid
selection.

I did manage to trigger such a situation (see the test added in this
commit) although it is quite contrived. There may be others that I
didn't think of, and it's also possible that future changes could
introduce new cases. (Including ones which trigger panics, if indexing
is used on the assumption that the selection indices are always valid.)

The current HTML specification doesn't explicitly say that
selectionStart/End must remain within the length of the content, but
that does seems to be the consensus reached in a discussion of this:

whatwg/html#2424

The test case I've added here is currently undefined in the spec which
is why I've added it in tests/wpt/mozilla.

Source-Repo: https://github.com/servo/servo
Source-Revision: f6463c89d50b0ebc377bfe10bd0ffe0c59dd84ca

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : a8ee624291af3d58b8126cd067ea9f7292bfca6b
@emanchado
Copy link

Ok, see different approach in the branch selection-and-cursor-processing-input-textarea (commit 852ec69). Sorry for the delay, I've been busy.

It's obviously incomplete, but I seem to be getting better results:

  • The first two tests (for defaultValue) behave differently in Firefox and Chrome.
  • The color type switching test fails in both (in different ways, too). I wonder if this has defined behaviour and/or it's important.

Comments welcome. If this looks good now, I'll keep going and will try to cover all the mentioned cases.

nupurbaghel pushed a commit to paavininanda/servo that referenced this issue Mar 14, 2018
The TextInput::assert_ok_selection() method is meant to ensure that we
are not getting into a state where a selection refers to a location in
the control's contents which doesn't exist.

However, before this change we could have a situation where the
internals of the TextInput are changed by another part of the code,
without using its public API. This could lead to us having an invalid
selection.

I did manage to trigger such a situation (see the test added in this
commit) although it is quite contrived. There may be others that I
didn't think of, and it's also possible that future changes could
introduce new cases. (Including ones which trigger panics, if indexing
is used on the assumption that the selection indices are always valid.)

The current HTML specification doesn't explicitly say that
selectionStart/End must remain within the length of the content, but
that does seems to be the consensus reached in a discussion of this:

whatwg/html#2424

The test case I've added here is currently undefined in the spec which
is why I've added it in tests/wpt/mozilla.
domenic added a commit that referenced this issue Mar 16, 2018
Per the test case included as an example in the spec, this is more in
line with browser behavior, and makes more sense. A similar conclusion
was reached on related matters in
#2424, and also in
#2437.
domenic added a commit that referenced this issue Mar 16, 2018
domenic added a commit that referenced this issue Mar 16, 2018
Per the test case included as an example in the spec, this is more in
line with browser behavior, and makes more sense. A similar conclusion
was reached on related matters in
#2424, and also in
#2437.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
Previously the spec allowed this to be "up to platform conventions", but
the web depends on it being at the beginning: see
https://bugzilla.mozilla.org/show_bug.cgi?id=1337392.

This also updates the spec to require resetting the cursor to the
beginning after type changes. This is not currently interoperable,
although it is the case in Edge and Gecko.

This was brought up in the context of the larger issue of whatwg#2424.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
Per the test case included as an example in the spec, this is more in
line with browser behavior, and makes more sense. A similar conclusion
was reached on related matters in
whatwg#2424, and also in
whatwg#2437.
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 2, 2019
…rom jonleighton:prevent-invalid-selections); r=jdm

The TextInput::assert_ok_selection() method is meant to ensure that we
are not getting into a state where a selection refers to a location in
the control's contents which doesn't exist.

However, before this change we could have a situation where the
internals of the TextInput are changed by another part of the code,
without using its public API. This could lead to us having an invalid
selection.

I did manage to trigger such a situation (see the test added in this
commit) although it is quite contrived. There may be others that I
didn't think of, and it's also possible that future changes could
introduce new cases. (Including ones which trigger panics, if indexing
is used on the assumption that the selection indices are always valid.)

The current HTML specification doesn't explicitly say that
selectionStart/End must remain within the length of the content, but
that does seems to be the consensus reached in a discussion of this:

whatwg/html#2424

The test case I've added here is currently undefined in the spec which
is why I've added it in tests/wpt/mozilla.

Source-Repo: https://github.com/servo/servo
Source-Revision: f6463c89d50b0ebc377bfe10bd0ffe0c59dd84ca

UltraBlame original commit: 9d5cf31fee15fe12d992bc4548c777e94be01664
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 2, 2019
…rom jonleighton:prevent-invalid-selections); r=jdm

The TextInput::assert_ok_selection() method is meant to ensure that we
are not getting into a state where a selection refers to a location in
the control's contents which doesn't exist.

However, before this change we could have a situation where the
internals of the TextInput are changed by another part of the code,
without using its public API. This could lead to us having an invalid
selection.

I did manage to trigger such a situation (see the test added in this
commit) although it is quite contrived. There may be others that I
didn't think of, and it's also possible that future changes could
introduce new cases. (Including ones which trigger panics, if indexing
is used on the assumption that the selection indices are always valid.)

The current HTML specification doesn't explicitly say that
selectionStart/End must remain within the length of the content, but
that does seems to be the consensus reached in a discussion of this:

whatwg/html#2424

The test case I've added here is currently undefined in the spec which
is why I've added it in tests/wpt/mozilla.

Source-Repo: https://github.com/servo/servo
Source-Revision: f6463c89d50b0ebc377bfe10bd0ffe0c59dd84ca

UltraBlame original commit: 9d5cf31fee15fe12d992bc4548c777e94be01664
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 2, 2019
…rom jonleighton:prevent-invalid-selections); r=jdm

The TextInput::assert_ok_selection() method is meant to ensure that we
are not getting into a state where a selection refers to a location in
the control's contents which doesn't exist.

However, before this change we could have a situation where the
internals of the TextInput are changed by another part of the code,
without using its public API. This could lead to us having an invalid
selection.

I did manage to trigger such a situation (see the test added in this
commit) although it is quite contrived. There may be others that I
didn't think of, and it's also possible that future changes could
introduce new cases. (Including ones which trigger panics, if indexing
is used on the assumption that the selection indices are always valid.)

The current HTML specification doesn't explicitly say that
selectionStart/End must remain within the length of the content, but
that does seems to be the consensus reached in a discussion of this:

whatwg/html#2424

The test case I've added here is currently undefined in the spec which
is why I've added it in tests/wpt/mozilla.

Source-Repo: https://github.com/servo/servo
Source-Revision: f6463c89d50b0ebc377bfe10bd0ffe0c59dd84ca

UltraBlame original commit: 9d5cf31fee15fe12d992bc4548c777e94be01664
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other topic: forms
Development

No branches or pull requests

7 participants