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

Fix selection/text entry cursor position processing for input/textarea #3472

Merged
merged 3 commits into from
Mar 16, 2018

Conversation

domenic
Copy link
Member

@domenic domenic commented Feb 17, 2018

This is mainly aimed at fixing #2424. I split it up into three commits: the first is editorial to give concepts some definitions and make them easier to work with; the second fixes a bug in a related area; and the last one contains the important clamping changes discussed there.

Feel free to review only the last two diffs, or only the last one.

/cc @bzbarsky @tkent-google @jonleighton @jdm @emanchado as people involved in the various threads related to this issue.

As discussed in #2424 (comment), I need help writing the web platform tests for this. Also, probably someone should port the example in the spec that I added (for distinguishing raw value vs. API value) to another WPT.


/form-control-infrastructure.html ( diff )
/form-elements.html ( diff )
/input.html ( diff )

@domenic domenic added topic: forms needs tests Moving the issue forward requires someone to write tests labels Feb 17, 2018
@jonleighton
Copy link

Thanks for getting this started. The changes you've made so far make sense to me but per my comment in #3468, I personally think it would make better logical sense for the reset algorithm to set the text entry cursor to the beginning of the text control, remove any selection, and set the selection direction to "none". When the reset algorithm is invoked, the user wants to return the control to its initial state, so IMO it doesn't make sense to retain the existing cursor / selection bounds (which might even relate to a different pre-reset value).

@domenic
Copy link
Member Author

domenic commented Feb 17, 2018

I understand that position, but I don't think we should be inventing new behavior for the reset algorithm; I'd rather it behave the same as other value changes.

Copy link
Contributor

@tkent-google tkent-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@bzbarsky
Copy link
Contributor

The proposed changes look ok to me.

domenic pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 6, 2018
These tests already show differences between eg. Firefox and
Chrome, and show what seems to be browser bugs.

Follows whatwg/html#3472.
@domenic domenic removed the needs tests Moving the issue forward requires someone to write tests label Mar 15, 2018
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the longer conditionals would also be clearer with "then" added, but since this is three commits I didn't go in and make that change.

data-x="">none</code>".</p></li>
element's <span data-x="concept-textarea/input-cursor">text entry cursor position</span> to the
beginning of the text control, and <span data-x="set the selection direction">set its selection
direction</span> to "<code data-x="">none</code>".</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a comma before "and" here? Because it's a slightly longer sentence?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess so. I think without it you might start reading things as "Set the element's text entry cursor position to the (beginning of the text control and X)" before you realize that X is a verb phrase so you have to switch your reading to "(Set the element's text entry cursor position to the beginning of the text control) and (X)".

<dfn data-x="concept-textarea/input-selection">selection</dfn> or a <dfn
data-x="concept-textarea/input-cursor">text entry cursor position</dfn> at all times (even for
elements that are not <span>being rendered</span>). The initial state must consist of a <span
data-x="concept-textarea/input-cursor">text entry cursor</span> at the beginning of the control.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a position not a collapsed selection?

Copy link
Member Author

@domenic domenic Mar 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. That would be a pretty invasive change to make, and I'm not sure of all the implications, given all the other places in the spec that depend on this dual concept. Could be investigated as a follow-up, I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the answer may lie in this (preexisting) clause: "If the user agent does not support empty selection".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just seems weird to me since that's how Selection objects work. (That doesn't seem to allow user agents to not support that either.)

source Outdated
<div class="example">
<p>The use of <span data-x="concept-fe-api-value">API value</span> instead of <span
data-x="concept-textarea-raw-value">raw value</span> for <code>textarea</code> elements means
that U+00D (CR) characters are normalized away. For example,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

U+000D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks!

This gives explicit definitions to the concepts of "selection", "text
entry cursor position", and "relevant value" for selection/cursor
purposes.
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.
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll approve since editorially it's fine and bz said it was fine (presumably for the non-editorial stuff).

It doesn't make sense to me though that we use a fundamentally different model here from the rest of the tree, even though this supposedly happens in user agent shadow trees. Let's file a follow-up?

@domenic
Copy link
Member Author

domenic commented Mar 16, 2018

I'm not sure shadow trees are related, but sure.

@domenic domenic merged commit 3d2719a into master Mar 16, 2018
@domenic domenic deleted the selection-start-end-again branch March 16, 2018 09:20
@annevk
Copy link
Member

annevk commented Mar 16, 2018

@domenic isn't the idea model that <textarea> is just <div contenteditable> with some restrictions hidden via a shadow tree? I thought that's even how it's implemented in some engines.

@domenic
Copy link
Member Author

domenic commented Mar 16, 2018

If so, I was not aware. E.g. you cannot bold the contents of a textarea. It seems more like input than contenteditable to me.

@domenic
Copy link
Member Author

domenic commented Mar 16, 2018

Browser bugs:

I don't have a Mac handy, so if someone could test Safari Tech Preview against http://w3c-test.org/html/semantics/forms/textfieldselection/selection-start-end-extra.html and file a bug if they fail, I'd much appreciated it.

@annevk
Copy link
Member

annevk commented Mar 16, 2018

https://bugs.webkit.org/show_bug.cgi?id=183692

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 1, 2018
…ge selection update, a=testonly

Automatic update from web-platform-testsAdd first batch of tests for selection update

These tests already show differences between eg. Firefox and
Chrome, and show what seems to be browser bugs.

Follows whatwg/html#3472.

wpt-commits: 05d6e35a439546add8dc619c4641ae83551249f8
wpt-pr: 9799
wpt-commits: 05d6e35a439546add8dc619c4641ae83551249f8
wpt-pr: 9799
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 2, 2019
…ge selection update, a=testonly

Automatic update from web-platform-testsAdd first batch of tests for selection update

These tests already show differences between eg. Firefox and
Chrome, and show what seems to be browser bugs.

Follows whatwg/html#3472.

wpt-commits: 05d6e35a439546add8dc619c4641ae83551249f8
wpt-pr: 9799
wpt-commits: 05d6e35a439546add8dc619c4641ae83551249f8
wpt-pr: 9799

UltraBlame original commit: ab846aeed267cce05d8f796111c0f70d6a01c4a7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 2, 2019
…ge selection update, a=testonly

Automatic update from web-platform-testsAdd first batch of tests for selection update

These tests already show differences between eg. Firefox and
Chrome, and show what seems to be browser bugs.

Follows whatwg/html#3472.

wpt-commits: 05d6e35a439546add8dc619c4641ae83551249f8
wpt-pr: 9799
wpt-commits: 05d6e35a439546add8dc619c4641ae83551249f8
wpt-pr: 9799

UltraBlame original commit: ab846aeed267cce05d8f796111c0f70d6a01c4a7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 2, 2019
…ge selection update, a=testonly

Automatic update from web-platform-testsAdd first batch of tests for selection update

These tests already show differences between eg. Firefox and
Chrome, and show what seems to be browser bugs.

Follows whatwg/html#3472.

wpt-commits: 05d6e35a439546add8dc619c4641ae83551249f8
wpt-pr: 9799
wpt-commits: 05d6e35a439546add8dc619c4641ae83551249f8
wpt-pr: 9799

UltraBlame original commit: ab846aeed267cce05d8f796111c0f70d6a01c4a7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants