Closed Bug 1357206 Opened 3 years ago Closed 2 years ago

Don't move selection to end when .value is set to its existing value

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(5 files, 3 obsolete files)

1.89 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
3.66 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
7.03 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
1.26 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
11.04 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
This is a recent spec change.  See https://github.com/whatwg/html/issues/2412 and https://github.com/whatwg/html/pull/2437

There are wpts at https://github.com/w3c/web-platform-tests/pull/5147 which is not in our tree yet, but http://w3c-test.org/html/semantics/forms/textfieldselection/selection-after-content-change.html can already be used to test the new behavior.

This shouldn't be too hard to deal with, I'd think... at least in the non-editor cases.  And I think the editor case already has the newly specced behavior, mostly, except maybe for the CRLF bits.
Flags: needinfo?(bzbarsky)
Duplicate of this bug: 1357204
Unfortunately, doing this without it turning into a performance disaster is ... complicated.  :(  Textarea already has comments about the horrible performance of having to get the old API value on sets; having to get the new one as well would not help that at all.

And input normally doesn't get either value, of course.

I'll think about this a bit, but it's a pretty low priority given the performance implications.
I guess if the "same value" check is done post-sanitization-algorithm this is not too bad.  The spec isn't quite clear on whether it is, though...  Waiting for a response to my question about that.
OK, I have patches for this, but they lead to all sorts of failures in accessibility, devtools, and jetpack tests.  The a11y and devtools failures are definitely due to the cursor not being where the tests expect it to be; the jetpack failures are as usual inscrutable.

I did debug the devtools failures, and they are due to a behavior change that will affect any React-based app, as far as I can tell.  I reported this in the spec issue at https://github.com/whatwg/html/issues/2412#issuecomment-295575211

I'll attach the patches for posterity, but as I said in the github issue I'm pretty worried about the web compat fallout here.
Flags: needinfo?(bzbarsky)
Otherwise we end up calling into editor code and doing a bunch of work even though the value hasn't actually changed, when a value with \r in it is set repeatedly.
At this point both calls happen unconditionally.  This also removes a search for
\r which the callee will perform anyway.
Attachment #8859850 - Attachment is obsolete: true
Attachment #8859849 - Attachment is obsolete: true
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(ehsan)
Attachment #8859846 - Flags: review?(ehsan)
Attachment #8859847 - Flags: review?(ehsan)
Attachment #8859848 - Flags: review?(ehsan)
Attachment #8859870 - Flags: review?(ehsan)
Attachment #8860263 - Flags: review?(ehsan)
Hmm...  Do you know what the motivation behind this spec change is?  Reading https://github.com/whatwg/html/pull/2437 it seems like the motivation is to align the behavior of all engines around the various early bailouts that Chrome and Edge have here.  I don't really see any reason to believe that these changes are going to be Web compatible.

The last time we changed the placement of the cursor in inputs/textareas which was recently we discovered bug 1334723.  There have been other examples like this which we haven't discovered before shipping IIRC.  I am really not in a rush to do this once again, especially given that now our train cycles are shorter so the likelihood of us finding regressions like this is even smaller.

I don't know what to do here.  I'm inclined to say we should wait it out for Chrome and Edge on this one for them both to ship this and deal with the Web compat fallout.  Given that they are the engines with the early bailouts we are at a disadvantage in terms of the likelihood of Web compat issues biting us comparatively, I think.  :-(

What do you think?
Flags: needinfo?(ehsan) → needinfo?(bzbarsky)
> Do you know what the motivation behind this spec change is?

All the info I have is in https://github.com/whatwg/html/issues/2412

In brief, this is not a behavior change for <textarea> in any browsers, assuming you do the value set "late enough" in Firefox (after we set up the editor on the textarea; if you do it before we manage to do that, i.e. before we have a frame, we go through the text editor state and do end up moving the cursor).

It's not a behavior change for us for <input> if you do it on an <input> that has an editor set up already.

So I think the main motivation is to align the spec with browsers for <textarea> and to align <input> with textarea for the browsers where it doesn't match already.  That's Safari, Chrome, and us in the "no editor" case.

Oh, and in terms of this being observable...  In Firefox 52, we set up an editor any time setSelectionRange is called, or selectionStart/End are manipulated.  That didn't change until Firefox 55, in bug 1343037.  So the only way to observe the different behavior is to have an <input> that the user never touched, that script never manipulated selection in, and that script sets .value = .value on.  In Firefox 53, that moves the cursor to the end; in Firefox 55 it would leave it at the beginning.  Note that in Firefox 52 the cursor is at the end whether .value is set or not; that's bug 1337392, of course, and we fixed that for 53.

One way of thinking about our current behavior is that we still have bug 1337392 for any page where React is involved, because React does the .value = .value thing when it binds to text controls.  And that this patch is fixing that case...  ;)

> I'm inclined to say we should wait it out for Chrome and Edge on this one for them both to ship this

According to https://github.com/whatwg/html/issues/2412#issue-212041720 Edge already has the behavior the spec changed to.  I just did some testing, and that looks correct to me.  So Edge is already shipping this and has been all along.  Did you mean WebKit when you said Edge?

Anyway, I can probably wait on this stuff until Chrome ships, if you still think the risk is too high after reading the above.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(ehsan)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #13)
> > Do you know what the motivation behind this spec change is?
> 
> All the info I have is in https://github.com/whatwg/html/issues/2412
> 
> In brief, this is not a behavior change for <textarea> in any browsers,
> assuming you do the value set "late enough" in Firefox (after we set up the
> editor on the textarea; if you do it before we manage to do that, i.e.
> before we have a frame, we go through the text editor state and do end up
> moving the cursor).

I don't see how we'd have a cursor without a frame here, so perhaps textareas aren't of concern here.

> It's not a behavior change for us for <input> if you do it on an <input>
> that has an editor set up already.

That's definitely of huge concern for us.  <input> elements are lazily initialized as you know.

> So I think the main motivation is to align the spec with browsers for
> <textarea> and to align <input> with textarea for the browsers where it
> doesn't match already.  That's Safari, Chrome, and us in the "no editor"
> case.
> 
> Oh, and in terms of this being observable...  In Firefox 52, we set up an
> editor any time setSelectionRange is called, or selectionStart/End are
> manipulated.  That didn't change until Firefox 55, in bug 1343037.  So the
> only way to observe the different behavior is to have an <input> that the
> user never touched, that script never manipulated selection in, and that
> script sets .value = .value on.  In Firefox 53, that moves the cursor to the
> end; in Firefox 55 it would leave it at the beginning. 

Well, OK, so given that most <input>s will not be touched by the user and script will not manipulate the selection in, the risk is for <input>s where the script is setting .value = .value on.  And of course I'm not really worried about something as simple as |i.value = i.value;| but I'm more worried about:

function enterTheAbyss(xxx) {
  // stuff...
  i.value = data;
}

var data = i.value;
enterTheAbyss(data);

That... sounds plausible, and even likely given libraries, abstractions and whatnot, to happen in real life.  :-(

> Note that in Firefox
> 52 the cursor is at the end whether .value is set or not; that's bug
> 1337392, of course, and we fixed that for 53.

Yep.

> One way of thinking about our current behavior is that we still have bug
> 1337392 for any page where React is involved, because React does the .value
> = .value thing when it binds to text controls.  And that this patch is
> fixing that case...  ;)

That's a good point.

> > I'm inclined to say we should wait it out for Chrome and Edge on this one for them both to ship this
> 
> According to https://github.com/whatwg/html/issues/2412#issue-212041720 Edge
> already has the behavior the spec changed to.  I just did some testing, and
> that looks correct to me.  So Edge is already shipping this and has been all
> along.  Did you mean WebKit when you said Edge?

Yes, sorry.

> Anyway, I can probably wait on this stuff until Chrome ships, if you still
> think the risk is too high after reading the above.

This is a mess.  I don't really know how to evaluate the severity of the risk besides guessing.  Your point above about pages with React is I think a good argument in favor of making this change.  But we should be prepared to deal with fallout.  This time we should also tell the web compat folks to watch out beforehand!  ni? as a proxy.

I'll review your patches tomorrow (apologies for the extended delay!) and I think I'm probably going to ask for a pref in case we need to deal with a disaster quickly here.  But I guess I don't have any concrete reasons to ask you to stop.  Sorry for the hesitation!
Flags: needinfo?(ehsan) → needinfo?(miket)
> I don't see how we'd have a cursor without a frame here

Well, we always have a selectionStart and selectionEnd.  "cursor" just means "selectionStart == selectionEnd".  The concept exists independent of whether we have a frame.

> That's definitely of huge concern for us.  <input> elements are lazily initialized as you know.

Yes, I very much know.

I can put this behind a pref, sure.  That seems fairly prudent.
Attachment #8860263 - Attachment is obsolete: true
Attachment #8860263 - Flags: review?(ehsan)
Attachment #8859846 - Flags: review?(ehsan) → review+
Attachment #8859847 - Flags: review?(ehsan) → review+
Attachment #8859848 - Flags: review?(ehsan) → review+
Comment on attachment 8862246 [details] [diff] [review]
Part 4 with a pref to disable

Review of attachment 8862246 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints-cond.js
@@ +13,5 @@
>      selectMenuItem(dbg, 2);
>      yield waitForElement(dbg, ".conditional-breakpoint-panel input");
>      findElementWithSelector(dbg, ".conditional-breakpoint-panel input").focus();
> +    // Position cursor reliably at the end of the text.
> +    pressKey(dbg, "End")

It seems like you probably want to modify these tests to also set the pref, no?

::: dom/html/nsTextEditorState.h
@@ +287,5 @@
>        bool IsDirty() const
>        {
>          return mIsDirty;
>        }
> +      void SetIsDirty() {

Nit: open brace goes on its own line.
Attachment #8862246 - Flags: review?(ehsan) → review+
Comment on attachment 8859870 [details] [diff] [review]
part 5.  When moving the cursor to the end of text on value set, reset the selection direction as well, per spec

Review of attachment 8859870 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, what caught this?  Can we test it?
Attachment #8859870 - Flags: review?(ehsan) → review+
> It seems like you probably want to modify these tests to also set the pref, no?

Why?  The current behavior is to put the cursor at the end; the new behavior is to not do that for the debugger case, but then the bit I added moves it to the end.  Either way, the cursor ends up at the end.

> Hmm, what caught this? 

The web platform test linked from comment 0.

> Can we test it?

Next time jgraham syncs wpt that will happen automatically.  I could cherry-pick those changes over if you prefer, of course, if we're worried about regressing between now and then.
Anyway, I'm going to commit this for now, but please let me know if you do want me to grab the wpt from upstream now as opposed to waiting for the sync.
Flags: needinfo?(ehsan)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8248f3e9f845
part 1.  Move conversion of the new textarea/input value to DOM linebreaks to before we check whether the new value matches the old value.  r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/81b593baf39d
part 2.  Common up the PlatformToDOMLineBreaks calls for the have-editor and do-not-have-editor cases in nsTextEditorState::SetValue.  r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/77a461be367e
part 3.  Rename eSetValue_MoveCursorToEnd to eSetValue_MoveCursorToEndIfValueChanged, because those are the semantics we want for it.  r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/c966ab0cfbb7
part 4.  Don't move the cursor even if eSetValue_MoveCursorToEndIfValueChanged is set, if the value did not change.  r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1d90267f5ed
part 5.  When moving the cursor to the end of text on value set, reset the selection direction as well, per spec.  r=ehsan
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #20)
> Anyway, I'm going to commit this for now, but please let me know if you do
> want me to grab the wpt from upstream now as opposed to waiting for the sync.

I think waiting for the next update would be ok.
Flags: needinfo?(ehsan)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/492530a9b80a
part 6. Disable new behavior until devtools tests are fixed. r=bustage-fix CLOSED TREE
Looks like on Windows only, "Ctrl+right" doesn't go to end when triggered via this codepath, even though it does if you actually press those keys...

I guess I get to experiment with what Windows will accept here in terms of key events.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b7985a96a5e
followup: fix the Windows key issues in devtools tests and reenable the new behavior.
(emailed the compat team/list as a heads up, thx!)
Flags: needinfo?(miket)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.