Cursor goes to the end of the editing box in Wikipedia and WordPress

RESOLVED FIXED in mozilla2.0b12

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: amir.aharoni, Assigned: Ehsan)

Tracking

({regression})

Trunk
mozilla2.0b12
x86
All
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker], URL)

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b10) Gecko/20100101 Firefox/4.0b10
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b10) Gecko/20100101 Firefox/4.0b10

Before Firefox beta 10, when i would open for editing a Wikipedia article or a Wordpress.com post, the cursor would be placed at the beginning of the editing box. Since Firefox beta 10 it is placed at its end. This doesn't happen if i open for editing a draft email in GMail - there the cursor is placed at the beginning.

Unfortunately i don't know what exactly causes it, but it seems that it is something that Wikipedia and Wordpress.com have in common and that GMail has not.

Reproducible: Always

Steps to Reproduce:
1. Open a Wikipedia article, for example http://en.wikipedia.org/wiki/Holam .
2. Click the "Edit" tab at the top.
Actual Results:  
The editing box is scrolled all the way to the bottom and the cursor is placed at its end.

Expected Results:  
The cursor is supposed to be placed at the beginning of the editing box and it is not supposed to scroll down.

Comment 1

8 years ago
Confirmed on
Mozilla/5.0 (X11; Linux i686; rv:2.0b11pre) Gecko/20110129 Firefox/4.0b11pre ID:20110129030338

Regression window(cached hourly):
Works:
http://hg.mozilla.org/mozilla-central/rev/749b2a61f372
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101010 Firefox/4.0b8pre ID:20101013063308
Fails:
http://hg.mozilla.org/mozilla-central/rev/6ed1d80316c0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101012 Firefox/4.0b8pre ID:20101013094112
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=749b2a61f372&tochange=6ed1d80316c0

In local build,
build from 178f26e21cfc : fails
build from 18d0545c5f0e : works
Blocks: 597331
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Component: General → Editor
Ever confirmed: true
Keywords: regression
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → editor
Version: unspecified → Trunk
(Assignee)

Comment 2

8 years ago
I think we should soft-block on this.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
blocking2.0: ? → final+
Whiteboard: [softblocker]

Comment 3

8 years ago
The most likely cause is that DOM manipulation is done on the <textarea>. On Wikipedia, there is JS wrapping the <textarea> in a bunch of <div>s after document ready.

However, since the report only mentions that GMail doesn't do this, it might just be a difference between <textarea>s and content-editable <iframe>s, regardless of DOM manipulation. If that were the case, however, it would probably have been noticed earlier and on more web sites, so I don't think this is likely, but maybe you could test on a page with a vanilla <textarea> with no JS messing around with it, just to be sure?
(Assignee)

Comment 4

8 years ago
Created attachment 508609 [details]
Test case

On a build with this problem fixed, the textarea should not be scrolled to the bottom.

We should really only restore the text selection offsets if the original selection was visible.  Reframes like what's done in the test case should not be restoring the selection.

I have a patch which fixes this.  I tested it both on wikipedia and wordpress (in addition to this test case) to make sure that the scrolling problem is fixed with this patch.  I'll attach it after writing a test for it.
(Assignee)

Comment 5

8 years ago
Created attachment 508620 [details] [diff] [review]
Patch (v1)
Attachment #508620 - Flags: review?(roc)
Boolean parameters are bad. Instead of aIgnoreInvisibleSelection, why not let the caller do the CheckVisibility calls?

Another option would be to have GetVisibleSelectionRange be a method that calls GetSelectionRange and then does the visibility checks.
(Assignee)

Comment 7

