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

Text control selections API conformance #19171

Closed
KiChjang opened this Issue Nov 10, 2017 · 13 comments

Comments

Projects
None yet
4 participants
@KiChjang
Member

KiChjang commented Nov 10, 2017

We currently have WebIDL bindings to the selectionStart, selectionEnd and selectionRange setter functions on the HTMLInputElement, but these setters do not follow the spec completely since they do not fire select events on the input element when they are called.

In addition, we should also check that the code for these setters are matching the steps as outlined in the spec.

Code: components/script/dom/htmlinputelement.rs
Spec: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#dom-textarea%2Finput-selectionstart
Test: tests/wpt/web-platform-tests/html/semantics/forms/textfieldselection/selection-start-end.html

EDIT: Since @jonleighton has taken this up and focused their efforts on ensuring conformance of both HTMLInputElement and HTMLTextAreaElement selections API, the scope of this issue has been expanded accordingly.

@highfive

This comment has been minimized.

highfive commented Nov 10, 2017

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@tigercosmos

This comment has been minimized.

Collaborator

tigercosmos commented Nov 10, 2017

Did the previous PR solve part of this?

@KiChjang

This comment has been minimized.

Member

KiChjang commented Nov 10, 2017

No, it did not.

@KiChjang KiChjang added I-enhancement and removed I-wrong labels Nov 10, 2017

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 13, 2017

Hi, I'd like to have a go at working on this. I've not contributed to servo before so please bear with me...

I've established that the select event is already being fired by setSelectionRange, since that was implemented in f60de52. However it is not fired by the selectionStart or selectionEnd setters.

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 13, 2017

PR submitted for firing the event. Once it is approved I could look into other related issues (e.g. the remaining failing tests, also the spec says that the event should only fire if the selection has actually changed).

jonleighton added a commit to jonleighton/servo that referenced this issue Nov 15, 2017

bors-servo added a commit that referenced this issue Nov 15, 2017

Auto merge of #19202 - jonleighton:issue-19171, r=KiChjang
Fire 'select' event in SetSelection{Start,End}

See #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/19202)
<!-- Reviewable:end -->
@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 17, 2017

I think this assertion (and the other related assertions in other tests) is not correct per the spec.

The spec says:

If there is no selection, return the offset (in logical order) to the character that immediately follows the text entry cursor.

Further down, it then says:

All elements to which this API applies have either a selection or a text entry cursor position at all times (even for elements that are not being rendered). The initial state must consist of a text entry cursor at the beginning of the control.

The assertion in the test is checking that the text entry cursor is at the end of the control, however.

I made a quick test to check the behaviour in other browsers. Both Firefox and Chromium print 0 for all values in that test page.

So I think the test needs to change, but I'm just checking to see if I have missed something?

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Nov 17, 2017

Sorry, I'm incorrect because the value is set via JS, therefore step 5 from here applies:

If the element's value (after applying the value sanitization algorithm) is different from oldValue, and the element has a text entry cursor position, move the text entry cursor position to the end of the text control, unselecting any selected text and resetting the selection direction to "none".

bors-servo added a commit that referenced this issue Nov 21, 2017

Auto merge of #19272 - jonleighton:issue-19171-2, r=jdm
Further changes in relation to #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/19272)
<!-- Reviewable:end -->

@KiChjang KiChjang added the C-assigned label Nov 21, 2017

jonleighton added a commit to jonleighton/servo that referenced this issue Nov 23, 2017

bors-servo added a commit that referenced this issue 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 -->

jonleighton added a commit to jonleighton/servo that referenced this issue Nov 25, 2017

bors-servo added a commit that referenced this issue 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 -->

@KiChjang KiChjang changed the title from Fire select event when setting selection{Start, End, Range} on HTMLInputElement to Text control selections API conformance Nov 27, 2017

@KiChjang KiChjang removed the E-easy label Nov 27, 2017

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Dec 3, 2017

Latest PR: #19461 (I forgot to ref this issue there)

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Dec 5, 2017

@highfive: assign me

@highfive

This comment has been minimized.

highfive commented Dec 5, 2017

It looks like this has already been assigned to someone. I'll leave the decision to a core contributor.

jonleighton added a commit to jonleighton/servo that referenced this issue Dec 8, 2017

jonleighton added a commit to jonleighton/servo that referenced this issue Dec 11, 2017

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Dec 11, 2017

Once #19544 is merged, the only remaining thing that I'm aware of is steps 7-9 of the input type change algorithm. I can't see any existing WPT tests for that so will need to add some I think.

@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Dec 11, 2017

Actually, I have just added another commit on that PR which deals with the above, so I think we can close this issue once the PR is merged.

jonleighton added a commit to jonleighton/servo that referenced this issue Dec 15, 2017

jonleighton added a commit to jonleighton/servo that referenced this issue Jan 18, 2018

bors-servo added a commit that referenced this issue Jan 26, 2018

Auto merge of #19544 - jonleighton:issue-19171-5, r=nox
Text selection API conformance

This is my next batch of changes for issue #19171. All the tests in tests/wpt/metadata/html/semantics/forms/textfieldselection/ are now passing (and also some tests outside of there).

I've made detailed notes about the changes in each commit message.

r? @KiChjang

<!-- 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/19544)
<!-- Reviewable:end -->

jonleighton added a commit to jonleighton/servo that referenced this issue Jan 26, 2018

bors-servo added a commit that referenced this issue Jan 26, 2018

Auto merge of #19544 - jonleighton:issue-19171-5, r=nox
Text selection API conformance

This is my next batch of changes for issue #19171. All the tests in tests/wpt/metadata/html/semantics/forms/textfieldselection/ are now passing (and also some tests outside of there).

I've made detailed notes about the changes in each commit message.

r? @KiChjang

<!-- 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/19544)
<!-- Reviewable:end -->
@jonleighton

This comment has been minimized.

Contributor

jonleighton commented Jan 28, 2018

@KiChjang this can be closed now!

@KiChjang KiChjang closed this Jan 28, 2018

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