Implement input.setSelectionRange

[saurvs]

Fixes #9862.

Passes all tests for input in tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/textfieldselection-setSelectionRange.html.

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[jdm]

Looks like textfieldselection-setSelectionRange.html.ini should be removed as part of this PR, in that case!

[emilio]

Looks good!

I left a few comments, and don't forget to remove the test expectations as @jdm said! :)

-S-awaiting-review +S-needs-code-changes

---

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

components/script/textinput.rs, line 520 [r1] (raw file):
Could this underflow? From a quick look it looks like it, but I might be wrong.

---

components/script/dom/htmlinputelement.rs, line 94 [r1] (raw file):
Please call this selection_direction.

---

components/script/dom/htmlinputelement.rs, line 459 [r1] (raw file):
There should be links to the spec for this, right?

Ditto for all the other webidl methods.

---

components/script/dom/htmlinputelement.rs, line 521 [r1] (raw file):
DOMString can dereference automatically to a &str, so this String::from should not be needed.

---

Comments from the review on Reviewable.io

[jdm]

You'll need to run ./mach test-tidy and address the stylistic issues that it reports.

[saurvs]

@ecoal95 Yes, get_text_point_for_absolute_point() did underflow when there was an empty string in textinput.lines. It's fixed now.

Also, I had to modify a single wpt test; input.selectionEnd should point to the end of the text, which is it's length minus 1.

[jdm]

Note to reviewer: Firefox currently passes the length+1 test that's being modified, so please check carefully that the test actually needs modification rather than the implementation.

[saurvs]

@jdm I guess I misread the spec slightly. Sorry!

[emilio]

A few more nits, good job! :)

-S-awaiting-review -S-fails-tidy +S-needs-code-changes

---

Reviewed 3 of 3 files at r1.
Review status: 1 of 5 files reviewed at latest revision, 7 unresolved discussions.

---

components/script/textinput.rs, line 520 [r5] (raw file):
This doesn't seem to be fixed. If self.lines is empty this will cause a panic in checked builds, even when this value is not used. Maybe an early return in the case self.lines is empty could be more elegant, but a conditional would also be fine.

If for some reason lines can't be empty, I'd love a comment explaining why :)

---

components/script/textinput.rs, line 531 [r5] (raw file):
Aren't this lines equivalent to let line_end = min(val.len(), 1)?

---

components/script/textinput.rs, line 539 [r5] (raw file):
Please keep the else in the same line as the closing brace.

---

components/script/dom/htmlinputelement.rs, line 521 [r1] (raw file):
This idiom is not used a lot, &**selection_direction should be the same and is more used across servo's codebase.

---

Comments from the review on Reviewable.io

[saurvs]

Review status: 1 of 4 files reviewed at latest revision, 7 unresolved discussions.

---

components/script/textinput.rs, line 520 [r5] (raw file):
self.lines is never going to empty as long as TextInput is created with it's new(lines: Lines, initial: DOMString, .. method; even if an empty string is passed to initial, with either Single or Multiple passed to lines, self.lines will always have one element. And so last_line_idx won't underflow.

---

components/script/textinput.rs, line 531 [r5] (raw file):
Actually it should be max(val.len(), 1) because, if it was min(val.len(), 1) then the algorithm would increment line every two characters. Anyway, the code is shorter now. Thanks!

---

Comments from the review on Reviewable.io

[emilio]

This looks good to me, thank you for doing this! :)

I left a comment, to avoid a few unnecessary string allocations.

Please let me know if you'd prefer this to land as-is and leave that as a followup (or file an issue in case you can't/don't want to), or doing it now.

-S-awaiting-review +S-awaiting-answer

---

Reviewed 1 of 4 files at r3, 2 of 3 files at r6.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

components/script/textinput.rs, line 520 [r5] (raw file):
Ok, fine then :)

---

components/script/textinput.rs, line 531 [r5] (raw file):
Whoops, sorry, writing review comments at night, my fault... :P

---

components/script/dom/htmlinputelement.rs, line 485 [r6] (raw file):
This (and a few more methods), needlessly allocates a new string with SelectionDirection.

We could create a new set_selection_range that gets directly the SelectionDirection instead of parsing it, and make SetSelectionRange just a wrapper over that.

It's fine as a followup though :)

---

Comments from the review on Reviewable.io

[saurvs]

Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

components/script/dom/htmlinputelement.rs, line 485 [r6] (raw file):
Yes that sounds like a good idea. I'll make that change right now.

---

Comments from the review on Reviewable.io

[emilio]

Great! Nice job!

For the next time, feel free to ping me when you push new commits, since GitHub doesn't send a notification :)

@bors-servo: r+

-S-awaiting-answer -S-awaiting-review +S-awaiting-merge

---

Reviewed 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

Comments from the review on Reviewable.io

[emilio]

Great, thank you again!

@bors-servo: r+

[bors-servo]

📌 Commit a3d7779 has been approved by ecoal95

[bors-servo]

⌛ Testing commit a3d7779 with merge baca08a...

[bors-servo]

💔 Test failed - linux-rel

[emilio]

@bors-servo: retry

• #9278

[emilio]

@bors-servo: retry

• #9278

[emilio]

@bors-servo: r+

[bors-servo]

💡 This pull request was already approved, no need to approve it again.

• There's another pull request that is currently being tested, blocking this pull request: #9948

[bors-servo]

📌 Commit a3d7779 has been approved by ecoal95

[bors-servo]

⌛ Testing commit a3d7779 with merge cff90fd...

[bors-servo]

💔 Test failed - status-appveyor

[jdm]

@bors-servo: retry

• rolling appveyor builds

[bors-servo]

⌛ Testing commit a3d7779 with merge eac68c5...

[bors-servo]

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor