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)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: eric, Assigned: ehsan.akhgari)

References

Details

(Keywords: dev-doc-complete, regression)

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
(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.
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.
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
Eric, can you reproduce this in Firefox 4.0.1? What about nightly builds?
Yes, using the latest nightly build I can reproduce this.
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
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.
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!
Eric sent me login info privately, and I have reproduced the problem, and I think I have a fix at hand...
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.
Blocks: 353539
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
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).
(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)?
Attached patch Patch (v1) (obsolete) (deleted) — Splinter Review
I need to push this to the try server, because other tests in our tree might require adjustments too...
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #531178 - Flags: review?(roc)
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.
(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... ;-)
Attached patch Patch (v2) (deleted) — Splinter Review
With more test fixes...
Attachment #531178 - Attachment is obsolete: true
Attachment #531376 - Flags: review?(roc)
Attachment #531178 - Flags: review?(roc)
(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+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Keywords: dev-doc-needed
Documentation updated: https://developer.mozilla.org/en/HTML/Element/textarea Also mentioned on Firefox 6 for developers.
Depends on: 669184
This reminds me of bug 128953. If this behavior can be changed so simple, then perhaps that bug should be reconsidered too?
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.

Attachment

General

Created:
Updated:
Size: