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

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Amir Aharoni]

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

[Alice0775 White]

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

Comment 2

[(no longer active)]

I think we should soft-block on this.

Comment 3

[Roan Kattouw]

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?

Comment 4

[(no longer active)]

Attachment: 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.

Comment 5

[(no longer active)]

Attachment: Patch (v1)

Comment 6

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

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.

Comment 7

[(no longer active)]

(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.

Comment 8

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

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.

Comment 9

[(no longer active)]

(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?

Comment 10

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

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.

Comment 11

[(no longer active)]

(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).

Comment 12

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

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.

Comment 13

[(no longer active)]

(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!  :-)

Comment 14

[(no longer active)]

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.

Comment 15

[(no longer active)]

Attachment: Part 1: Save the scroll state for text controls the standard way

Comment 16

[(no longer active)]

Attachment: Part 2: Do not scroll the selection into view when restoring the selection offsets for a text control after a reframe

Comment 17

[(no longer active)]

Attachment: Part 3: test

This patch takes the test in attachment 508620 [details] [diff] [review] out as a separate patch, unmodified.

Comment 18

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

+  nsCOMPtr<nsITextControlElement> textControlElement =
+    do_QueryInterface(aContent->GetParent());
+  if (aContent->IsInAnonymousSubtree() && !textControlElement) {

We should avoid doing the do_QI when aContent is not anonymous.

Comment 19

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

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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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?

Comment 22

[(no longer active)]

(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?

Comment 23

[(no longer active)]

Attachment: Part 1: Save the scroll state for text controls the standard way

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Is that good enough?

I don't know.  I'd need to read through the code carefully to tell.  Should I?

Comment 25

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

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.

Comment 26

[(no longer active)]

(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...

Comment 27

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

Can't you just make nsTextControlFrame implement nsIStatefulFrame, and forward SaveState/RestoreState to the frame for the anonymous DIV?

Comment 28

[(no longer active)]

Attachment: 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?

Comment 29

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

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

Comment 30

[(no longer active)]

(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!

Comment 31

[(no longer active)]

Attachment: Part 1: Save the scroll state for text controls the standard way

Comment 29 is implemented in this patch.

Comment 32

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

Instead of adding an nsPoint member, use a frame property similar to   NS_DECLARE_FRAME_PROPERTY(ComputedOffsetProperty, DestroyPoint)

Comment 33

[(no longer active)]

Attachment: Part 1: Save the scroll state for text controls the standard way

Using a frame property...

Comment 34

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

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().

Comment 35

[(no longer active)]

Attachment: Part 1: Save the scroll state for text controls the standard way

Done.

Comment 36

[(no longer active)]

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 :(