331269 - Undo or redo doesn't scroll selection into view

Status: REOPENED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

There is another bug open for a similar issue, bug 188184, but I'm just opening a new bug.
According to bug 178139 this was fixed, but it it is now already for quite some time broken again, since I can see the bug also in Mozilla1.7.

Comment 1

[Martijn Wargers (dead)]

Attachment: patch

Well, this fixes it for me.
I'm removing nsAutoUpdateViewBatch here, not sure if that's allowed.
It was added in bug 75305 to fix some crash, but that was a long time ago.

Comment 2

[Martijn Wargers (dead)]

Kathleen, you know whether this code can safely be removed now?

Comment 3

[Kathleen :Brade]

I'm ok with the "scroll selection into view" calls being added (r=brade) but I am concerned about removing the stack-based class--race conditions may still exist.  I'd certainly test this patch against the test cases in bug 75305 and bug 73291.

Ask Kin Blas or Simon Fraser to review/sr this small patch.

Comment 4

[Martijn Wargers (dead)]

Attachment: patch2

Well, the problem is that the scroll selection into view doesn't work without the removal of the nsAutoUpdateViewBatch class, so that needs to be removed to get it working.
I don't crash with the testcases in the bugs you mentioned, fwiw.

+    ScrollSelectionIntoView(PR_TRUE);
+    ScrollSelectionIntoView(PR_FALSE);
I'm doing this to get both ends of the selection into view. First the anchorNode part of the selection, then the focusNode part of the selection (where the caret is).

Comment 5

[Simon Fraser [no longer active]]

The view batch stuff may also have been an optimization.

Comment 6

[Martijn Wargers (dead)]

So basically I want to know whether it's safe to remove the nsAutoUpdateViewBatch class, hoping that someone knows.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'm pretty sure removing that will cause visual issues, and it may well not be safe depending on what else is in the reflow queue, etc.  I'd have to study the editor code and everything it notifies pretty deeply to judge.

Basically, the batching there looks like the right thing to be doing.  So the real issue is the interaction of batching with selection, no?

Comment 8

[Martijn Wargers (dead)]

Attachment: patch3

So like this then?
Turn the ViewBatch off just before the scrollselectionintoview code.
I'm removing the nsAutoUpdateViewBatch class and replacing it with BeginUpdateViewBatch and EndUpdateViewBatch (which is what the nsAutoUpdateViewBatch class also was doing).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Except you need to be inside the batch across the Undo and Redo calls, of course.  And I'm not sure why we're removing the helper class...

Comment 10

[Martijn Wargers (dead)]

(In reply to comment #9)
> Except you need to be inside the batch across the Undo and Redo calls, of
> course.  And I'm not sure why we're removing the helper class...
Oops, you're right. So maybe I can move BeginUpdateViewBatch() and EndUpdateViewBatch() into nsEditor::Undo.
I'm removing the helper class, because there are only 3 callers, and with what I'm changing there would only be 1 caller left. But I can keep it in if you prefer?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> move BeginUpdateViewBatch() and EndUpdateViewBatch() into nsEditor::Undo.

Not sure whether it needs to happen around the other stuff those methods are doing... and are there other callers of nsEditor::Undo and nsEditor::Redo?

> and with what I'm changing there would only be 1 caller left

Ah, then probably no need for a helper class if early returns are a non-issue.

Comment 12

[Martijn Wargers (dead)]

Attachment: patch4

(In reply to comment #11)
> Not sure whether it needs to happen around the other stuff those methods are
> doing... and are there other callers of nsEditor::Undo and nsEditor::Redo?
Yes, that's what I realised after I had made my previous comment, so in this patch I'm copying it, not sure if this is the right way.
Afaik, only nsHTMLEditor also uses nsEditor::Undo also, so with this patch nsHTMLEditor would also use UpdateViewBatching. Not sure if that's a good thing.
Otherwise, I could start batching in nsPlaintextEditor::Undo and stop batching in nsEditor::Undo while testing whether it is a plaintexteditor.

Btw, see:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/libeditor/base/nsEditor.cpp&rev=1.460#2699
// XXX we may not need these view batches anymore.  This is handled at a higher level now I believe
That's an incorrect statement then, I presume?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I have no idea how correct that comment is....  :(

Comment 14

[Simon Fraser [no longer active]]

(In reply to comment #12)

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/editor/libeditor/base/nsEditor.cpp&rev=1.460#2699
> // XXX we may not need these view batches anymore.  This is handled at a higher
> level now I believe
> That's an incorrect statement then, I presume?

I think that just refers to the batch around DoTransaction().

In your patch, rather that not using the nsAutoBatch class, you should just add scoping to control when its destructor fires, if indeed changing the end of the batch is the correct thing to do. Getting this wrong could hurt typing performance quite a bit.

Comment 15

[Martijn Wargers (dead)]

Attachment: patch5

(In reply to comment #14)
> In your patch, rather that not using the nsAutoBatch class, you should just add
> scoping to control when its destructor fires, if indeed changing the end of the
> batch is the correct thing to do. Getting this wrong could hurt typing
> performance quite a bit.
Well, only undo and redo is involved, so it would only hurt undo/redo performance.
But anyway, it seems that using a nsAutoPlaceHolderBatch is the answer to the problem. It does the same as nsAutoUpdateViewBatch, but scrolls the selection into view after batching is done.
I haven't removed nsAutoUpdateViewBatch in this patch, you all seem keen on keeping it, but there is only one consumer left.

Comment 16

[Simon Fraser [no longer active]]

Comment on attachment 217278 [details] [diff] [review]
patch5

>Index: editor/libeditor/base/nsEditor.cpp
>===================================================================

>     // time to turn off the batch
>     EndUpdateViewBatch();
>     // make sure selection is in view
>+    PRBool isCollapsed = PR_FALSE;
>+    selection->GetIsCollapsed(&isCollapsed);
>+    if (!isCollapsed)
>+      ScrollSelectionIntoView(PR_TRUE);

Don't you want the caret scrolled into view (when the selection is collapsed)?

>Index: editor/libeditor/text/nsPlaintextEditor.cpp
>===================================================================

> NS_IMETHODIMP 
> nsPlaintextEditor::Undo(PRUint32 aCount)
> {
>-  nsAutoUpdateViewBatch beginViewBatching(this);
>+  nsAutoPlaceHolderBatch batch(this, nsnull);

nsAutoPlaceHolderBatch does something different to the view batch,
and is not a drop-in replacement. It puts placeholder transactions
on the transaction stack, and has nothing to do with view refreshes.

Comment 17

[Martijn Wargers (dead)]

Attachment: patch6

Ok, it appears to be that I don't need the scrollselectionintoview code in nseditor.cpp, since this patch also seems to work for designmode iframes (which I thought would not work, but which I did not test, until now).

(In reply to comment #16)
> Don't you want the caret scrolled into view (when the selection is collapsed)?
See:
http://lxr.mozilla.org/seamonkey/source/editor/libeditor/base/nsEditor.h#310
The focusNode is where the caret is. When the selection is collapse, anchorNode and focusNode are the same.
Note that this is only optimisation, I can leave it out, if you prefer.
ScrollSelectionIntoView(PR_TRUE) is only necessary (and not event then always) when there is an uncollapsed selection.

Comment 18

[Simon Fraser [no longer active]]

Comment on attachment 217365 [details] [diff] [review]
patch6

>Index: editor/libeditor/text/nsPlaintextEditor.cpp
>===================================================================

>-   
>+
>+  EndUpdateViewBatch();
>+
>+  PRBool isCollapsed = PR_FALSE;
>+  selection->GetIsCollapsed(&isCollapsed);
>+  if (!isCollapsed)
>+    ScrollSelectionIntoView(PR_TRUE);
>+  ScrollSelectionIntoView(PR_TRUE);
>+  ScrollSelectionIntoView(PR_FALSE);

I don't see why you need to call it 2 or 3 times here.

>+  if (!isCollapsed)
>+    ScrollSelectionIntoView(PR_TRUE);
>+  ScrollSelectionIntoView(PR_FALSE);

Same.

Comment 19

[Martijn Wargers (dead)]

(In reply to comment #18)
> I don't see why you need to call it 2 or 3 times here.

Sorry, 3 times was too much, but I need this (when a selection is not collapsed):
+    ScrollSelectionIntoView(PR_TRUE);
+  ScrollSelectionIntoView(PR_FALSE);
To get both the anchorNode part of the selection as the focusNode part of the selection into view. 

Comment 20

[Martijn Wargers (dead)]

Attachment: testcase

Consider this testcase, you need to test it locally because of enhanced privileges.
- select the bottom text ("Select this text from downside up") from downside up
- Scroll the textarea to the top
- Press the scrollselectionintoview button
Result: only the first line of the selection is visible.

That's why I'm also doing the scrollselectionintoview for the anchorNode.
I know it's stupid to call the same function two times, but I currently see no other way. I think there should be a way, but I don't think there is a way currently.

Comment 21

[Simon Fraser [no longer active]]

(In reply to comment #19)
> To get both the anchorNode part of the selection as the focusNode part of the
> selection into view. 

Oh I see, you want both ends. Well, first, if the selection is long you have no guarantee that both will be showable at the same time, so you'll have to make a choice. Second, I think the better fix would be to fix ScrollSelectionIntoView() and whatever it calls to do a better job, perhaps with richer parameters.

Comment 22

[Martijn Wargers (dead)]

I've made that choice by first calling scrollSelectionIntoView(true) and then scrollSelectionIntoView(false).
This way, the focusNode is always in view (that is the part where the caret is).

I agree, scrollSelectionIntoView needs to be fixed.
I've filed bug 333136 for it and made a suggestion for a possible solution.
Not sure I can fix that :(

Comment 23

[Martijn Wargers (dead)]

Attachment: patch7

Ok, bug 333136 is fixed, so now the patch can be simplified to this.

Comment 24

[Martijn Wargers (dead)]

Comment on attachment 218315 [details] [diff] [review]
patch7

Patch for bug 333136 wasn't any good, so that means this patch is also not suitable anymore.

Comment 25

[Martijn Wargers (dead)]

Attachment: patch8

Ok, this is based on the suggestion in comment 21.
I've used the existing NUM_SELECTION_REGIONS name for the behavior I want for undo/redo scrollselectionintoview.
That one isn't used currently. But maybe I should use a different, more appropriate name? (anyone has a suggestion?)
I've made the patch so, that after an undo/redo, the focusNode part of the selection is always visible.
However, after an undo transaction, the focusNode is always at the most right/bottom, so it's not that useful yet. I filed bug 358033 for that.

Comment 26

[shimon doodkin]

i added a bug, maybe related:

no scrollInToView on undo/redo in textarea
https://bugzilla.mozilla.org/show_bug.cgi?id=361196

Comment 27

[Boris Zbarsky [:bzbarsky]]

*** Bug 361196 has been marked as a duplicate of this bug. ***

Comment 28

[Martijn Wargers (dead)]

Comment on attachment 243487 [details] [diff] [review]
patch8

Simon or Kathleen, can one of you review?

Comment 29

[Kathleen :Brade]

Comment on attachment 243487 [details] [diff] [review]
patch8

r=brade but I defer to Simon for the changes in nsPlaintextEditor::Undo().

Nit: there seems to be alignment issues in nsSelection.cpp; an "if (ns_failed)" block is off by a space.

Comment 30

[Simon Fraser [no longer active]]

Comment on attachment 243487 [details] [diff] [review]
patch8

>Index: editor/libeditor/text/nsPlaintextEditor.cpp
>===================================================================

> NS_IMETHODIMP 
> nsPlaintextEditor::Undo(PRUint32 aCount)
> {
>-  nsAutoUpdateViewBatch beginViewBatching(this);
>+  BeginUpdateViewBatch();

Rather than removing the nsAutoUpdateViewBatch, you could just add scope
which ends before the ScrollSelectionIntoView() call. This way, it's robust
to people adding returns in the middle.

>+  ScrollSelectionIntoView(nsISelectionController::NUM_SELECTION_REGIONS);

I don't like this use of the "special" NUM_SELECTION_REGIONS value.
It's not obvious what passing that value means.

>Index: layout/generic/nsSelection.cpp
>===================================================================

>+ 
>+      if (anchorScrollableView == focusScrollableView) {
>+        nsRect newRect;
>+        newRect.UnionRect(focusRect, anchorRect);
>+
>+        result = ScrollRectIntoView(anchorScrollableView, newRect,
>+                                    NS_PRESSHELL_SCROLL_ANYWHERE,
>+                                    NS_PRESSHELL_SCROLL_ANYWHERE, PR_TRUE);
>+      }
>+
>+      if (focusScrollableView) {
>+        return ScrollRectIntoView(focusScrollableView, focusRect,
>+                                    NS_PRESSHELL_SCROLL_ANYWHERE,
>+                                    NS_PRESSHELL_SCROLL_ANYWHERE, PR_TRUE);
>+      }
>+      if (anchorScrollableView) {
>+        result = ScrollRectIntoView(anchorScrollableView, anchorRect,
>+                                    NS_PRESSHELL_SCROLL_ANYWHERE,
>+                                    NS_PRESSHELL_SCROLL_ANYWHERE, PR_TRUE);
>+      }

Aren't you potentially doing 3 scrolls here? That sounds pretty expensive.

Comment 31

[Martijn Wargers (dead)]

Attachment: patch9

I sort of tried to fix the comments made in comment 30.

But I still have to use ScrollRectIntoView 2 times for the NUM_SELECTION_REGIONS case. I think this is because of:
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsSelection.cpp#7157
7157         // Scroll down so aRect's bottom edge is visible. Make sure
7158         // aRect's top edge is still visible
But because of bug 358033, the bottom edge is where the focus node is, so I really need to make sure that the rect's bottom edge is still visible.

I guess I could change the ScrollRectIntoView function to do that, but I'm afraid I would break things by doing that.
Anyway, if bug 358033 is fixed, I would need a way anyway to make sure that the focus node rect is still visible (which could be on the bottom or top edge then).

Comment 32

[Martijn Wargers (dead)]

Attachment: patch9b

This is an alternative patch without the NUM_SELECTION_REGIONS stuff.
Instead, this patch makes undo/redo scroll to the focus node.
So that means that there is a chance that you just get one line of the selection scrolled into view, while there is a bigger selection.
I guess you might like this one more.

Comment 33

[Martijn Wargers (dead)]

Attachment: patch9b - diffw

Comment 34

[Martijn Wargers (dead)]

Comment on attachment 282639 [details] [diff] [review]
patch9b

My guess is you will like this patch more than patch9.

This patch doesn't try to scroll as much of the selection as possible into view, but it scrolls at least the focus node of the selection into view.

Comment 35

[Simon Fraser [no longer active]]

Comment on attachment 282639 [details] [diff] [review]
patch9b

I assume you'll take out the if (1)?

Comment 36

[Martijn Wargers (dead)]

(In reply to comment #35)
> I assume you'll take out the if (1)?

But that's needed for the scoping of the nsAutoUpdateViewBatch class, no?

Comment 37

[Kathleen :Brade]

Comment on attachment 282639 [details] [diff] [review]
patch9b

Hello.  I'm sorry if I'm out of line but I need to r- this patch.  The patch is incomplete.

These calls also need to be fixed:
http://lxr.mozilla.org/seamonkey/source/editor/libeditor/html/nsHTMLDataTransfer.cpp#1444
http://lxr.mozilla.org/seamonkey/source/editor/libeditor/text/nsPlaintextDataTransfer.cpp#141
http://lxr.mozilla.org/seamonkey/source/editor/libeditor/html/nsHTMLEditor.cpp#1288

Also, are there going to be tests?

Thanks!

Comment 38

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #36)
> (In reply to comment #35)
> > I assume you'll take out the if (1)?
> 
> But that's needed for the scoping of the nsAutoUpdateViewBatch class, no?

You can introduce a new block without the bogus conditional, the brackets alone suffice.

Comment 40

[Eduard Moraru]

Ping 3 years after...

Comment 41

[Martijn Wargers (dead)]

Feel free to take this bug over from me.

Comment 42

[(no longer active)]

This should be really easy, based on the existing patch...

Note that you would also need to add a test for this.  We have existing tests which test the scroll position after editing operations.  Also note that we need tests for both the plaintext and HTML editors.

Comment 43

[alexander.kern]

I just found this out and then wanted to report it but found this bug. It's easy to test it here on bugzilla, just type stuff that is not in view, hit ctrl+z and see how you don't see anything, because Firefox doesn't scroll the line where the change happened into view. It seems kind of important to me because it's easy to delete stuff that way without noticing, so I decided to comment on this since the last one was 7 years ago.

Related: https://bugzilla.mozilla.org/show_bug.cgi?id=188184
maybe https://bugzilla.mozilla.org/show_bug.cgi?id=427172 ?

Cheers