Closed
Bug 612129
Opened 14 years ago
Closed 14 years ago
focus() should place caret at beginning instead of end (to avoid scroll-into-view jumpiness and match other browsers)
Categories
(Core :: DOM: Editor, defect)
Tracking
()
RESOLVED
FIXED
mozilla6
People
(Reporter: eric, Assigned: ehsan.akhgari)
References
Details
(Keywords: dev-doc-complete, regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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¤t=clip0010.mp4
If there's any more I can do to help you reproduce it on your end, I will.
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Comment 1•14 years ago
|
||
(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¤t=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.
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•14 years ago
|
||
Looks like this might be from bug 353539. Ehsan, can you take a look at this?
Assignee: general → nobody
Component: JavaScript Engine → Editor
QA Contact: general → editor
Assignee | ||
Comment 4•14 years ago
|
||
Eric, can you reproduce this in Firefox 4.0.1? What about nightly builds?
Assignee | ||
Comment 6•14 years ago
|
||
Can you try to use the mozregression tool to create a regression range for this, please? http://harthur.github.com/mozregression/
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•14 years ago
|
||
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.
Assignee | ||
Comment 9•14 years ago
|
||
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!
Assignee | ||
Comment 10•14 years ago
|
||
Eric sent me login info privately, and I have reproduced the problem, and I think I have a fix at hand...
Assignee | ||
Comment 11•14 years ago
|
||
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•14 years ago
|
||
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?
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).
Assignee | ||
Comment 14•14 years ago
|
||
(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)?
Assignee | ||
Comment 15•14 years ago
|
||
I need to push this to the try server, because other tests in our tree might require adjustments too...
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?
(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.
Assignee | ||
Comment 18•14 years ago
|
||
(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... ;-)
Assignee | ||
Comment 19•14 years ago
|
||
With more test fixes...
Attachment #531178 -
Attachment is obsolete: true
Attachment #531376 -
Flags: review?(roc)
Attachment #531178 -
Flags: review?(roc)
Assignee | ||
Comment 20•14 years ago
|
||
(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 on attachment 531376 [details] [diff] [review]
Patch (v2)
Review of attachment 531376 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #531376 -
Flags: review?(roc) → review+
Assignee | ||
Comment 22•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Assignee | ||
Updated•14 years ago
|
Keywords: dev-doc-needed
Comment 23•13 years ago
|
||
Documentation updated:
https://developer.mozilla.org/en/HTML/Element/textarea
Also mentioned on Firefox 6 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Comment 24•13 years ago
|
||
This reminds me of bug 128953. If this behavior can be changed so simple, then perhaps that bug should be reconsidered too?
Updated•13 years ago
|
Summary: Template editing in IPB Admin Control Panel is jumpy when switching between templates → focus() should place caret at beginning instead of end (to avoid scroll-into-view jumpiness and match other browsers)
You need to log in
before you can comment on or make changes to this bug.
Description
•