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

Move selection to end when textarea value is assigned #19358

Merged
merged 3 commits into from Nov 25, 2017

Conversation

Projects
None yet
6 participants
@jonleighton
Contributor

jonleighton commented Nov 23, 2017

Issue #19171


This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Nov 23, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/htmltextareaelement.rs
  • @KiChjang: components/script/dom/htmltextareaelement.rs
@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 23, 2017

@highfive highfive assigned KiChjang and unassigned emilio Nov 23, 2017

// Step 4
textinput.adjust_horizontal_to_limit(Direction::Forward, Selection::NotSelected);
} else {
textinput.selection_begin = old_selection;

This comment has been minimized.

@jonleighton

jonleighton Nov 23, 2017

Contributor

This is necessary because textinput.set_content resets the selection_begin field. But maybe it would be better to change set_content so that it doesn't do that unless necessary? (I.e. unless selection_begin now points to an invalid location.) It already does something similar with edit_point.

This comment has been minimized.

@jonleighton

jonleighton Nov 23, 2017

Contributor

By the way, we could just not call set_content at all if old_value == value. But I didn't go for that because the spec differentiates the "raw value" and the "API value". This differentiation isn't made in the code, yet, but I presumed that in the future we may want to call set_content, and then pull out the "API value" for the comparison. Maybe this is a premature assumption though.

This comment has been minimized.

@KiChjang

KiChjang Nov 23, 2017

Member

This looks fine to me. If the API value has not changed, selection_begin shouldn't point to any invalid location at all.

This comment has been minimized.

@KiChjang

KiChjang Nov 23, 2017

Member

So, I think for our implementation, we don't make the distinction between API values and raw values, because in set_content, we split the input DOMString via LF into multiple Strings in a Vec, and when we call get_content, the function knows to append LF to every String in the Vec, and return the result as a single DOMString.

However, looking at the algorithm to get an API value from HTMLTextAreaElement, I can see that we currently don't split CRLF or CR in set_content. So perhaps we should change that and ensure it does split on CRLF and CR.

This comment has been minimized.

@jonleighton

jonleighton Nov 23, 2017

Contributor

This looks fine to me. If the API value has not changed, selection_begin shouldn't point to any invalid location at all.

Yep, the implementation works fine, I just felt like changing set_content would make the code here a bit cleaner. I don't really mind either way though.

So perhaps we should change that and ensure it does split on CRLF and CR.

Yes, definitely extra handling for this will be needed in order to follow the spec. I haven't tried to address that in this PR but I agree it will need to be done. There are already some tests for this stuff in tests/wpt/web-platform-tests/html/semantics/forms/the-textarea-element/value-defaultValue-textContent.html. I think that it may not be enough to make set_content also split on CR/CRLF, as that would lose the original "raw value".

This comment has been minimized.

@KiChjang

KiChjang Nov 23, 2017

Member

I don't think that's an important concern, since every instance of the raw value is being converted into an API value, or a value, or that it's being used for comparison, in which case you can also use the corresponding API value or value to compare.

This comment has been minimized.

@KiChjang

KiChjang Nov 24, 2017

Member

This needs to be addressed.

This comment has been minimized.

@jonleighton

jonleighton Nov 24, 2017

Contributor

Ok, I felt like that was a separate change really, but I can take a look

@KiChjang

Nice work here! I have a couple of comments below.

// Step 4
textinput.adjust_horizontal_to_limit(Direction::Forward, Selection::NotSelected);
} else {
textinput.selection_begin = old_selection;

This comment has been minimized.

@KiChjang

KiChjang Nov 23, 2017

Member

This looks fine to me. If the API value has not changed, selection_begin shouldn't point to any invalid location at all.

// Step 4
textinput.adjust_horizontal_to_limit(Direction::Forward, Selection::NotSelected);
} else {
textinput.selection_begin = old_selection;

This comment has been minimized.

@KiChjang

KiChjang Nov 23, 2017

Member

So, I think for our implementation, we don't make the distinction between API values and raw values, because in set_content, we split the input DOMString via LF into multiple Strings in a Vec, and when we call get_content, the function knows to append LF to every String in the Vec, and return the result as a single DOMString.

However, looking at the algorithm to get an API value from HTMLTextAreaElement, I can see that we currently don't split CRLF or CR in set_content. So perhaps we should change that and ensure it does split on CRLF and CR.

"element.selectionEnd should be value.length");
}, `selection is always collapsed to the end after setting values on ${tag}`);
for (let val of ["", "foo", "foobar"]) {

This comment has been minimized.

@KiChjang

KiChjang Nov 23, 2017

Member

I think upstream W3C does not allow ES6 syntax, so we'll have to use var instead of let here.

This comment has been minimized.

@jonleighton

jonleighton Nov 23, 2017

Contributor

Are you sure about this? There is already loads of ES6 syntax in the existing tests (try ag "=>" tests/wpt/web-platform-tests). I used the backticks as I'd already seen them in another test, and then assumed that let would also be fine.

This comment has been minimized.

@jdm

jdm Nov 23, 2017

Member

ES6 is fine.

This comment has been minimized.

@KiChjang

KiChjang Nov 23, 2017

Member

Nevermind me, false alarm. WPT does support ES6 syntax.

This comment has been minimized.

@jonleighton

jonleighton Nov 24, 2017

Contributor

Ok great, so I think this can be merged? Was there something else?

assert_equals(el.selectionEnd, val.length,
"element.selectionEnd should be value.length");
}
}, `selection is collapsed to the end after changing values on ${tag}`);

