Open Bug 671187 Opened 13 years ago Updated 2 years ago

Cannot scroll iframe by drag if the iframe is overflowed from its parent

Categories

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P5)

Product:
Core
Component:
DOM: Copy & Paste and Drag & Drop
Type:
defect

Tracking

()

People

(Reporter: masayuki, Unassigned)

References

(
URL
)

Details

Attachments

(5 files, 4 obsolete files)

Patch part.1 Make inline methods to check reason of mouse event
6.14 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Patch part.2 Make a new mouse event reason for scrolling by drag
15.80 KB, patch
smaug
: review+
roc
: superreview+
Details | Diff | Splinter Review
Patch part.5 Ancestor scrollable elements of selection root element should be scrollable by on-edge-scrolling
17.99 KB, patch
smaug
: review+
faaborg
: ui-review+
Details | Diff | Splinter Review
Patch part.4 Sort out drag handling code of nsFrame
20.34 KB, patch
roc
: review+
Details | Diff | Splinter Review
Patch part.3 Use synthesized mousemove event for auto scroll at dragging
26.75 KB, patch
smaug
: review+
roc
: superreview+
Details | Diff | Splinter Review 
1. Open the URL in smaller window (i.e., need to resize if you don't see body's scrollbar).
2. Mousedown on "aaaaa".
3. Mousemove to bottom edge of the parent's body.

The iframe cannot be scrolled because its bottom edge is clipped by parent's body.

I'm looking for the simple fix...
I'll add new synthesized event reason for auto scrolling. This patch sorts out the related code by the new inline functions.
Attachment #548711 - Flags: review?(Olli.Pettay)
This patch adds a new synthesized mouse reason and changes nsIPresShell::SynthesizeMouseMove() for it can dispatch the new reason.

And PresShell::HandleEventInternal() calls nsIFrame::HandleSynthesizedMouseEvent() since the synthesized events don't cause DOM events.
Attachment #548713 - Flags: review?(Olli.Pettay)
This is first patch witch changes the behavior actually.

First, nsTypedSelection::DoAutoScroll() is moved to nsFrame::DoAutoScroll().

Second, the auto scroll timer fires the new synthesized mouse move event.

And nsFrame handles the new mouse move event as real mouse move event for repeating to scroll.

By this change, nsFrame can always choose a scroll target which is under mouse cursor. This provides better behavior with part.5.

And also, PresShell has a bug, this patch also fixes it too. When NS_MOUSE_EXIT is handling, root PresShell forgets the last mouse cursor position even when mouse events are captured.  This bug causes stopping this new auto scrolling unexpectedly if mouse cursor is moved outside of the window.

This patch makes the root PresShell not forget the mouse cursor position if mouse events are captured.  And when mouse capture is released, it forgets the cursor position.

Even if capturing content is replaced with another content, the PresShell forgets the cursor position. This isn't the best behavior. However, another window might be topmost under the cursor. So, we need to wait next real mouse move event for knowing correct position.
Attachment #548715 - Flags: review?(Olli.Pettay)
This patch sorts out the drag handling code in nsFrame.cpp. This doesn't change behavior.

See each static method's comment for the detail of each of them.

nsFrame::IsOnScrollableFrameEdge() is added new parameter aCursorPosInScrollableFrame. It doesn't make sense in this patch. But this would help next patch's performance. We can reduce the count of calling nsLayoutUtils::GetEventCoordinatesRelativeTo().
Attachment #548716 - Flags: review?(Olli.Pettay)
This patch is the main patch of this bug fix.

When mouse cursor isn't on any scrollable edge inside selection root element, nsFrame tries to find an ancestor scrollable frame which clips selection root element. If mouse cursor is *in* selection root frame and mouse cursor is on an ancestor's edge or outside of it, that means the selection root frame is clipped by the found ancestor.  Then, this scrolls the ancestor.

Note that if mouse cursor is outside of the selection root frame, this patch doesn't change the behavior. So, this is a little bit safer than its look.

By this change, user becomes possible to scroll to every point under selection root by dragging. Therefore, now, we can limit the selection target by nsIPresShell::SCROLL_NO_PARENT_FRAMES. This makes easily understandable behavior when there are complex scrollable elements are nested.
Attachment #548718 - Flags: review?(Olli.Pettay)
Comment on attachment 548718 [details] [diff] [review]
Patch part.5 Ancestor scrollable elements of selection root element should be scrollable by on-edge-scrolling

