It all used to be in the frame. Now some of it is in the editor state, some is not, and it's a bit messy. We should just centralize this in the editor state.

MozReview-Commit-ID: IyFv8NRuZIO

MozReview-Commit-ID: L2Ozu7Vvort

MozReview-Commit-ID: Ca95YfRaq9r

MozReview-Commit-ID: IDdt0qedJpT

Really, there are only two cases we need to worry about. Either IsSelectionCached(), and then our SelectionProperties has the data we want, or not and then we have a non-null mSelCon which has the data we want. Since we are now using cached selection state a lot more (instead of initializing the editor whenever someone asks for selection state), we need to actually update it more correctly when .value is set. And since we now update the cached selection state for the default case (to point to the end of the text), we need to change HTMLInputElement::HasCachedSelection to return false for that default case. Otherwise we will always do eager editor init. We handle that by not doing eager init if the cached selection is collapsed. The web platform test changes test the "update on .value set" behavior. They fail without this patch, pass with it. MozReview-Commit-ID: DDU8U4MGb23

MozReview-Commit-ID: EQWxjgTdloR

MozReview-Commit-ID: FNn4vVCM50s

Really, there are only two cases we need to worry about. Either IsSelectionCached(), and then our SelectionProperties has the data we want, or not and then we have a non-null mSelCon which has the data we want. MozReview-Commit-ID: AEW9D1zG6sM

MozReview-Commit-ID: G7ODMdAjzxV

MozReview-Commit-ID: 5xUkcnkptwQ

This introduces three behavior changes: 1) Before this change, in cached mode, we did not enforce the "start <= end" invariant. 2) Before this change, in cached mode, we did not fire "select" events on selectionStart changes. 3) Changes the IDL type of HTMLInputElement's selectionStart attribute to "unsigned long" to match the spec and HTMLTextareaElement. MozReview-Commit-ID: JM9XXMMPUHM

This introduces three behavior changes: 1) Before this change, in cached mode, we did not enforce the "start <= end" invariant. 2) Before this change, in cached mode, we did not fire "select" events on selectionEnd changes. 3) Changes the IDL type of HTMLInputElement's selectionEnd attribute to "unsigned long" to match the spec and HTMLTextareaElement. MozReview-Commit-ID: J3Gkhr8VnbS

This introduces two behavior changes: 1) In cached mode, we used to treat unknown selection directions as "none". Now we treat it like "forward", consistently with the "have an editor" mode. 2) Before this change, in cached mode, we did not fire "select" events on selectionDirection changes. MozReview-Commit-ID: 4nBCAm3mAiz

MozReview-Commit-ID: 1bLLYhjmlff

MozReview-Commit-ID: E8zYAWolg94

MozReview-Commit-ID: HhmTHjw8AwW

MozReview-Commit-ID: FEo9yv5iu6U

In the end, I ended up mostly centralizing stuff from input/textarea. nsTextControlFrame::SetSelectionRange and its callees (SetSelectionEndPoints and OffsetToDOMPoint) are still around. I _think_ the former can be moved pretty easily, because the text editor state knows about editor initialization. But nsTextControlFrame::OffsetToDOMPoint is a bit of a hassle, because it pokes around inside the editor's anon content. If we're OK with doing that, we can probably move it. After the patches in this bug, the only caller of SetSelectionRange is the editor state itself. SetSelectionEndPoints is also called from nsTextControlFrame::EnsureEditorInitialized, which may need a bit more thought. Maybe it's redundant with work we do from the editor state now?

Comment on attachment 8842060 [details] [diff] [review] part 5. Simplify the setup around the editor state's GetSelectionRange function Review of attachment 8842060 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/HTMLInputElement.cpp @@ +6484,1 @@ > return Nullable<int32_t>(selEnd); Is this returning uninitialized memory? I know it won't be read, but still. ::: dom/html/nsTextEditorState.cpp @@ +1600,5 @@ > > dom::Selection* sel = selection->AsSelection(); > mozilla::dom::Element* root = GetRootNode(); > + if (NS_WARN_IF(!root)) { > + aRv.Throw(NS_ERROR_UNEXPECTED); ... and return ::: testing/web-platform/tests/html/semantics/forms/textfieldselection/selection.html @@ +89,5 @@ > > > test(function() { > + for (var el of [createInputElement(sampleText, true), > + createInputElement(sampleText, false)]) { Nit: for (var append of [true, false]) { var el = createInputElement(sampleText, append); ... and perhaps move the test() inside the loop

Comment on attachment 8842066 [details] [diff] [review] part 11. Implement nsTextEditorState::SetSelectionStart Review of attachment 8842066 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/html/semantics/forms/textfieldselection/selection-start-end.html @@ +53,5 @@ > + } > + }, "onselect should fire when selectionStart is changed"); > + > + test(function() { > + for (let el of createTestElements(testValue)) { Please use `var`; Servo is on an ancient SM that doesn't have for-{let,const}-of yet.

> Is this returning uninitialized memory? I know it won't be read, but still. Yes, it is. I was a bit torn on whether to add an extra branch and return Nullable<int32_t>()... It seems annoying. But yes, something like valgrind may not be happy. :( >... and return Good catch. I wonder whether that static analysis has landed yet.... > for (var append of [true, false]) { OK, I can do that. And yes, moving the test() inside the true/false loop makes some sense. Though it makes things a bit weird in UAs that don't do for-of. > Please use `var`; Servo is on an ancient SM that doesn't have for-{let,const}-of yet. That would require nasty changes to other parts of the test, because the loop body creates closures that reference "el". Using "var" would lead those closures to see the wrong value. It can be done, with some hoops, but...

Really, there are only two cases we need to worry about. Either IsSelectionCached(), and then our SelectionProperties has the data we want, or not and then we have a non-null mSelCon which has the data we want. Since we are now using cached selection state a lot more (instead of initializing the editor whenever someone asks for selection state), we need to actually update it more correctly when .value is set. And since we now update the cached selection state for the default case (to point to the end of the text), we need to change HTMLInputElement::HasCachedSelection to return false for that default case. Otherwise we will always do eager editor init. We handle that by not doing eager init if the cached selection is collapsed. The web platform test changes test the "update on .value set" behavior. They fail without this patch, pass with it. MozReview-Commit-ID: DDU8U4MGb23

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #21) > > Please use `var`; Servo is on an ancient SM that doesn't have for-{let,const}-of yet. > > That would require nasty changes to other parts of the test, because the > loop body creates closures that reference "el". Using "var" would lead > those closures to see the wrong value. It can be done, with some hoops, > but... Ugh, ok... Leave it as-is, I'll deal.

Comment on attachment 8842058 [details] [diff] [review] part 3. Get rid of nsIDOMHTMLTextareaElement's selectionStart and selectionEnd accessors Review of attachment 8842058 [details] [diff] [review]: ----------------------------------------------------------------- Good riddance!

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #18) > In the end, I ended up mostly centralizing stuff from input/textarea. > > nsTextControlFrame::SetSelectionRange and its callees (SetSelectionEndPoints > and OffsetToDOMPoint) are still around. I _think_ the former can be moved > pretty easily, because the text editor state knows about editor > initialization. But nsTextControlFrame::OffsetToDOMPoint is a bit of a > hassle, because it pokes around inside the editor's anon content. If we're > OK with doing that, we can probably move it. Yes, that's fine. Various bits and pieces in Gecko do depend on the editor's native anonymous content already and there is no point in drawing a line in the sand here and pretend it means anything. :-) > After the patches in this bug, > the only caller of SetSelectionRange is the editor state itself. > SetSelectionEndPoints is also called from > nsTextControlFrame::EnsureEditorInitialized, which may need a bit more > thought. Maybe it's redundant with work we do from the editor state now? Hmm, I wouldn't expect any code in EnsureEditorInitialized to be performance sensitive, so I'd leave it as is. That setup is a house of cards...

(In the actual initialization, I mean.)

Comment on attachment 8842063 [details] [diff] [review] part 8. Simplify the setup around the editor state's GetSelectionDirection function Review of attachment 8842063 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/nsTextEditorState.cpp @@ +1628,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + aRv.Throw(rv); > + return nsITextControlFrame::eForward; // Doesn't really matter > + } > + if (NS_WARN_IF(!selection)) { Nit: Instead of duplicating code, how about we do this first: if (!selection) { rv = NS_ERROR_FAILURE; }

Comment on attachment 8842065 [details] [diff] [review] part 10. Implement a SetSelectionRange function on nsTextEditorState Review of attachment 8842065 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/nsTextEditorState.cpp @@ +1685,5 @@ > + rv = mBoundFrame->ScrollSelectionIntoView(); > + // Press on to firing the event even if that failed, like our old code did. > + // But is that really what we want? Firing the event _and_ throwing from > + // here is weird. Maybe we should just ignore ScrollSelectionIntoView > + // failures? I think the right thing to do here is to ignore the return value of this function. In reality, I think we only should have async scrolling here (right?), so this should only fail when we fail to dispatch a runnable, which means OOM or something? @@ +1689,5 @@ > + // failures? > + > + // XXXbz This is preserving our current behavior of firing a "select" event > + // on all mutations when we have an editor, but we should really consider > + // fixing that... Do you mind filing follow-ups for both of these, please?

Comment on attachment 8842104 [details] [diff] [review] part 5. Simplify the setup around the editor state's GetSelectionRange function Review of attachment 8842104 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/nsTextEditorState.cpp @@ +1579,5 @@ > + // GetSelectionController() if we haven't initialized our editor yet. > + if (IsSelectionCached()) { > + const SelectionProperties& props = GetSelectionProperties(); > + *aSelectionStart = props.GetStart(); > + *aSelectionEnd = props.GetEnd(); Nice! @@ +1592,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + aRv.Throw(rv); > + return; > + } > + if (NS_WARN_IF(!selection)) { Same comment as the one on one of the previous patches. @@ +2282,5 @@ > + // selection state, and we should point it to the end of the new value. > + if (IsSelectionCached()) { > + SelectionProperties& props = GetSelectionProperties(); > + props.SetStart(value.Length()); > + props.SetEnd(value.Length()); I'm not 100% sure if this always does the right thing in the face of bug 1337392. The value here can sometimes be the default value, right? r- for that.

Comment on attachment 8842066 [details] [diff] [review] part 11. Implement nsTextEditorState::SetSelectionStart Review of attachment 8842066 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/nsTextEditorState.cpp @@ +1714,5 @@ > + int32_t start = 0; > + if (!aStart.IsNull()) { > + // XXXbz This will do the wrong thing for input values that are out of the > + // int32_t range... > + start = aStart.Value(); Doesn't this mean that we'd interpret |start| as a negative integer for such values? That doesn't sound OK... For the next iteration, can you please add a test examining this?

Comment on attachment 8842067 [details] [diff] [review] part 12. Implement nsTextEditorState::SetSelectionEnd Review of attachment 8842067 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/nsTextEditorState.cpp @@ +1742,5 @@ > +{ > + int32_t end = 0; > + if (!aEnd.IsNull()) { > + // XXXbz This will do the wrong thing for input values that are out of the > + // int32_t range... Similar r-, and we need similar tests here also.

Comment on attachment 8842071 [details] [diff] [review] part 16. Remove the now-unused nsITextControlElement::GetSelectionRange Review of attachment 8842071 [details] [diff] [review]: ----------------------------------------------------------------- Nit: Please make the commit message say something about de-virtualization going on here as well.

> I wouldn't expect any code in EnsureEditorInitialized to be performance sensitive, so I'd leave it as is. Right, the question is which class SetSelectionEndPoints should live in. ;) I filed bug 1345228. > Nit: Instead of duplicating code, how about we do this first: The code duplication goes away in bug 1343275 anyway, since we switch to a non-XPCOM getter. I'll just land that together with this... > Do you mind filing follow-ups for both of these, please? Done. Bug 1345224 and bug 1345227. > Same comment as the one on one of the previous patches. Same response: code goes away in bug 1343275. > I'm not 100% sure if this always does the right thing in the face of bug 1337392. Hmm. I will look into what's up here. > Doesn't this mean that we'd interpret |start| as a negative integer for such values? Yes. That's exactly what we do right now, afaict, at the binding layer. So this isn't really changing behavior, just moving the place where the bad coercion happens... I agree it's not necessarily ok, but it is what it is at the moment, given that all the stuff is int32_t once you get far enough down. We should really move all these selectionstart/end bits over to uint32_t, right? I can do a followup for that, if you'd like. Or a prequel to this bug, I suppose...

> I'm not 100% sure if this always does the right thing in the face of bug 1337392. Ah, you mean with that bug _fixed_. I basically preserved our existing behavior, which that bug is changing. I'll wait for that to land, then rebase on top and fix tests to pass. Updated patch coming up.

I filed bug 1345237 on the int32_t issue.

Boris and I talked on IRC about this. I didn't realize the int32_t issues were pre-existing. r+ on those.

MozReview-Commit-ID: L7LNF2Bfwgk

It's not that much saner when there is an _editor_, as the tests will show. This patch applies on top of the 'Addendum to part 5' patch.

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cad4522cee16 part 1. Get rid of nsIDOMHTMLInputElement's selectionStart accessors. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/6dcb7090fd33 part 2. Get rid of nsIDOMHTMLInputElement's selectionEnd accessors. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb2aacfc644 part 3. Get rid of nsIDOMHTMLTextareaElement's selectionStart and selectionEnd accessors. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/1034463cb379 part 4. Fix type changes on an input to properly grab the selection offsets from the old editor before we ask the editor state for them. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/23164576aaf5 part 5. Make <textarea> behave more like <input type=text> in terms of reset behavior. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/de068e5a963b part 6. Simplify the setup around the editor state's GetSelectionRange function. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/08b01e3c03f8 part 7. Get rid of nsIDOMHTMLTextareaElement's selectionDirection attribute. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/b7de9c9c1c31 part 8. Get rid of nsIDOMHTMLInputElement's selectionDirection attribute. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc2e304113b part 9. Simplify the setup around the editor state's GetSelectionDirection function. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/73198c9c3975 part 10. Remove the unused SetSelectionStart/SetSelectionEnd bits on text control frame. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/77c3b36269a4 part 11. Implement a SetSelectionRange function on nsTextEditorState. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/271e63cd7bfa part 12. Implement nsTextEditorState::SetSelectionStart. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/202a7b464808 part 13. Implement nsTextEditorState::SetSelectionEnd. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/a6833ad9712f part 14. Implement nsTextEditorState::SetSelectionDirection. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/67d29abbbd53 part 15. Implement nsTextEditorState::GetSelectionDirection. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/1d0da4f3c82b part 16. Implement a version of nsTextEditorState::SetSelectionRange that takes a string for the direction. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4ec6e6b168 part 17. Remove the now-unused nsITextControlElement::GetSelectionRange. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/77c26865ce8e part 18. Implement nsTextEditorState::SetRangeText. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/45f9d9f47222 part 19. Add some tests. r=ehsan

I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=82842964&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/f584babc1167

OK, so this is exciting. The reason it fails is the first two patches in this bug. The ones that make the form fill controller return an error nsresult when returning random data.... but also make it call the per-spec selectionStart/End getters. Those getters throw for <input type="email">, while our XPCOM getters do not. And the formfill test at toolkit/components/satchel/test/test_form_autocomplete.html does stuff with an <input type="email">. Oh, and the code that gets selectionStart/End is in nsAutoCompleteController::HandleKeyNavigation and is #ifdef XP_MACOSX, which is why my try runs didn't run into this. Anyway, before the patches in this bug, those selectionStart/End gets worked on that input. With the patches they actually start returning an error nsresult and random outparam value. But the caller ignores the return value (!), and the random values it got did not make it happy. I'm going to add versions of GetSelectionStart/GetSelectionEnd for use from the form fill controller that ignore the input type and just work any time we have an editor state.

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/02897bc3cc41 part 1. Get rid of nsIDOMHTMLInputElement's selectionStart accessors. r=ehsan,MattN https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea9a26ce33a part 2. Get rid of nsIDOMHTMLInputElement's selectionEnd accessors. r=ehsan,MattN https://hg.mozilla.org/integration/mozilla-inbound/rev/3512debaa707 part 3. Get rid of nsIDOMHTMLTextareaElement's selectionStart and selectionEnd accessors. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/08db504dc8e4 part 4. Fix type changes on an input to properly grab the selection offsets from the old editor before we ask the editor state for them. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/5118456a4b9d part 5. Make <textarea> behave more like <input type=text> in terms of reset behavior. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/605f2142963d part 6. Simplify the setup around the editor state's GetSelectionRange function. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/70ea4278d286 part 7. Get rid of nsIDOMHTMLTextareaElement's selectionDirection attribute. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3520682a3f part 8. Get rid of nsIDOMHTMLInputElement's selectionDirection attribute. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/4a7dfcd863e1 part 9. Simplify the setup around the editor state's GetSelectionDirection function. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe6ea1d1332 part 10. Remove the unused SetSelectionStart/SetSelectionEnd bits on text control frame. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/b3416e9f0d5c part 11. Implement a SetSelectionRange function on nsTextEditorState. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/69da5429c4b4 part 12. Implement nsTextEditorState::SetSelectionStart. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/301369c6601c part 13. Implement nsTextEditorState::SetSelectionEnd. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed58ad7b2d5 part 14. Implement nsTextEditorState::SetSelectionDirection. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/6628ee6c2248 part 15. Implement nsTextEditorState::GetSelectionDirection. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c1dd9a7a13 part 16. Implement a version of nsTextEditorState::SetSelectionRange that takes a string for the direction. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/48bc790fbaf5 part 17. Remove the now-unused nsITextControlElement::GetSelectionRange. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/ff3d9260a132 part 18. Implement nsTextEditorState::SetRangeText. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/d737e4692b28 part 19. Add some tests. r=ehsan

https://hg.mozilla.org/mozilla-central/rev/02897bc3cc41 https://hg.mozilla.org/mozilla-central/rev/0ea9a26ce33a https://hg.mozilla.org/mozilla-central/rev/3512debaa707 https://hg.mozilla.org/mozilla-central/rev/08db504dc8e4 https://hg.mozilla.org/mozilla-central/rev/5118456a4b9d https://hg.mozilla.org/mozilla-central/rev/605f2142963d https://hg.mozilla.org/mozilla-central/rev/70ea4278d286 https://hg.mozilla.org/mozilla-central/rev/ba3520682a3f https://hg.mozilla.org/mozilla-central/rev/4a7dfcd863e1 https://hg.mozilla.org/mozilla-central/rev/ebe6ea1d1332 https://hg.mozilla.org/mozilla-central/rev/b3416e9f0d5c https://hg.mozilla.org/mozilla-central/rev/69da5429c4b4 https://hg.mozilla.org/mozilla-central/rev/301369c6601c https://hg.mozilla.org/mozilla-central/rev/2ed58ad7b2d5 https://hg.mozilla.org/mozilla-central/rev/6628ee6c2248 https://hg.mozilla.org/mozilla-central/rev/b7c1dd9a7a13 https://hg.mozilla.org/mozilla-central/rev/48bc790fbaf5 https://hg.mozilla.org/mozilla-central/rev/ff3d9260a132 https://hg.mozilla.org/mozilla-central/rev/d737e4692b28