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

Do not change input/textarea text selection for the same value #2437

Merged
merged 1 commit into from
Apr 17, 2017

Conversation

domenic
Copy link
Member

@domenic domenic commented Mar 14, 2017

Fixes #2412.

This also makes slight editorial changes to nearby setter/getter pairs to separate their steps and stop saying "it must".

Tests: web-platform-tests/wpt#5147

/cc @bzbarsky @tkent-google @rniwa

source Outdated
must set the element's <code data-x="attr-input-value">value</code> attribute to the new
value.</p>
<p>On getting, if the element has a <code data-x="attr-input-value">value</code> content
attribute, return that attribute's value; otherwise, it return the empty string.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

s/it return/return/?

source Outdated
perform the following steps:</p>

<ol>
<li><p>Let <var>oldValue</var> be this element's <span
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be value, raw value, or API value?

Why did we pick value?

Copy link
Member Author

Choose a reason for hiding this comment

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

value and API value seem to be equivalent as both normalize line endings in some way. I went with value because it matched the corresponding text for input. And I picked between those two since it seemed like changing the line endings should not impact the selection since it's not observable during submission or through the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I recall correctly, API value is the one that is reasonably cheap to produce, while "value" is the one that takes a bunch of processing. But I could be getting them backwards! This is for textarea; for input the two are identical, iirc... It's really worth checking what UAs actually do here.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the purposes of comparing old and new, API value and value will be identical. Raw value will have unnormalized line endings. Chrome at least does not treat \r\n different from \n when determining whether to reset the selection.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the purposes of comparing old and new, API value and value will be identical.

I'm not sure I follow. value is affected by the wrap attribute; API value is not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and both oldValue and the new value will be affected in the same way, if they are identical.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but if they are not, the value can still end up the same even if the API value is different.

@bzbarsky
Copy link
Contributor

This would require every single browser to change behavior, right? And they're all interoperable right now? I'm somewhat worried about how web-compatible this change will be....

@domenic
Copy link
Member Author

domenic commented Mar 15, 2017

This would require every single browser to change behavior, right? And they're all interoperable right now? I'm somewhat worried about how web-compatible this change will be....

No, they're not really interoperable, according to the tests at http://w3c-test.org/submissions/5147/html/semantics/forms/textfieldselection/selection-after-content-change.html. From what I see in the tests:

  • Firefox doesn't appear to have any no-op behavior.
  • Edge has no-op behavior for inputs while in document; other scenarios are harder to test since selectionStart/selectionEnd/selectionDirection are not set correctly for out of document and selectionDirection is undefined on textareas.
  • Chrome has no-op behavior for textarea (in and out of document), not input; however Chrome also fails the part of the spec that requires reset to go to none (it chooses forward instead).

@bzbarsky
Copy link
Contributor

No, they're not really interoperable

OK, correction. I thought things were interoperable in terms of cursor position behavior for setting .value when an input actually has focus (or possibly just has a CSS box). I agree that the out-of-document cases are all sorts of weird. For one thing, they're incredibly buggy in Firefox unless you're testing a current nightly.

One other note on the tests: they assert that selection direction becomes "none" in various cases, but it's not clear to me that they can in fact do that. That's because the platform may not support selections with direction "none" at all. See the bits in https://html.spec.whatwg.org/multipage/forms.html#set-the-selection-range

It looks like some other parts of the spec (e.g. the value setter) explicitly set the selection direction to "none", but if the platform doesn't support that value it's really just not clear what it means to do that.

@domenic
Copy link
Member Author

domenic commented Mar 15, 2017

OK, correction. I thought things were interoperable in terms of cursor position behavior for setting .value when an input actually has focus (or possibly just has a CSS box).

I'll add focus to the test matrix, although I think it'd be ideal if we didn't branch on that. Certainly if an element has a CSS box there is no interop, e.g. Chrome has no-op behavior for textarea whereas Firefox does not (Edge might or might not). But I guess if testing reveals nobody has no-op behavior for focused inputs we should probably code that in.

One other note on the tests: they assert that selection direction becomes "none" in various cases, but it's not clear to me that they can in fact do that.

Oh dear, good point, hmm. For the tests, I'll loosen it to allow either forward or none. I guess the spec will need some kind of generic set selection direction thingy.

One weird thing is that Chrome sets a selection direction of none for input, but not textarea (on Windows). Is that reasonable, or should we disallow that? @tkent-google

domenic added a commit that referenced this pull request Mar 15, 2017
This fixes the issue noted in
#2437 (comment) where
the spec sometimes assumed it was OK to set the selection direction to
none, even though not all platforms support this. It introduces a new
concept, "set the selection direction", which will convert "none" to
"forward" as appropriate.

This commit also makes a couple editorial tweaks:

* Replaces the italicized selection direction states with simple
  strings, which allows us to avoid a switch statement to translate
  between them.
* Splits out getter and setters in <input> and <textarea>'s value IDL
  attribute definitions, and gives the setters a proper numbered list.
@domenic domenic force-pushed the do-not-change-same-value-selection branch from ef56a40 to 7c751c9 Compare March 15, 2017 23:00
@domenic
Copy link
Member Author

domenic commented Mar 15, 2017

I've updated this PR to be on top of #2439, which should make the important changes clearer if you just look at the second commit. I've also updated the tests. There doesn't seem to be a lot of interop around the failing test cases---even when different browsers fail the same test, they often do so in different ways. So I think the plan in this PR is still pretty good.

domenic added a commit that referenced this pull request Mar 20, 2017
This fixes the issue noted in
#2437 (comment) where
the spec sometimes assumed it was OK to set the selection direction to
none, even though not all platforms support this. It introduces a new
concept, "set the selection direction", which will convert "none" to
"forward" as appropriate.

This commit also makes a couple editorial tweaks:

* Replaces the italicized selection direction states with simple
  strings, which allows us to avoid a switch statement to translate
  between them.
* Splits out getter and setters in <input> and <textarea>'s value IDL
  attribute definitions, and gives the setters a proper numbered list.
foolip pushed a commit that referenced this pull request Mar 21, 2017
This fixes the issue noted in
#2437 (comment) where
the spec sometimes assumed it was OK to set the selection direction to
none, even though not all platforms support this. It introduces a new
concept, "set the selection direction", which will convert "none" to
"forward" as appropriate.

This commit also makes a couple editorial tweaks:

* Replaces the italicized selection direction states with simple
  strings, which allows us to avoid a switch statement to translate
  between them.
* Splits out getter and setters in <input> and <textarea>'s value IDL
  attribute definitions, and gives the setters a proper numbered list.
@tkent-google
Copy link
Contributor

One weird thing is that Chrome sets a selection direction of none for input, but not textarea (on Windows). Is that reasonable, or should we disallow that? @tkent-google

Definitely we shouldn't set "none" on platforms without "none".

@domenic domenic force-pushed the do-not-change-same-value-selection branch from 7c751c9 to ced0b03 Compare March 23, 2017 09:30
@domenic
Copy link
Member Author

domenic commented Mar 23, 2017

@bzbarsky are you still worried about compat for this, given the test results? As I said, there doesn't seem to be a lot of interop around the failing test cases seen in web-platform-tests/wpt#5147, so I am not worried myself.

@tkent-google would you mind doing a review to make sure this matches your intent when proposing #2412? You may also want to review web-platform-tests/wpt#5147.

@bzbarsky
Copy link
Contributor

bzbarsky commented Mar 23, 2017

Yeah, I'm still somewhat worried about compat. I'm not an expert on this stuff, though. @smaug---- and @masayuki-nakano should probably have a look at this.

@masayuki-nakano
Copy link

I have no idea what accident could be caused by the behavior.

I like doing nothing if setting value is same as current value better because some web apps may do such redundant thing for keeping their implementation simpler but it might cause odd undo transaction or killing some IME feature. For example, that may cause browser notifying IME of text change and selection change of focused editor. Then, IME may discard the last commit information for undoing.

And I have a question, if setting same value should do nothing, should active composition not be committed if there is? Firefox ignores setting same value when there is composition, to protect the composition:
http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/dom/html/nsTextEditorState.cpp#2387-2391,2402-2411

@tkent-google
Copy link
Contributor

@tkent-google would you mind doing a review to make sure this matches your intent when proposing #2412? You may also want to review web-platform-tests/wpt#5147.

The spec change lgtm.

@domenic domenic merged commit 8cbd605 into master Apr 17, 2017
@domenic domenic deleted the do-not-change-same-value-selection branch April 17, 2017 21:43
domenic added a commit to web-platform-tests/wpt that referenced this pull request Apr 17, 2017
@domenic domenic added interop Implementations are not interoperable with each other topic: forms normative change labels Apr 17, 2017
@bzbarsky
Copy link
Contributor

@domenic
Copy link
Member Author

domenic commented Apr 17, 2017

@bzbarsky
Copy link
Contributor

Ah, I think we basically mid-aired on filing this. ;)

@bzbarsky
Copy link
Contributor

@domenic I tried implementing it and ran into something that's unclear. See web-platform-tests/wpt#5147 (comment)

domenic added a commit that referenced this pull request Apr 19, 2017
This is a follow-up to #2437, which introduced the value comparison and
its impact on selection. See
web-platform-tests/wpt#5147 (comment).
domenic added a commit that referenced this pull request Apr 20, 2017
This is a follow-up to #2437, which introduced the value comparison and
its impact on selection. See
web-platform-tests/wpt#5147 (comment).
inikulin pushed a commit to HTMLParseErrorWG/html that referenced this pull request May 9, 2017
This is a follow-up to whatwg#2437, which introduced the value comparison and
its impact on selection. See
web-platform-tests/wpt#5147 (comment).
inikulin pushed a commit to HTMLParseErrorWG/html that referenced this pull request May 9, 2017
This is a follow-up to whatwg#2437, which introduced the value comparison and
its impact on selection. See
web-platform-tests/wpt#5147 (comment).
domenic added a commit that referenced this pull request Feb 17, 2018
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.
domenic added a commit that referenced this pull request Mar 16, 2018
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.
domenic added a commit that referenced this pull request Mar 16, 2018
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.
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
This fixes the issue noted in
whatwg#2437 (comment) where
the spec sometimes assumed it was OK to set the selection direction to
none, even though not all platforms support this. It introduces a new
concept, "set the selection direction", which will convert "none" to
"forward" as appropriate.

This commit also makes a couple editorial tweaks:

* Replaces the italicized selection direction states with simple
  strings, which allows us to avoid a switch statement to translate
  between them.
* Splits out getter and setters in <input> and <textarea>'s value IDL
  attribute definitions, and gives the setters a proper numbered list.
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
This is a follow-up to whatwg#2437, which introduced the value comparison and
its impact on selection. See
web-platform-tests/wpt#5147 (comment).
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
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
whatwg#2424, and also in
whatwg#2437.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interop Implementations are not interoperable with each other normative change topic: forms
Development

Successfully merging this pull request may close these issues.

4 participants