And this patch should be also needs ui-review. faaborg, see also comment 5.

This patch adds a special case only when mouse cursor is in selection root element rect but actually on another element, i.e., selection root element is clipped by its ancestor.

So, this makes user can scroll only ancestor scrollable elements, i.e., not all scrollable elements under sibling elements of ancestors.
Attachment #548718 - Flags: ui-review?(faaborg)
Smaug:

There is another reason to change the scroll timer behavior (part.3).

That is for bug 672181. I think that the bug can be fixed easily if we don't start auto scroll by real mouse move event.  I.e., nsFrame should scroll only by the new synthesized mouse move event. It means that continuous real mouse move events can prevent auto scrolling even if dragging on scrollable edges. I believe that this delay to scroll approach is correct because the main purpose of dragging is expanding selection, not scroll in most cases. Therefore, I think we should give the operation -- expanding selection -- higher priority.
Comment on attachment 548718 [details] [diff] [review]
Patch part.5 Ancestor scrollable elements of selection root element should be scrollable by on-edge-scrolling

I have to admit my understanding of what this patch does based on the description might not be fully accurate since I read through this pretty quickly, but I agree that we should give priority to expanding the selection.
Attachment #548718 - Flags: ui-review?(faaborg) → ui-review+
Attachment #548711 - Flags: review?(Olli.Pettay) → review+
Attachment #548713 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 548716 [details] [diff] [review]
Patch part.4 Sort out drag handling code of nsFrame

this certainly needs a layout peer review.
Comment on attachment 548716 [details] [diff] [review]
Patch part.4 Sort out drag handling code of nsFrame

Sure. I think that roc is best person.

Roc, see comment 4 about the detail of this patch.
Attachment #548716 - Flags: review?(Olli.Pettay) → review?(roc)
Comment on attachment 548713 [details] [diff] [review]
Patch part.2 Make a new mouse event reason for scrolling by drag

needs sr due to nsIPresShell change.
Attachment #548713 - Flags: superreview?(roc)
Btw, since this is making rather major changes late in this cycle, I wonder
if we should backout bug 552707 from m-c and try to re-land it after
next Aurora.
Attachment #548713 - Flags: superreview?(roc) → superreview+
Comment on attachment 548716 [details] [diff] [review]
Patch part.4 Sort out drag handling code of nsFrame

Review of attachment 548716 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +2210,5 @@
> + *              aScrollableFrame scroll port.
> + *              One or two flags of CURSOR_ON_*_EDGE if cursor is on the edge
> + *              of the scroll port.
> + *              Otherwise, i.e., if cursor is outside the scroll port,
> + *              one ore two flags of CURSOR_OUTSIDE_*.

This comment makes it sound like INSIDE takes priority over OUTSIDE. But actually OUTSIDE takes priority over INSIDE.

But it might be simpler to just let ComputeCursorLocatedInScrollPort return all relevant bits and let the caller be responsible for choosing priorities.

@@ +2258,5 @@
> +  } else if (scrollPortRect.x + edgeH > aCursorPosInScrollableFrame.x) {
> +    result |= CURSOR_ON_LEFT_EDGE;
> +  } else if (scrollPortRect.XMost() < aCursorPosInScrollableFrame.x) {
> +    result |= CURSOR_OUTSIDE_RIGHT;
> +  } else if (scrollPortRect.XMost() - edgeH < aCursorPosInScrollableFrame.x) {

Why are these all "else" clauses? Shouldn't you set all the relevant bits?

@@ +2271,5 @@
> +    result |= CURSOR_ON_TOP_EDGE;
> +  } else if (frameRect.YMost() < aCursorPosInScrollableFrame.y) {
> +    result |= CURSOR_OUTSIDE_BOTTOM;
> +  } else if (frameRect.YMost() - edgeV <
> +               aCursorPosInScrollableFrame.y) {

Again, why are these all "else" clauses?
(In reply to comment #13)

First, I should explain the terms of the comments.

         +------------------------------------------------------------+
         |      ^                                                     |
         |      v  32px                                               |
         |<---->+----------------------------------------------+      |
         | 32px |                                              |      |
         |      |                                              |      |
         |      |                                              |      |
-------->|<---->|<------------------INSIDE_EDGES ------------->|      |
OUTSIDE    EDGE

"outside" means the cursor is outside the scrollport. "edge" is the area close to scrollport edge. And "inside edges" is the internal area surrounded by "edge"s.

So, each area doesn't include the others. I.e., there is no case such as the (CURSOR_ON_TOP_EDGE | CURSOR_ON_BOTTOM_EDGE) or (CURSOR_ON_TOP_EDGE | CURSOR_OUTSIDE_TOP).

> Comment on attachment 548716 [details] [diff] [review] [diff] [details] [review]
> ::: layout/generic/nsFrame.cpp
> @@ +2210,5 @@
> > + *              aScrollableFrame scroll port.
> > + *              One or two flags of CURSOR_ON_*_EDGE if cursor is on the edge
> > + *              of the scroll port.
> > + *              Otherwise, i.e., if cursor is outside the scroll port,
> > + *              one ore two flags of CURSOR_OUTSIDE_*.
> 
> This comment makes it sound like INSIDE takes priority over OUTSIDE. But
> actually OUTSIDE takes priority over INSIDE.
> 
> But it might be simpler to just let ComputeCursorLocatedInScrollPort return
> all relevant bits and let the caller be responsible for choosing priorities.

Therefore, if the cursor is located outside the scrollport, it shouldn't return CURSOR_ON_*_EDGE, it doesn't make sense for callers.

> @@ +2258,5 @@
> > +  } else if (scrollPortRect.x + edgeH > aCursorPosInScrollableFrame.x) {
> > +    result |= CURSOR_ON_LEFT_EDGE;
> > +  } else if (scrollPortRect.XMost() < aCursorPosInScrollableFrame.x) {
> > +    result |= CURSOR_OUTSIDE_RIGHT;
> > +  } else if (scrollPortRect.XMost() - edgeH < aCursorPosInScrollableFrame.x) {
> 
> Why are these all "else" clauses? Shouldn't you set all the relevant bits?
> 
> @@ +2271,5 @@
> > +    result |= CURSOR_ON_TOP_EDGE;
> > +  } else if (frameRect.YMost() < aCursorPosInScrollableFrame.y) {
> > +    result |= CURSOR_OUTSIDE_BOTTOM;
> > +  } else if (frameRect.YMost() - edgeV <
> > +               aCursorPosInScrollableFrame.y) {
> 
> Again, why are these all "else" clauses?

The "else"s reduce redundant checks like:

if (scrollPortRect.x > aCursorPosInScrollableFrame.x) {
  result |= CURSOR_OUTSIDE_LEFT;
}
if (scrollPortRect.x <= aCursorPosInScrollableFrame.x &&
    scrollPortRect.x + edgeH > aCursorPosInScrollableFrame.x) {
  result |= CURSOR_ON_LEFT_EDGE;
}
Comment on attachment 548716 [details] [diff] [review]
Patch part.4 Sort out drag handling code of nsFrame

Review of attachment 548716 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +2264,5 @@
> +  }
> +
> +  nsRect frameRect = scrollableFrame->GetRect();
> +  frameRect.MoveTo(0, 0);
> +  if (frameRect.y > aCursorPosInScrollableFrame.y) {

Why are you using scrollableFrame->GetRect() here but GetScrollPortRect above? seems like you should use the same rect in both cases.
(In reply to comment #15)
> Comment on attachment 548716 [details] [diff] [review] [diff] [details] [review]
> Patch part.4 Sort out drag handling code of nsFrame
> 
> Review of attachment 548716 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsFrame.cpp
> @@ +2264,5 @@
> > +  }
> > +
> > +  nsRect frameRect = scrollableFrame->GetRect();
> > +  frameRect.MoveTo(0, 0);
> > +  if (frameRect.y > aCursorPosInScrollableFrame.y) {
> 
> Why are you using scrollableFrame->GetRect() here but GetScrollPortRect
> above? seems like you should use the same rect in both cases.

Oh, sure. It's just my mistake.
Status: NEW → ASSIGNED
I realized why I did the mistake.

We need to use the frame rect for "outside" check because "edge" includes scrollbars and borders of the frame.

This patch explains each words clearly.
Attachment #550585 - Flags: review?(roc)
Attachment #548716 - Attachment is obsolete: true
Attachment #548716 - Flags: review?(roc)
Comment on attachment 550585 [details] [diff] [review]
Patch part.4 Sort out drag handling code of nsFrame

Review of attachment 550585 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +2222,5 @@
> + *            | |      |                     |      |    | |
> + *            | |      |                     |      |    | |
> + *            | |      |                     |      |    | |
> + * ----------> <------> <-------------------> <-----------> <------------
> + *    outside     edge    inside-of-edges         edge         outside

Great diagram!

@@ +2226,5 @@
> + *    outside     edge    inside-of-edges         edge         outside
> + *
> + * outside, edge and inside-edges are different areas. They must not overlap.
> + *
> + * "edge" is close area from border of scroll port.  The distance is defined by

I'm not sure what you mean about "edge" here.

@@ +2233,5 @@
> + *
> + * "inside-of-edges" is internal area which is surrounded by all "edge"s.
> + *
> + * "outside" is outside of the scrollable frame.  I.e., it's margin or outer of
> + * margin box.

"or outside of margin box"

@@ +2314,5 @@
> +
> +    return result;
> +  }
> +
> +  if (scrollableFrameRect.XMost() < aCursorPosInScrollableFrame.x) {

<= I think

@@ +2320,5 @@
> +  } else if (scrollableFrameRect.x > aCursorPosInScrollableFrame.x) {
> +    result |= CURSOR_OUTSIDE_LEFT;
> +  }
> +
> +  if (scrollableFrameRect.YMost() < aCursorPosInScrollableFrame.y) {

<= here too

@@ +2335,5 @@
> + * aScrollableFrame from current scroll position.
> + *
> + * @param aScrollableFrame      A scrollable frame
> + * @return      SCROLLABLE_TO_NOWHERE if aScrollableFrame isn't scrollable.
> + *              Otherwise, one or two other SCROLLABLE_TO_* flags.

It can return all four flags at once, can't it?

@@ +2342,5 @@
> +  SCROLLABLE_TO_NOWHERE = 0,
> +  SCROLLABLE_TO_TOP     = 1,
> +  SCROLLABLE_TO_BOTTOM  = 2,
> +  SCROLLABLE_TO_LEFT    = 4,
> +  SCROLLABLE_TO_RIGHT   = 8

Since we're not going to scroll all the way to the top or bottom, I think probably better names would be SCROLLABLE_NOWHERE, SCROLLABLE_UP, SCROLLABLE_DOWN, SCROLLABLE_LEFT, SCROLLABLE_RIGHT.

@@ +2356,5 @@
> +  if (scrollRange.x < scrollPosition.x) {
> +    result |= SCROLLABLE_TO_LEFT;
> +  }
> +
> +  if (scrollRange.x + scrollRange.width > scrollPosition.x) {

scrollRange.XMost()

@@ +2364,5 @@
> +  if (scrollRange.y < scrollPosition.y) {
> +    result |= SCROLLABLE_TO_TOP;
> +  }
> +
> +  if (scrollRange.y + scrollRange.height > scrollPosition.y) {

scrollRange.YMost()

@@ +2387,5 @@
> + * @return      SCROLLABLE_TO_NOWHERE if aScrollableFrame isn't scrollable.
> + *              Otherwise, one or two other SCROLLABLE_TO_* flags.
> + */
> +static PRUint32
> +ComputeActuallScrollableDirection(nsIScrollableFrame* aScrollableFrame,

"Actual"

However, it's hard to distinguish this function from ComputeScrollableDirection just based on the name. It might be better to just inline ComputeScrollableDirection into this function and call this function ComputeScrollableDirection. Actually "ComputeScrollableDirections" would be even better since multiple directions can be returned.

@@ +2430,5 @@
> + * aScrollableFrame.  If aScrollableFrame isn't large enough, returns smaller
> + * value than pref value.
> + */
> +static nsSize
> +ComputeOnEdgeScrollAmout(nsIScrollableFrame* aScrollableFrame)

ComputeEdgeScrollAmount is probably a better name.

@@ +2438,5 @@
> +
> +  nsPresContext* pc = scrollableFrame->PresContext();
> +  nscoord onePixel = pc->DevPixelsToAppUnits(1);
> +
> +  // The scrolling mouse is defined by pref, however, if the amount is

// The scroll amount
> @@ +2335,5 @@
>> + * aScrollableFrame from current scroll position.
>> + *
>> + * @param aScrollableFrame      A scrollable frame
>> + * @return      SCROLLABLE_TO_NOWHERE if aScrollableFrame isn't scrollable.
>> + *              Otherwise, one or two other SCROLLABLE_TO_* flags.
> 
> It can return all four flags at once, can't it?

No. The function computes the scrollable directions for the mouse cursor position. E.g., when mouse cursor is on both left edge and bottom edge, the result never contains SCROLLABLE_TOP and SCROLLABLE_RIGHT because the auto scrolling shouldn't do scroll to such direction at that time. I guess there are some better names but I have no idea.
# I added some comments for explaining the reason.
Attachment #550585 - Attachment is obsolete: true
Attachment #553119 - Flags: review?(roc)
Attachment #550585 - Flags: review?(roc)
Comment on attachment 548715 [details] [diff] [review]
Patch part.3 Use synthesized mousemove event for auto scroll at dragging


>+   * NotifyCapturingContentReleased() is called when capturing content is
>+   * released.  The called instance is the prefShell to have been capturing
prefShell?

>+nsFrame::DoAutoScroll(nsIFrame* aScrolledFrame,
>+                      nsFrameSelection* aFrameSelection,
>+                      const nsPoint &aScrollTo)
>+{
>+  nsIPresShell* ps = aFrameSelection->GetShell();
>+  ps->ScrollFrameRectIntoView(aScrolledFrame, nsRect(aScrollTo, nsSize(1,1)),
>+                              NS_PRESSHELL_SCROLL_ANYWHERE,
>+                              NS_PRESSHELL_SCROLL_ANYWHERE, 0);
Is it guaranteed that aFrameSelection and ps stay alive while calling 
ScrollFrameRectIntoView?
Based on // Note: DoAutoScroll might destroy arbitrary frames etc.
no, so this could lead to crashes.

>+
>+  const PRUint32 kAutoScrollTimerDelay = 30;
>+  aFrameSelection->StartAutoScrollTimer(kAutoScrollTimerDelay);
>+}
It is rather strange to have DoAutoScroll in nsFrame, yet it doesn't do
anything with 'this' object.
Attachment #548715 - Flags: review?(Olli.Pettay) → review-
Comment on attachment 548718 [details] [diff] [review]
Patch part.5 Ancestor scrollable elements of selection root element should be scrollable by on-edge-scrolling

>@@ -2849,21 +2878,16 @@ nsFrame::ExpandSelectionByMouseMove(nsFr
>   if (!capturingContent) {
>     return NS_OK;  // The capture was canceled.
>   }
>   nsIContent* selectionRoot =
>     GetSelectionRootContentForCapturingContent(aPresShell, capturingContent);
> 
>   nsIScrollableFrame* scrollableFrame =
>     FindNearestScrollableFrameForSelection(this, selectionRoot);
>-  // If a non-scrollable content captures by script and there is no scrollable
>-  // frame between the selection root and this, we don't need to do anymore.
>-  if (!scrollableFrame) {
>-    return NS_OK;
>-  }
Could you explain why this change is ok.
The patch is missing context to see whether null scrollableFrame is ok.


> 
>   if (!handleTableSelection) {
>     nsIScrollableFrame* selectionRootScrollableFrame =
>       FindNearestScrollableFrameForSelection(selectionRoot->GetPrimaryFrame(),
>                                              selectionRoot);
>     while (scrollableFrame) {
>       nsIFrame* frame = do_QueryFrame(scrollableFrame);
>       nsPoint cursorPosInScrollableFrame =
>@@ -2875,16 +2899,84 @@ nsFrame::ExpandSelectionByMouseMove(nsFr
>                      scrollTo);
>         return NS_OK;
>       }
> 
>       scrollableFrame =
>         FindNearestScrollableFrameForSelection(frame->GetParent(),
>                                                selectionRoot);
>     }
>+
>+    // If mouse cursor is in selection root's frame rect but not on any
>+    // scrollable element's edge in the selection root element, some ancestor
>+    // frames might hide this frame.  Then, the ancestors should be also
>+    // scrollabe by on-edge-scroll.
scrollable


>+    nsIFrame* selectionRootFrame = do_QueryFrame(selectionRootScrollableFrame);
>+    if (!selectionRootFrame) {
>+      selectionRootFrame = selectionRoot->GetPrimaryFrame();
>+      if (!selectionRootFrame) {
>+        return NS_OK;
>+      }
>+    }
>+    nsRect selectionRootFrameRect = selectionRootFrame->GetRect();
>+    selectionRootFrameRect.MoveTo(0, 0);
>+    nsPoint cursorPosInSelectionRootFrame =
>+      nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, selectionRootFrame);
>+    if (selectionRootFrameRect.Contains(cursorPosInSelectionRootFrame)){
Missing space before {
Attachment #548718 - Flags: review?(Olli.Pettay) → review+
(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 548718 [details] [diff] [review]
> Patch part.5 Ancestor scrollable elements of selection root element should
> be scrollable by on-edge-scrolling
> 
> >@@ -2849,21 +2878,16 @@ nsFrame::ExpandSelectionByMouseMove(nsFr
> >   if (!capturingContent) {
> >     return NS_OK;  // The capture was canceled.
> >   }
> >   nsIContent* selectionRoot =
> >     GetSelectionRootContentForCapturingContent(aPresShell, capturingContent);
> > 
> >   nsIScrollableFrame* scrollableFrame =
> >     FindNearestScrollableFrameForSelection(this, selectionRoot);
> >-  // If a non-scrollable content captures by script and there is no scrollable
> >-  // frame between the selection root and this, we don't need to do anymore.
> >-  if (!scrollableFrame) {
> >-    return NS_OK;
> >-  }
> Could you explain why this change is ok.
> The patch is missing context to see whether null scrollableFrame is ok.

For example, even if mousedowned iframe's document isn't scrollable, the iframe might be clipped by its ancestor scollable element.  At that time, the clipping ancestor should be scrolled when mouse cursor is moved to its edge and cursor is still in the iframe.  I.e., added code which is after the removing part should be run at that time.
Right. I moved back DoAutoScroll() to nsFrameSelection.
Attachment #548715 - Attachment is obsolete: true
Attachment #556532 - Flags: review?(Olli.Pettay)
Ah, StartAutoScrollTimer() can be protected (or private) method...
Comment on attachment 556532 [details] [diff] [review]
Patch part.3 Use synthesized mousemove event for auto scroll at dragging

>-  aFrameSelection->StartAutoScrollTimer(scrolledFrame, scrollTo,
>-                                        kAutoScrollTimerDelay);
>+  // NOTE: DoAutoScroll() might destroy this frame and something.
Perhaps drop "and something"

>+    if (!mTimer) {
>+      nsresult rv;
>+      mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
>+      NS_ENSURE_SUCCESS(rv, );
NS_ENSURE_SUCCESS without latter parameter looks strange.
please use
if (NS_FAILED(rv)) {
  return;
}


>+nsFrameSelection::DoAutoScroll(nsIFrame* aScrolledFrame,
>+                               const nsPoint &aScrollTo)
>+{
>+  NS_ENSURE_TRUE(mShell, );
>+  NS_ASSERTION(this == sDraggingFrameSelection,
>+               "Why nsFrameSelection::DoAutoScroll() is called?");
>+
>+  mShell->ScrollFrameRectIntoView(aScrolledFrame,
>+                                  nsRect(aScrollTo, nsSize(1,1)),
>+                                  NS_PRESSHELL_SCROLL_ANYWHERE,
>+                                  NS_PRESSHELL_SCROLL_ANYWHERE, 0);
>+
>+  if (!sDraggingFrameSelection) {
>+    return; // destroyed.
>+  }
What if sDraggingFrameSelection just points to some other selection object here?


The old code stops timer when DoAutoScroll is called. Why is that not needed anymore?
Attachment #556532 - Flags: review?(Olli.Pettay) → review-
Okay, this patch checks the auto scrolling state strictly.

> The old code stops timer when DoAutoScroll is called. Why is that not needed anymore?

nsFrame::EndSelectionChangeByMouse() and nsFrameSelection::AbortDragForSelection() calls nsFrameSelection::StopAutoScroll() in new code. I think that it's enough.

However, we can do same thing for safer change.
Attachment #556532 - Attachment is obsolete: true
Attachment #556804 - Flags: review?(Olli.Pettay)
Comment on attachment 556804 [details] [diff] [review]
Patch part.3 Use synthesized mousemove event for auto scroll at dragging

Could you post all these scrolling/selection related patches to
tryserver (including patches in other bugs).
I'd like to try them out.
Attachment #556804 - Flags: review?(Olli.Pettay) → review+
Attachment #556804 - Flags: superreview?(roc)
Attachment #556804 - Flags: superreview?(roc) → superreview+
Component: Event Handling → User events and focus handling

Resetting assignee which I don't work on in this several months.

Assignee: masayuki → nobody
Status: ASSIGNED → NEW

Although bug 552707 is no more reproducible, but this bug has not been fixed yet.

Severity: minor → S3
Component: DOM: UI Events & Focus Handling → DOM: Copy & Paste and Drag & Drop
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: