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

Require text entry cursor at the beginning of controls initially #2770

Merged
merged 4 commits into from
Jun 23, 2017

Conversation

domenic
Copy link
Member

@domenic domenic commented Jun 16, 2017

This is a minor point from the larger issue of #2424.

Tests: web-platform-tests/wpt#6265 (everyone passes except Safari fails the last one, possibly because they don't treat type=number as unselectable?) expanded tests reveal more issues; see #2770 (comment) for summary

@bzbarsky and/or @tkent-google to review.

@domenic
Copy link
Member Author

domenic commented Jun 16, 2017

So while writing tests for this, I noticed the problem is what does "initial state" mean. I think it needs to mean before setting defaultValue, not e.g. "before the user touches it".

Setting defaultValue is a whole nother thing, the subject of #2424 (which I am trying to solve, but in small steps).

So maybe this PR should be updated to say something like "on creation"? But... what about elements whose types change? E.g. from something without selection to something with selection. I think we need extra states to reset those to the beginning of selection...

@domenic
Copy link
Member Author

domenic commented Jun 16, 2017

OK @bzbarsky, I'm reasonably happy with this and the accompanying tests. Let me know what you think. I'd ideally like to get this landed before tackling the rest of #2424.

@domenic
Copy link
Member Author

domenic commented Jun 21, 2017

Ping @bzbarsky, this one's pretty small.

source Outdated
@@ -44456,6 +44456,9 @@ interface <dfn>HTMLInputElement</dfn> : <span>HTMLElement</span> {
<li><p>Invoke the <span>value sanitization algorithm</span>, if one is defined for the <code
data-x="attr-input-type">type</code> attribute's new state.</p></li>

<li><p>If the element has a text entry cursor position, move it to the end of the text control,
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. Is that what UAs do?

So in particular, if I create an input with createElement, then set its value attribute, then set its type to "search", should the text entry cursor end up at the end? Testcase:

<input>
<script>
  onload = function() {
    var i = document.querySelector("input");
    i.setAttribute("value", "hey");
    i.setAttribute("type", "search");
    alert(i.selectionStart);
  }
</script>

as I read the spec text this should alert 3, but it seems to alert 0 in at least Firefox and Chrome.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, thanks for catching this. It should be "start". I even wrote tests assuming "start" in web-platform-tests/wpt#6265 (Input switching from unselectable type to selectable type). Although I should also test switching from selectable type to another selectable type.

Copy link
Contributor

@bzbarsky bzbarsky Jun 21, 2017

Choose a reason for hiding this comment

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

I'm not sure that matches browser behavior either, though. Testcase:

<input>
<script>
  onload = function() {
    var i = document.querySelector("input");
    i.setAttribute("value", "hey");
    i.selectionStart = 1;
    i.setAttribute("type", "search");
    alert(i.selectionStart);
  }
</script>

alerts 1 in Chrome, Firefox, Safari. Haven't checked Edge yet.

And this one:

<input>
<script>
  onload = function() {
    var i = document.querySelector("input");
    i.setAttribute("value", "hey");
    i.selectionStart = 1;
    i.setAttribute("type", "checkbox");
    i.setAttribute("type", "search");
    alert(i.selectionStart);
  }
</script>

alerts 0 in Firefox but 1 in Chrome and Safari. Some more tests:

<input>
<script>
  onload = function() {
    var i = document.querySelector("input");
    i.setAttribute("value", "hey");
    i.selectionStart = 3;
    i.setAttribute("type", "checkbox");
    i.setAttribute("value", "ab");
    i.setAttribute("type", "search");
    alert(i.selectionStart);
  }
</script>

0 in Firefox, 3 in Chrome and Safari (yes, out of range). And:

<input>
<script>
  onload = function() {
    var i = document.querySelector("input");
    i.setAttribute("value", "hey");
    i.selectionStart = 3;
    i.setAttribute("value", "ab");
    i.setAttribute("type", "search");
    alert(i.selectionStart);
  }
</script>

2 in Firefox, 3 in Chrome, 0 in Safari. Safari is 0 even without that last "type" change.

For what it's worth, what Firefox does is preserve selection state when switching between selectable types, drop it completely when switching to a non-selectable type, set it to "no selection, cursor at 0" (the initial state) when switching from a non-selectable type to a selectable type.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like Firefox's model, so let me spec that and add lots of tests. Thanks for all the test cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

The last test is out of scope for this PR but I made a specific note about it in #2424 (comment) so hopefully I don't forget about it

@domenic domenic force-pushed the beginning-of-control branch from 836b323 to cd759d8 Compare June 21, 2017 20:21
Copy link
Contributor

@bzbarsky bzbarsky left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

domenic added a commit to web-platform-tests/wpt that referenced this pull request Jun 21, 2017
@domenic
Copy link
Member Author

domenic commented Jun 21, 2017

I guess now that we are no longer just filling a spec hole where everyone already agreed, but actually taking care of an interop issue, I should get sign off from the browsers whose behavior is being made invalid.

Ignoring bugs due to not yet having implemented #1006, http://w3c-test.org/submissions/6265/html/semantics/forms/textfieldselection/selection-after-type-change.html passes in Edge and Gecko, and fails in WebKit and Blink. In particular Blink and WebKit appear to "remember" the selection state even after you switch to type=checkbox, so that when you switch back to type=search, it recalls the old values. (Although they remember in different ways, e.g. Blink lets it be out of bounds if the value has changed in the meantime.) Wherease Edge/Gecko/the proposed spec reset the selection when switching to type=checkbox.

So, @tkent-google, @cdumez, are you OK with this change? I'll assume it's OK unless I hear otherwise soon-ish, since it's a 2/2 split that I doubt people will have strong opinions about.

I'll file appropriate bugs for this whole area on everyone after I also fix #2424.

@domenic domenic added the interop Implementations are not interoperable with each other label Jun 21, 2017
@tkent-google
Copy link
Contributor

I'm OK with the change. It looks reasonable.

@domenic domenic merged commit dd0fb78 into master Jun 23, 2017
@domenic domenic deleted the beginning-of-control branch June 23, 2017 16:34
domenic added a commit to web-platform-tests/wpt that referenced this pull request Jun 23, 2017
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.

3 participants