8 years ago
(In reply to comment #6)
> Boolean parameters are bad. Instead of aIgnoreInvisibleSelection, why not let
> the caller do the CheckVisibility calls?

The caller doesn't have access to the DOM nodes, and we really don't want to give it access to them, since they're text nodes inside the anonymous subtree for the frame.

> Another option would be to have GetVisibleSelectionRange be a method that calls
> GetSelectionRange and then does the visibility checks.

This is not feasible because of the same problem as above.

I guess I could refactor GetSelectionRange into two methods, one which returns the DOM nodes and offsets, and one which computed the total offsets from those, but that sounds like a stretch to me.  But please let me know if that's what you'd rather I do here, or if you have something else in mind.
OK, how about giving GetSelectionRange an out parameter that receives whether the selection is visible or not?

Now that I look at this patch again, I'm not sure this is the right way to go. Seems to me that on a reframe we should simply never scroll anything into view and just restore the old scroll position.
(Assignee)

Comment 9

8 years ago
(In reply to comment #8)
> Now that I look at this patch again, I'm not sure this is the right way to go.
> Seems to me that on a reframe we should simply never scroll anything into view
> and just restore the old scroll position.

But that would break usescases like this:

1. Load https://bug596710.bugzilla.mozilla.org/attachment.cgi?id=476169.
2. Click on the right side of the last line.  The caret seems to be on the end
of that line.
3. Press the down arrow key (or the left arrow key).

If we don't scroll the selection into view, the down arrow would seem to have no effect (because the new caret position is outside of the view, and it won't be visible unless we scroll it into view).  Would that not be worth supporting?
I am confused. With my proposal, after step 3 we would scroll the caret into view, and then after the reframe we would scroll to the position before the reframe, i.e. with the caret in view.
(Assignee)

Comment 11

8 years ago
(In reply to comment #10)
> I am confused. With my proposal, after step 3 we would scroll the caret into
> view, and then after the reframe we would scroll to the position before the
> reframe, i.e. with the caret in view.

No, after the reflow, there is nothing to scroll to the position before the reframe, until we do it explicitly as we do right now.  (I've tested this, removing the ScrollSelectionIntoView call after the reframe makes us not scroll to the current selection).
Which reflow? I'm confused. Are you saying that after the keypress, before we fire the timeout and reframe, it's impossible for us to scroll down?

Reframes are an artifact of our implementation. They should never have unnecessary side effects such as resetting state or scrolling things into view.
(Assignee)

Comment 13

8 years ago
(In reply to comment #12)
> Which reflow? I'm confused. Are you saying that after the keypress, before we
> fire the timeout and reframe, it's impossible for us to scroll down?

Sorry, I meant reframe.

I'm saying that after we reframe, we reset the scroll position manually by scrolling to the selection right now.  If we only set the scroll position without scrolling it into the view *after the reframe*, the selection will not be scrolled into view.  That will cause the textarea to not scroll correctly after step 3 in the testcase in comment 9.

> Reframes are an artifact of our implementation. They should never have
> unnecessary side effects such as resetting state or scrolling things into view.

Well, resetting the selection state and scrolling things into view are what we're doing precisely in order to work around the unnecessary side effects.  Before bug 597331, the reframe caused the caret to change its position incorrectly in the testcase in bug 597331 comment 0.  With that bug fixed, the carets position changes correctly as far as the user is concerned.  What happens behind the scenes is that we reset the selection state and scroll it into view *after* the reframe in order to make sure that we don't have any side effects.

This bug, which happens as a side effect of the fix in bug 597331, is about reframes to textareas before the load fires (such as what happens when we move the textarea in the DOM, as the test case in attachment 508609 [details] shows.  My fix in patch (v1) is kind of a hack, and I'm not completely satisfied with it, but my point is that implementing your suggestion in the last paragraph of comment 8 makes us suffer from some of the forms of bug 597331 again (such as the testcase I presented in comment 9).


Sorry if things are getting exponentially more confusing here!  :-)
(Assignee)

Comment 14

8 years ago
I had a conversation about this with roc.  roc is suggesting that we should use nsGfxScrollFrameInner::{Save/Restore}State to restore the scroll position after the reframe.  The reason that it probably doesn't work for text controls right now is the destruction and recreation of the anonymous content, but I bet I can fix that!

Once that's done, the hack in bug 597331 won't be necessary, and this would probably be fixed by removing that hack.
(Assignee)

Updated

8 years ago
Attachment #508620 - Attachment is obsolete: true
Attachment #508620 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Whiteboard: [softblocker] → [softblocker][needs new patch]
(Assignee)

Comment 15

8 years ago
Created attachment 509919 [details] [diff] [review]
Part 1: Save the scroll state for text controls the standard way
Attachment #509919 - Flags: review?(roc)
(Assignee)

Comment 16

8 years ago
Created attachment 509947 [details] [diff] [review]
Part 2: Do not scroll the selection into view when restoring the selection offsets for a text control after a reframe
Attachment #509947 - Flags: review?(roc)
(Assignee)

Comment 17

8 years ago
Created attachment 509948 [details] [diff] [review]
Part 3: test

This patch takes the test in attachment 508620 [details] [diff] [review] out as a separate patch, unmodified.
Attachment #509948 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Whiteboard: [softblocker][needs new patch] → [softblocker][has patch][needs review roc]
+  nsCOMPtr<nsITextControlElement> textControlElement =
+    do_QueryInterface(aContent->GetParent());
+  if (aContent->IsInAnonymousSubtree() && !textControlElement) {

We should avoid doing the do_QI when aContent is not anonymous.
I'm concerned that this change might lead to us restoring state for the text control in an <input type="file">. We shouldn't do that.

I actually wonder why we don't just restore state for all anonymous content, and have a special check for subtrees under <input type="file"> (and maybe certain others we want to exclude). Not restoring state for anonymous content goes way back to 1999:
https://bugzilla.mozilla.org/show_bug.cgi?id=13058
But I guess we don't want to make that big a change for FF4.

For now, let's allow state save/restore for anonymous elements whose parent is a non-anonymous text control.
> I actually wonder why we don't just restore state for all anonymous content,

Because the state-restoration key generation algorithm uses things like indices in the form, indices in its parent, etc.  None of which make sense for anonymous content.
And in particular, the attached patch looks wrong given that.  At the very least, state key generation for anonymous content should encode that fact in the key so that it can't stomp on non-anonymous content, no?
(Assignee)

Comment 22

8 years ago
(In reply to comment #19)
> I'm concerned that this change might lead to us restoring state for the text
> control in an <input type="file">. We shouldn't do that.

Hmm, yes, you're right.  Will submit a new patch.

(In reply to comment #21)
> And in particular, the attached patch looks wrong given that.  At the very
> least, state key generation for anonymous content should encode that fact in
> the key so that it can't stomp on non-anonymous content, no?

For anonymous content, the index "-1" is used in the key generation algorithm, and it seems to be working fine.  Is that good enough?
(Assignee)

Comment 23

8 years ago
Created attachment 510208 [details] [diff] [review]
Part 1: Save the scroll state for text controls the standard way
Attachment #509919 - Attachment is obsolete: true
Attachment #510208 - Flags: review?(roc)
Attachment #509919 - Flags: review?(roc)
> Is that good enough?

I don't know.  I'd need to read through the code carefully to tell.  Should I?
Maybe it's safer/better to propagate the state of the anonymous DIV to the state of the text control element, if that's simple to do.
(Assignee)

Comment 26

8 years ago
(In reply to comment #25)
> Maybe it's safer/better to propagate the state of the anonymous DIV to the
> state of the text control element, if that's simple to do.

That's not really simple, I don't think...  But if you or bz feel that my current approach is not safe, I'll look into doing that...
Can't you just make nsTextControlFrame implement nsIStatefulFrame, and forward SaveState/RestoreState to the frame for the anonymous DIV?
(Assignee)

Comment 28

8 years ago
Created attachment 510791 [details] [diff] [review]
Part 1: Save the scroll state for text controls the standard way

(In reply to comment #27)
> Can't you just make nsTextControlFrame implement nsIStatefulFrame, and forward
> SaveState/RestoreState to the frame for the anonymous DIV?

Well, it was not that easy, since the nsTextControlFrame::RestoreState call would happen before the anonymous content/frame ever exists.  But I managed to find a not-so-horrible way to fix that (by performing a tiny surgery on nsFrameManager::RestoreFrameStateFor.  How do you like this?
Attachment #510208 - Attachment is obsolete: true
Attachment #510791 - Flags: review?(roc)
Attachment #510208 - Flags: review?(roc)
Hmm, this is a bit too hacky for my taste. How about this alternative:
-- Make nsTextControlFrame::RestoreState stash the scroll position as a new nsPoint frame property and succeed, if the anonymous DIV's frame does not exist
-- In nsTextControlFrame::SetInitialChildList, grab the scroll position from the property and set it on the anonymous DIV's frame
(Assignee)

Comment 30

8 years ago
(In reply to comment #29)
> Hmm, this is a bit too hacky for my taste. How about this alternative:
> -- Make nsTextControlFrame::RestoreState stash the scroll position as a new
> nsPoint frame property and succeed, if the anonymous DIV's frame does not exist
> -- In nsTextControlFrame::SetInitialChildList, grab the scroll position from
> the property and set it on the anonymous DIV's frame

Sounds like a plan!
(Assignee)

Comment 31

8 years ago
Created attachment 510820 [details] [diff] [review]
Part 1: Save the scroll state for text controls the standard way

Comment 29 is implemented in this patch.
Attachment #510791 - Attachment is obsolete: true
Attachment #510820 - Flags: review?(roc)
Attachment #510791 - Flags: review?(roc)
Instead of adding an nsPoint member, use a frame property similar to   NS_DECLARE_FRAME_PROPERTY(ComputedOffsetProperty, DestroyPoint)
(Assignee)

Comment 33

8 years ago
Created attachment 510889 [details] [diff] [review]
Part 1: Save the scroll state for text controls the standard way

Using a frame property...
Attachment #510820 - Attachment is obsolete: true
Attachment #510889 - Flags: review?(roc)
Attachment #510820 - Flags: review?(roc)
Comment on attachment 510889 [details] [diff] [review]
Part 1: Save the scroll state for text controls the standard way

+    nsPoint* contentScrollPos = static_cast<nsPoint*>
+      (Properties().Get(ContentScrollPos()));
+    if (contentScrollPos) {
+      // If we have a scroll pos stored to be passed to our anonymous
+      // div, do it here!
+      nsIStatefulFrame* statefulFrame = do_QueryFrame(first);
+      NS_ASSERTION(statefulFrame, "unexpected type of frame for the anonymous div");
+      nsPresState fakePresState;
+      fakePresState.SetScrollState(*contentScrollPos);
+      statefulFrame->RestoreState(&fakePresState);
+      Properties().Delete(ContentScrollPos());

Use Properties().RemoveProperty() and "delete contentScrollPos" instead of calling Properties().Delete().
Attachment #510889 - Flags: review?(roc) → review+
(Assignee)

Comment 35

8 years ago
Created attachment 511091 [details] [diff] [review]
Part 1: Save the scroll state for text controls the standard way

Done.
Attachment #510889 - Attachment is obsolete: true
(Assignee)

Comment 36

8 years ago
http://hg.mozilla.org/mozilla-central/rev/757171acb2a2
http://hg.mozilla.org/mozilla-central/rev/c7327b618b34
http://hg.mozilla.org/mozilla-central/rev/855ac974c3c1

And:

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

as a bustage fix because I failed to test the final version of the patch before pushing.  My bad :(
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [softblocker][has patch][needs review roc] → [softblocker]
Target Milestone: --- → mozilla2.0b12
You need to log in before you can comment on or make changes to this bug.