353539 - textarea.focus() puts caret at end without scrolling it into view

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Load the testcase.
2. Click the button labeled "Focus the textarea".
3. Wonder whether anything happened.
4. Press an arrow key.
5. Realize there was a blinking insertion point at the end of the textarea, where it was invisible because it was scrolled off.

Comment 1

[Jesse Ruderman]

Attachment: testcase (see steps in comment 0)

Comment 2

[Jesse Ruderman]

See also bug 232405, "textbox/textarea doesn't scroll if I change cursor position with setSelectionRange, selectionStart, etc."

Comment 3

[Jesse Ruderman]

See also bug 64170, same bug for textboxes.

Comment 4

[Priit Laes]

Attachment: mozilla-input-selection-scroll-on-focus.patch

Patch taken from bug 231389.
Seems to fix bug 353539, bug 232405 and bug 231389.

Comment 5

[Priit Laes]

It should be noted that also happens on Win/Linux amd64/x86

Comment 6

[Boris Zbarsky [:bzbarsky]]

I think you want to do async scroll here, unless there's a very strong reason to do sync.  The sync scroll won't flush out layout before scrolling, so if the content gets updated but layout hasn't changed yet (e.g. if we make setting .value not flush layout) the scroll attempt could fail (because there's nothing to scroll to yet).

I wonder whether it's possible to write a test for this, actually...

I can't think of anything offhand that would break with an async scroll.

Comment 7

[Priit Laes]

Attachment: mozilla-input-selection-scroll-on-focus.patch

Here comes the asynchronous version.

It doesn't seem that it fully fixes the current issue... (check the testcase attached to this bug), but it at least makes testcases from bug 232405 and bug 231389 working...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm. That's very odd.  I would expect it to work just fine on the testcase in this bug...

Comment 9

[Priit Laes]

Any chance it gets approval? It at least fixes some of the major issues..

Comment 10

[Boris Zbarsky [:bzbarsky]]

Well.  If it doesn't fix this bug, then it's kind of wrong, no?  Can you tell what's going on?

Comment 11

[Priit Laes]

Well, it fixes the other two bugs ;)

Comment 12

[Boris Zbarsky [:bzbarsky]]

It's attached to this bug, though.  And the point is, if it doesn't fix this bug something is wrong.  Which means it's likely to break other things.  And at this point in the cycle we'd rather let old bugs be than create new ones.

If you don't plan to debug what's going wrong with the latest patch, please let me know and I'll try to find time to, I guess...

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK, over here neither of the two patches fixes this bug.  This is no surprising: the focus() call doesn't even end up in SetSelectionInternal().

Sounds like the second patch should be placed on a bug it does in fact fix (at which point we should indeed review it and check it in), and we still need a fix for this bug.

Comment 14

[Priit Laes]

Comment on attachment 288837 [details] [diff] [review]
mozilla-input-selection-scroll-on-focus.patch

Nice catch, so marking this one obsolete ;)

Comment 15

[(no longer active)]

Attachment: Patch (v1)

Patch + test

Comment 16

[(no longer active)]

Comment on attachment 432861 [details] [diff] [review]
Patch (v1)

bz's out, switching the review request to roc.

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b13195a7fb81

Comment 18

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381

Comment 19

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381