This comment has been minimized.

@KiChjang

KiChjang Nov 23, 2017

Member

Same as above, we can't use backticks for string interpolation.

"element.selectionStart should be unchanged");
assert_equals(el.selectionEnd, 4,
"element.selectionEnd should be unchanged");
}, `selection is not collapsed to the end when value is set to its existing value on ${tag}`);

This comment has been minimized.

@KiChjang

KiChjang Nov 23, 2017

Member

Same here.

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 24, 2017

Added one more small commit. Everything in tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/selection-start-end.html is now passing. (But there are still parts of the selection API spec which are unimplemented, I'll look at that next.)

@jonleighton jonleighton force-pushed the jonleighton:issue-19171-3 branch from 0713071 to e177093 Nov 24, 2017

el.remove();
}
}, "Setting selectionStart to a value larger than selectionEnd should increase selectionEnd");
}, "Setting selectionStart to a value larger than selectionEnd should set selectionStart to selectionEnd");

This comment has been minimized.

@KiChjang

KiChjang Nov 24, 2017

Member

I presume you saw that text in set selection range step 3? That unfortunately isn't the correct place to look; on the selectionStart's setter's algorithm step 3, we can clearly see that end is supposed to be set to the given value, BEFORE running set selection range.

This comment has been minimized.

@jonleighton

jonleighton Nov 25, 2017

Contributor

Right, thanks, I did miss that nuance. Fixed now.

@jonleighton jonleighton force-pushed the jonleighton:issue-19171-3 branch from e177093 to a3afbb0 Nov 25, 2017

@KiChjang

This comment has been minimized.

Member

KiChjang commented Nov 25, 2017

@bors-servo r+

Thanks!

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 25, 2017

📌 Commit a3afbb0 has been approved by KiChjang

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 25, 2017

⌛️ Testing commit a3afbb0 with merge b00caa0...

bors-servo added a commit that referenced this pull request Nov 25, 2017

Auto merge of #19358 - jonleighton:issue-19171-3, r=KiChjang
Move selection to end when textarea value is assigned

Issue #19171

<!-- 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/19358)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 25, 2017

💔 Test failed - linux-rel-wpt

@jdm

This comment has been minimized.

Member

jdm commented Nov 25, 2017

  ▶ Unexpected subtest result in /html/semantics/forms/textfieldselection/selection.html:
  └ PASS [expected FAIL] test SelectionStart offset for textarea that is  not appended

  ▶ Unexpected subtest result in /html/semantics/forms/textfieldselection/selection.html:
  └ PASS [expected FAIL] test SelectionEnd offset for textarea that is  not appended

@jonleighton jonleighton force-pushed the jonleighton:issue-19171-3 branch from a3afbb0 to 9b06cb3 Nov 25, 2017

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 25, 2017

Should be fixed now 🤞

@KiChjang

This comment has been minimized.

Member

KiChjang commented Nov 25, 2017

@bors-servo r+

Gotta love more passing tests!

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 25, 2017

📌 Commit 9b06cb3 has been approved by KiChjang

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 25, 2017

⌛️ Testing commit 9b06cb3 with merge 3f0ccd0...

bors-servo added a commit that referenced this pull request Nov 25, 2017

Auto merge of #19358 - jonleighton:issue-19171-3, r=KiChjang
Move selection to end when textarea value is assigned

Issue #19171

<!-- 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/19358)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 25, 2017

@bors-servo bors-servo merged commit 9b06cb3 into servo:master Nov 25, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

jdm added a commit to web-platform-tests/wpt that referenced this pull request Jan 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment