612129 - focus() should place caret at beginning instead of end (to avoid scroll-into-view jumpiness and match other browsers)

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Eric]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101114 Firefox/4.0b8pre Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101114 Firefox/4.0b8pre Firefox/4.0b8pre

When you switch between the templates in the template editor in the latest IPB Admin Control Panel, the cursor will jump around, usually to the very bottom of the template, if it is very long. This is not an issue with IPB, as it works perfectly in other browsers.

Reproducible: Always

Steps to Reproduce:
1. Click on a skin in the Look & Feel tab
2. Click on a template, for instance the 'globalTemplate' template.
3. Click on a different template, then open the globalTemplate back up.
Actual Results:  
When you load the globalTemplate, it will briefly show the top of the text area, then the cursor will jump all the way to the bottom.

When you load it again, for instance after looking at another template, it will repeat the same thing and jump back to the bottom.

Expected Results:  
When you load the globalTemplate, the cursor should remain at the top/very beginning of the text area, or wherever the cursor is in the text area. When switching between different templates, it should be smooth and not jump around.

Since this is in an Admin Control Panel, I know of no way I can give a demo link, moreso here in public. I know it's not only me, as people I work with have the same issue as I when they use Firefox in the template editors. To help show the issue, I have made a video that shows a comparison between Firefox and Chrome when doing this (I would recommend full screening it, and I understand it may be a bit difficult to understand, but I hope my previous descriptions will make it easier):

http://s221.photobucket.com/albums/dd314/erictaytay/?action=view&current=clip0010.mp4

If there's any more I can do to help you reproduce it on your end, I will.

Comment 1

[David Mandelin [:dmandelin]]

(In reply to comment #0)
> Since this is in an Admin Control Panel, I know of no way I can give a demo
> link, moreso here in public. I know it's not only me, as people I work with
> have the same issue as I when they use Firefox in the template editors. To help
> show the issue, I have made a video that shows a comparison between Firefox and
> Chrome when doing this (I would recommend full screening it, and I understand
> it may be a bit difficult to understand, but I hope my previous descriptions
> will make it easier):
> 
> http://s221.photobucket.com/albums/dd314/erictaytay/?action=view&current=clip0010.mp4
> 
> If there's any more I can do to help you reproduce it on your end, I will.

Is this a regression from Firefox 3.6? If so, one thing that would help a lot would be if you could bisect using nightly builds to find out a regression range.

Comment 2

[Eric]

After playing with a bunch of nightlies, and considering I didn't somehow mess up, it seems to have started jumping around in this build:

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a4pre) Gecko/20100319 Minefield/3.7a4pre

Just over a year ago. I should note, however, that while those before this build it didn't jump to the bottom upon loading/viewing, they did not jump to any highlighted text, much like is done in todays builds. (Even though today's builds don't handle this jump to highlighted text as nicely as Chrome does, as can be seen in the video I provided.)

One last thing, I'm not exactly sure if this actually has anything to do with the JavaScript engine, but I figured it might due to how the templates are managed through AJAX and JS.

Comment 3

[David Mandelin [:dmandelin]]

Looks like this might be from bug 353539. Ehsan, can you take a look at this?

Comment 4

[(no longer active)]

Eric, can you reproduce this in Firefox 4.0.1?  What about nightly builds?

Comment 5

[Eric]

Yes, using the latest nightly build I can reproduce this.

Comment 6

[(no longer active)]

Can you try to use the mozregression tool to create a regression range for this, please? http://harthur.github.com/mozregression/

Comment 7

[Eric]

Last good nightly: 2010-3-18 First bad nightly: 2010-03-19

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2cc5ad2cf917&tochange=5108c4c2c043

Comment 8

[Josh Matthews [:jdm]]

b13195a7fb81	Ehsan Akhgari — Bug 353539 - textarea.focus() puts caret at end without scrolling it into view; r=roc
ba70a0cb9240	Ehsan Akhgari — Bug 231389 - Textarea scrolls to top after changing its .value, regardless of cursor position; r=roc

These look like the only likely culprits.

Comment 9

[(no longer active)]

Hmm, right.

Eric, how can I test this in IPB?  I need some way to be able to reproduce this.  If you think you can give me login info to an instance that I can use for testing, please send me an email.  Thanks!

Comment 10

[(no longer active)]

Eric sent me login info privately, and I have reproduced the problem, and I think I have a fix at hand...

Comment 11

[(no longer active)]

OK, so here's the deal.  At first I thought this is something related to bug 629878.  But this is a direct result of bug 353539.

Basically, bug 353539 argues that when you .focus() a textarea, the selection should scroll into view.  This bug argues that it shouldn't!

And this confusion is all coming from the fact that we put the selection at the end of the textarea by default, whereas webkit puts it at the beginning.

I have a hard time determining what the right solution here would be, but I think the sanest solution might be for us to switch to the webkit behavior here.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Well, our old behavior (which Opera shares, by the way) was clearly wrong.

I suppose we could put the caret at the beginning on focus... it really depends on what people do once focus happens.  Do they want to append, or edit from the beginning?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Given that there's no answer that's always correct, and authors can easily work around whatever the default behavior is, I think it's most important to be consistent. Currently IE (quirks and IE9 modes) and Chrome both put the caret at the start of the textarea by default, so I think we might as well change to match.

This needs to be specced though.

I found one other difference between what we do and what IE/Webkit do. If you scroll the caret out of view, click on blank space in the page to move focus away from the textarea (focusing nothing, I think), then cause textarea.focus() to be called, IE and Webkit scroll the caret into view again, but we don't. OTOH if you move focus away to another element like an <input>, then do textarea.focus(), we do scroll the caret into view. That seems like our bug (a different bug from this one).

Comment 14

[(no longer active)]

(In reply to comment #13)
> Given that there's no answer that's always correct, and authors can easily work
> around whatever the default behavior is, I think it's most important to be
> consistent. Currently IE (quirks and IE9 modes) and Chrome both put the caret
> at the start of the textarea by default, so I think we might as well change to
> match.

OK, I'll do that in this bug.

> This needs to be specced though.

I agree.  Does this fall into HTML5's scope?

> I found one other difference between what we do and what IE/Webkit do. If you
> scroll the caret out of view, click on blank space in the page to move focus
> away from the textarea (focusing nothing, I think), then cause textarea.focus()
> to be called, IE and Webkit scroll the caret into view again, but we don't.
> OTOH if you move focus away to another element like an <input>, then do
> textarea.focus(), we do scroll the caret into view. That seems like our bug (a
> different bug from this one).

Hmm, can you please file it (with a test case if you have one)?

Comment 15

[(no longer active)]

Attachment: Patch (v1)

I need to push this to the try server, because other tests in our tree might require adjustments too...

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

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

::: layout/forms/nsTextControlFrame.cpp
@@ +387,5 @@
>    // editor.
>    mUseEditor = PR_TRUE;
>  
> +  // Set the selection to the beginning of the text field.
> +  SetSelectionEndPoints(0, 0);

Why do we need to add this? Isn't there code somewhere else setting the selection to the end of the text, and we should be removing that or changing it?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #14)
> > This needs to be specced though.
> 
> I agree.  Does this fall into HTML5's scope?

Yes.

> Hmm, can you please file it (with a test case if you have one)?

Filed bug 655941 with a testcase.

Comment 18

[(no longer active)]

(In reply to comment #16)
> Why do we need to add this? Isn't there code somewhere else setting the
> selection to the end of the text, and we should be removing that or changing
> it?

The selection being set to the end of the field now is a by-product of setting the value, which causes the selection to be collapsed to the end of the text node (because the range representing the selection is moved after the textnode is mutated in nsRange::CharacterDataChanged), and then <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/nsTextEditRules.cpp#246> it's collapsed on the bogus BR node.  While we could special case CollapseSelectionToTrailingBRIfNeeded to handle this case, I think that would only make the code more complicated, and I'd live to avoid that.  A case in point: right now it may take somebody hours under gdb to determine what code is responsible for the existing selection behavior... ;-)

Comment 19

[(no longer active)]

Attachment: Patch (v2)

With more test fixes...

Comment 20

[(no longer active)]

(In reply to comment #17)
> (In reply to comment #14)
> > > This needs to be specced though.
> > 
> > I agree.  Does this fall into HTML5's scope?
> 
> Yes.

Filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=12644.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 531376 [details] [diff] [review]
Patch (v2)

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

Comment 22

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7af9641c195d

Comment 23

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/HTML/Element/textarea

Also mentioned on Firefox 6 for developers.

Comment 24

[Martijn Wargers (dead)]

This reminds me of bug 128953. If this behavior can be changed so simple, then perhaps that bug should be reconsidered too?