1337392 - in a pre filled textbox, cursor is behind all text, previously it was on the first position

Status: VERIFIED FIXED

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

Description

[Harka Győző]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160123151951

Steps to reproduce:

Opened our internal web based e-mail service with new firefox...


Actual results:

After upgraded to 51.01 In a prefilled textbox ( a reply ), cursor is behind all text.


Expected results:

Cursor should be on the first position, as it was for years...

Comment 1

[Loic]

It's due to bug 1287655.

Comment 2

[Harka Győző]

Very interesting, on the site cited in that bug
https://www.fxsitecompat.com/en-CA/docs/2016/caret-will-be-placed-at-the-end-when-textbox-automatically-gets-focus/
They say, "they follow the HTML spec". But where is this in the "HTML spec" ? And in which one?

Comment 3

[Loic]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1287655#c0

Comment 4

[Harka Győző]

I think the selection start, selection end is not the same as where is a cursor located by default in textarea. Other browsers still works fine.
So as I can see Firefox solved a problem with selection position return by a new bug - instead to count the text correctly, ff is just placing the cursor at the end by default. Still can't see this (default cursor position) in any specification btw.!

Comment 5

[Leif Velcro]

This has created a problem for us as well, both for our webmail client and for content management system.  Would it be possible to add an option to allow for the old behavior?

Comment 6

[thomas.hebinck]

We also get support requests from our customers. Could you please restore the old behavior? Thanks and bye, Thomas

Comment 7

[Andrew Overholt [:overholt]]

Decky, are you able to comment here? Or maybe smaug who reviewed your patch?

Comment 8

[Petr Hroudný]

This new behaviour is highly problematic for our webmail service, users are complaining why is now cursor placed at the bottom of all email text when replying and why they have to scroll back to beginning each time. In other browsers, the cursor is always at the beginning.

Happens in all Firefox 51 versions (Windows/Linux/Mac).

Please consider reverting this change, it's highly annoying and we're getting lots of complaints.

Thanks,
Petr.

Comment 9

[Loic]

ehsan, as smaug didn't reply to the NI, could you give your opinion about this issue? It seems to affect some users.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(FWIW, this is still in my todo list, but atm there are plenty of review requests to deal with first)

Comment 11

[(no longer active)]

Is there a test case?  As far as I can tell all of the complaints here are about internal webmail services and whatnot, so I'm not sure how I can help without being able to test what the problem is.

Comment 12

[Loic]

(In reply to :Ehsan Akhgari from comment #11)
> Is there a test case?  As far as I can tell all of the complaints here are
> about internal webmail services and whatnot, so I'm not sure how I can help
> without being able to test what the problem is.

Yes, see https://jsfiddle.net/zevp3dpa/1/ from https://bugzilla.mozilla.org/show_bug.cgi?id=1334723#c5

Comment 13

[(no longer active)]

(In reply to Loic from comment #12)
> (In reply to :Ehsan Akhgari from comment #11)
> > Is there a test case?  As far as I can tell all of the complaints here are
> > about internal webmail services and whatnot, so I'm not sure how I can help
> > without being able to test what the problem is.
> 
> Yes, see https://jsfiddle.net/zevp3dpa/1/ from
> https://bugzilla.mozilla.org/show_bug.cgi?id=1334723#c5

I don't understand...  On this test case, both Chrome and Safari put the cursor at the beginning, but on this one (bug 1287655 comment 1) they both put the cursor at the end.  What is the difference between the two?

Comment 14

[(no longer active)]

Huh, it seems like the difference in behavior depends on whether the value of the text control has been changed by script or not.  On this test case we all behave the same way: <https://jsfiddle.net/2fuatdng/>

Comment 16

[(no longer active)]

We should fix this ASAP, this probably occurs on piles on websites.  Andrew, can you please find an owner?

My guess on how to fix this would be to check whether the value has come from the user around here <https://hg.mozilla.org/mozilla-central/rev/52a0d2d76397#l13.20> and decide whether to use 0 or |length| accordingly and then fix all of the tests which will break as a result.

Comment 17

[Andrew Overholt [:overholt]]

Andreas said he could take this.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

farre, if you don't get to this soon, ping me. I could (should?) close my review queue for some time.

Comment 19

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1337392-Only-place-cursor-at-textarea-input-end-.patch

This seems to work, but I haven't checked all test cases changed by Bug 1287655 yet. Some seems to pass and some seems to fail. Fiddles in comments behave as expected though.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7f6291e631797f17870c8576a82fe97a5872487

Comment 20

[Gerry Chang [:gchang]]

Too late for 51 and we've built 52 RC. Mark 51 won't fix.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8842050 [details] [diff] [review]
0001-Bug-1337392-Only-place-cursor-at-textarea-input-end-.patch

We have still some issues dealing with selectionStart/End with display:none elements, but those are separate issues.

Because ValueChanged flag is cleared when reset happens, I thought this would affect to this, but looks like no.

I'd like to see which all tests need to be changed and how.

Comment 22

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1337392-Update-test-cases-to-match-new-cursor-be.patch

First batch of fixed tests. Basically either calling setSelectionRange on input/textarea element before calling focus (on elements with an unchanged value, that is) or changing expected result to have prepended characters instead of appended.

Pushed another try build to look for further errors: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad12dba4d4eeab0a4cc8604799780f278faab922

Comment 23

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1337392-Update-test-cases-to-match-new-cursor-be.patch

Another bunch of failed tests fixed.

Still tests failing on try, but I'm not sure if this fix is to blame. It's a bit messy.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5e0f408773a5e214d22cf11d1df56fad2b9ce23&selectedJob=81444715

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

So some changes are such which bug 1287655 didn't change.
Could you explain those.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8843327 [details] [diff] [review]
0002-Bug-1337392-Update-test-cases-to-match-new-cursor-be.patch

But in general this seems to back out some changes.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Could you perhaps file a followup bug related to selection handling when input uses style="display: none;"

Comment 27

[Boris Zbarsky [:bzbarsky]]

Bug 1343037 is blocked on this, and should address the display:none issues.  The reason it's blocked on this is to avoid backport pain here around the "where is the cursor" behavior in the display:none case.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 updated on top of , I think

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8844607 [details] [diff] [review]
Part 5 updated on top of , I think

bzexport fail....

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa56e40215a6
Only place cursor at textarea/input end if content was changed. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cce6ce016dfa
Update test cases to match new cursor behavior. r=smaug

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fa56e40215a6
https://hg.mozilla.org/mozilla-central/rev/cce6ce016dfa

Comment 32

[Ryan VanderMeulen [:RyanVM]]

(In reply to :Ehsan Akhgari from comment #16)
> We should fix this ASAP, this probably occurs on piles on websites.  Andrew,
> can you please find an owner?

Is this bad enough that we should be considering it for dot release inclusion? Also, I assume we're going to want some level of QA here before requesting branch uplifts?

Comment 33

[Andreas Farre [:farre]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #32)
> (In reply to :Ehsan Akhgari from comment #16)
> > We should fix this ASAP, this probably occurs on piles on websites.  Andrew,
> > can you please find an owner?
> 
> Is this bad enough that we should be considering it for dot release
> inclusion? Also, I assume we're going to want some level of QA here before
> requesting branch uplifts?

Ehsan, did you have numbers or is it a gut estimate?

Comment 34

[(no longer active)]

(In reply to Andreas Farre [:farre] from comment #33)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #32)
> > (In reply to :Ehsan Akhgari from comment #16)
> > > We should fix this ASAP, this probably occurs on piles on websites.  Andrew,
> > > can you please find an owner?
> > 
> > Is this bad enough that we should be considering it for dot release
> > inclusion? Also, I assume we're going to want some level of QA here before
> > requesting branch uplifts?
> 
> Ehsan, did you have numbers or is it a gut estimate?

I don't have numbers, no, but the code pattern in question is extremely common...  In terms of whether we should do a dot release, the frequency of the bug is one thing to consider, the implications are another.  This will appear as annoying issues that affect usability potentially, not crashes or security issues, so I doubt that the severity justifies a dot release on its own.  It's probably worth considering taking as a ride along if we do a dot release separately.  Ultimately of course it's the release manager's call.

About QA, it's certainly worth verifying that the websites this was reported on here and in bug 1334723 aren't affected after the fix.

Comment 35

[Julien Cristau [:jcristau]]

Flagging this for manual verification before potential uplift.

Comment 36

[Gerry Chang [:gchang]]

Track 53+/54+ as regression.

Comment 37

[Andrei Vaida [:avaida]]

(In reply to Julien Cristau [:jcristau] from comment #35)
> Flagging this for manual verification before potential uplift.

Forwarding this to Brindusa, who's usually taking care of bug work requests on the nightly channel.

Comment 38

[ovidiu boca[:Ovidiu]]

I verified this issue on Mac OS X 10.10, Windows 10, Ubuntu 16.04 with FF Nightly 55.0a1(2017-03-14)and I can confirm the fix. Using the test case from comment 12 I see the caret in front of the text, as expected.

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

Andreas, can you request uplift? We can give this a try on beta so that it will be fixed in 53 (current release date will be April 18th. )   If it looks good on beta then we can consider it in case of a 52 dot release too.

Comment 40

[Andreas Farre [:farre]]

Comment on attachment 8842050 [details] [diff] [review]
0001-Bug-1337392-Only-place-cursor-at-textarea-input-end-.patch

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1287655

[User impact if declined]:
Annoying behaviour that potentially affects usability and that is a divergence from other browsers and older versions of gecko. 

[Is this code covered by automated tests?]:
Yes, see patch 0002

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
STR as in comment 12 and bug 1334723

[List of other uplifts needed for the feature/fix]:
Only the patches attached to this bug.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Small change.

[String changes made/needed]:
None

Comment 41

[Andreas Farre [:farre]]

Comment on attachment 8843327 [details] [diff] [review]
0002-Bug-1337392-Update-test-cases-to-match-new-cursor-be.patch

Approval Request Comment
See comment 40

Comment 42

[Andreas Farre [:farre]]

Caveat: this is my very first uplift request! Please tell me if somethings missing!

Comment 43

[Gerry Chang [:gchang]]

Comment on attachment 8842050 [details] [diff] [review]
0001-Bug-1337392-Only-place-cursor-at-textarea-input-end-.patch

Fix a usability regression and was verified. Beta53+ & Aurora54+.

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/196c23408253
https://hg.mozilla.org/releases/mozilla-aurora/rev/e11071d8fb7d

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/505f5940b4a4
https://hg.mozilla.org/releases/mozilla-beta/rev/4a41fe2e0a5a

Comment 46

[Julien Cristau [:jcristau]]

Andreas, care to request uplift to release/esr52?

Comment 47

[Andreas Farre [:farre]]

Comment on attachment 8842050 [details] [diff] [review]
0001-Bug-1337392-Only-place-cursor-at-textarea-input-end-.patch

[Feature/Bug causing the regression]:
Bug 1287655

[User impact if declined]:
Annoying behaviour that potentially affects usability and that is a divergence from other browsers and older versions of gecko.

[Is this code covered by automated tests?]:
Yes, see patch 0002

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
STR as in comment 12 and bug 1334723

[List of other uplifts needed for the feature/fix]:
Only the patches attached to this bug.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Small change.

[String or UUID changes made/needed]:
None

Comment 48

[Andreas Farre [:farre]]

Comment on attachment 8843327 [details] [diff] [review]
0002-Bug-1337392-Update-test-cases-to-match-new-cursor-be.patch

See comment 47.

Comment 49

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 52.0.1 (ID=20170316213829) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 53.0b5, 54.0a2 on Win 8.1 x64 and Mac OS X 10.11 and Ubuntu 16.04 x64.

Comment 50

[Julien Cristau [:jcristau]]

Comment on attachment 8842050 [details] [diff] [review]
0001-Bug-1337392-Only-place-cursor-at-textarea-input-end-.patch

let's get this in esr52 (default branch) at least, I'm still on the fence about dot release inclusion.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/cf8216fc3b25
https://hg.mozilla.org/releases/mozilla-esr52/rev/36953ede0e59

Comment 52

[Julien Cristau [:jcristau]]

Comment on attachment 8842050 [details] [diff] [review]
0001-Bug-1337392-Only-place-cursor-at-textarea-input-end-.patch

I'm going to call 53 and 52.1.0esr good enough.

Comment 53

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out of esr52 for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=85754935&repo=mozilla-esr52

https://hg.mozilla.org/releases/mozilla-esr52/rev/c4f5a1f6d676

Comment 54

[Andreas Farre [:farre]]

(In reply to Wes Kocher (:KWierso) from comment #53)
> I had to back this out of esr52 for failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=85754935&repo=mozilla-
> esr52
> 
> https://hg.mozilla.org/releases/mozilla-esr52/rev/c4f5a1f6d676

Right, those kinds of failures are expected. As far as I can tell, dom/browser-element/mochitest/test_browserElement_inproc_SetInputMethodActive.html isn't on Nightly, so I guess that it got removed somewhere along the line?

Fixing that test case is just a matter of updating expectations.

Comment 55

[Julien Cristau [:jcristau]]

(In reply to Andreas Farre [:farre] from comment #54)
> (In reply to Wes Kocher (:KWierso) from comment #53)
> > I had to back this out of esr52 for failures like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=85754935&repo=mozilla-
> > esr52
> > 
> > https://hg.mozilla.org/releases/mozilla-esr52/rev/c4f5a1f6d676
> 
> Right, those kinds of failures are expected. As far as I can tell,
> dom/browser-element/mochitest/
> test_browserElement_inproc_SetInputMethodActive.html isn't on Nightly, so I
> guess that it got removed somewhere along the line?
> 
> Fixing that test case is just a matter of updating expectations.

Can we get a new patch with expectations fixed up for esr52 then?

Comment 56

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1337392-Fix-esr52-specific-test-cases.-r-baku.patch

There were two test cases that needed fixing in esr52:

devtools/client/debugger/new/test/mochitest/browser_dbg-breakpoints-cond.js
dom/browser-element/mochitest/browserElement_SetInputMethodActive.js

Both were of the sort that they needed their results checked at the front (#0hello, #0#1hello vs hello#0, hello#0#1 and 21 vs 12 respectively).

Comment 57

[Andreas Farre [:farre]]

Comment on attachment 8855750 [details] [diff] [review]
0001-Bug-1337392-Fix-esr52-specific-test-cases.-r-baku.patch

[Approval Request Comment]

If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Needed to fix test cases broken by uplifting 0001-Bug-1337392-Only-place-cursor-at-textarea-input-end-.patch

User impact if declined: 
0001-Bug-1337392-Only-place-cursor-at-textarea-input-end-.patch can't land on esr52.

Fix Landed on Version:
N/A. Needed to land 0001-Bug-1337392-Only-place-cursor-at-textarea-input-end-.patch on esr52

Risk to taking this patch (and alternatives if risky): 
Small, only changes test cases.

String or UUID changes made by this patch: 
None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8855750 [details] [diff] [review]
0001-Bug-1337392-Fix-esr52-specific-test-cases.-r-baku.patch

No need to get approval on test-only changes. Thanks for doing the rebase, we'll get it landed today!

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/4ab6d5c43036
https://hg.mozilla.org/releases/mozilla-esr52/rev/13f2d85da9a7

Comment 60

[Andrei Vaida [:avaida]]

Timea, could you please verify this on 52.1esr as well?

Comment 61

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I can confirm this issue is fixed, I verified using Firefox 52.1esr on Win 8.1 x64 and Mac OS X 10.11 and Ubuntu 14.04 x64.

Comment 62

[Leif Velcro]

Is this supposed to be fixed for everyone in all cases?

I'm still experiencing the problem in Windows 7, using Firefox 54.0 (32-bit).

Comment 63

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

Hi Leif,

I verified on Win 7 x86, Firefox 54.0 build 32 bit (buildID: 20170608105825),with link https://jsfiddle.net/zevp3dpa/1/ (comment 12), the cursor is focused on the first position.
Could you try again and confirm if I am wrong?

Thanks, 
       Timea