Firefox doesn't auto scroll when selecting content downwards

Build Identifier:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.2a1pre) Gecko/20110324
Firefox/4.2a1pre ID:20110324030442

Drag selection does not work properly in fullscreen mode.

Drag selection upward : scroll down is very slow
Drag selection downward : scroll stops at the bottom of screen.

Steps to Reproduce:
1. Start Minefield with new profile
2. Open Web page (long enough to scroll)
3. Selection start by mouse down and hold the mouse
4. Drag selection downward or upward
Actual Results:
  Drag selection downward : Scroll tops at the bottom of screen.
  Drag selection upward : Scroll speed is slightly slow.

Expected Results:
  The selection should be expanded.

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

I'm forwarding this from https://launchpad.net/bugs/744580:

Steps to reproduce:
Open firefox 4
Go full screen
Open a long page such as this one:- http://mail.gnome.org/archives/commits-list/2011-March/msg03805.html
Try to highlight a large chunk of text starting on screen and move the mouse down and try to go off the bottom of the screen (selecting text). Note that the browser doesn't scroll down. This is the bug.

Note, that if the window is not maximized, you will notice that auto-scrolling does eventually happen - but not until the pointer is outside of the window (which breaks completely for maximized windows)

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

After doing some digging today, I realised that this only ever worked in 3.6 because the statusbar made it possible for the pointer to travel beyond the end of the scrolled area.

This is caused because nsTypedSelection::DoAutoScroll calls PresShell::ScrollFrameRectIntoView with a 1x1 nsRect (in app units). This means that the pointer needs to travel beyond the edge of the scrolled area before that nsRect is out of view (which is what triggers scrolling).

To fix this, you can make the nsRect passed to ScrollFrameRectIntoView a bit larger. I've attached a patch which does this, which ensures that the nsRect starts to go out of view before the pointer reaches the edge of the scrolled area

Created attachment 522841
Fix selection autoscroll in fullscreen

The patch seems to work nicely, but needs tests.
roc or mats could review it.

Probably simpler to use nsIntRect(...).ToAppUnits(...);

(In reply to comment #6)
> Probably simpler to use nsIntRect(...).ToAppUnits(...);

Thanks, I didn't realise that existed. In any case, as aPoint is already in app units, it doesn't seem to make things any easier.

I'm attaching an updated patch now, slightly tidied and with a test case

Created attachment 523611
Fix selection autoscroll in fullscreen

Note, I've also verified the test fails on a current Firefox build (without the patch)

+ nscoord oneDevPixel = presContext->DevPixelsToAppUnits(1);
+ nsPoint offsetPoint = aPoint - nsPoint(oneDevPixel * 20, oneDevPixel * 20);
+ nsSize rectSize = nsSize(oneDevPixel * 40, oneDevPixel * 40);
   PRBool didScroll = presContext->PresShell()->
+ ScrollFrameRectIntoView(aFrame, nsRect(offsetPoint, rectSize),

Why not
  nsRect r = nsIntRect(-20, -20, 40, 40).
    ToAppUnits(presContext->AppUnitsPerDevPixel());
  PRBool didScroll = presContext->PresShell()->
    ScrollFrameRectIntoView(aFrame, r + aPoint);

Sure looks simpler to me!

This also related to Bug 636206

(In reply to comment #10)
> + nscoord oneDevPixel = presContext->DevPixelsToAppUnits(1);
> + nsPoint offsetPoint = aPoint - nsPoint(oneDevPixel * 20, oneDevPixel * 20);
> + nsSize rectSize = nsSize(oneDevPixel * 40, oneDevPixel * 40);
> PRBool didScroll = presContext->PresShell()->
> + ScrollFrameRectIntoView(aFrame, nsRect(offsetPoint, rectSize),
>
> Why not
> nsRect r = nsIntRect(-20, -20, 40, 40).
> ToAppUnits(presContext->AppUnitsPerDevPixel());
> PRBool didScroll = presContext->PresShell()->
> ScrollFrameRectIntoView(aFrame, r + aPoint);
>
> Sure looks simpler to me!

Yes, you're right. Thanks!

Created attachment 525563
Fix selection autoscroll in fullscreen (v2)

Updated patch

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

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

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

Seems the duplicates have noted this, dunno if it should be added to the description/title, but this behavior is also observed when the window is maximized (and there is no UI at the bottom).

Also can I asked the status of this bug? It looks like it was mostly fixed in April. Is it at a place it can land?

P.S. Hope it's alright that went ahead I updated the title.

Um, this patch may conflict with bug 552707. And I think that the body element should be scrollable on its edge of bug 552707's patch.

My bug 552707 patch is waiting final review of smaug. The patch has been already passed ui-review and roc's first review. And even if this patch would be landed first, I needed to backout of this change and need to change my patch for scrolling body on its edge. Therefore, I'd like you wait bug 552707 fix and recreate your patch based on my patch. If so, the behavior will be standardized on all situations.

I landed bug 552707 on inbound. So, the patch shouldn't be used. I'll post a new patch ASAP.

Um, I have a question.

Should this bug be fixed on all scrollable elements? I.e., should <input> and <textarea> be also scrolled when mouse cursor is on their edge?

For a11y, I think that they should be scrollable too, though. Chris's patch looks so. But I'm not sure whether it was intentional.

Created attachment 544683
Patch v3.0

testing on tryserver...

Created attachment 544738
Patch v3.0.1

I think this needs ui-review.

Users can scroll any selection root elements while they move mouse cursor on their edge. This means that user can scroll scrollable element always if all of it is visible.

If mouse cursor is on edge, the scrolling speed is always same which is set by prefs. If user moved the mouse cursor to outside, user would see slower scrolling than edge scrolling. I think this is odd. Therefore this patch also use on-edge scrolling speed as minimum speed of auto scrolling while mouse cursor is outside of the element. When the scrolling speed is faster than on-edge scrolling speed (i.e., user moves mouse cursor to far from the element), it doesn't use the on-edge scrolling speed.

Another proposal: Only when mouse cursor is on current screen edge, the selection root scrollable element can be scrolled on its edge. However, this would test harder than current patch. And it may confuse users to change the behavior depending on the window position/size in screen.

Comment on attachment 544738
Patch v3.0.1

Review of attachment 544738:
-----------------------------------------------------------------

sorry about the delay, approach sounds good

Created attachment 546015
Patch v3.1

Thank you, faaborg.

# fix a nit from previous patch

Note that the pref names are changed since they're used for selection root element too.

Comment on attachment 546015
Patch v3.1

Review of attachment 546015:
-----------------------------------------------------------------

r+ with that fixed

::: layout/generic/nsFrame.cpp
@@ +2663,5 @@
> +
> + // We should set minimum scroll speed same as the on-edge scrolling speed.
> + // E.g., while mouse cursor is on the edge, scrolling speed is always same.
> + // However, when the user moves 1px outside of the selection root element,
> + // the scrolling speed is slow down. We should avoid this strange behavior.

Remove the last two lines of this comment, since they're confusing; it sounds like you're describing something that actually happens with this patch applied.

@@ +2666,5 @@
> + // However, when the user moves 1px outside of the selection root element,
> + // the scrolling speed is slow down. We should avoid this strange behavior.
> + nsPoint currentScrollPos = scrollableFrame->GetScrollPosition();
> + nsRect visibleRectOfScrolledFrame = scrollableFrame->GetScrollPortRect();
> + visibleRectOfScrolledFrame.MoveTo(currentScrollPos);

This should be scrollableFrame->GetScrollPortRect() + curentScrollPos, right?

http://hg.mozilla.org/integration/mozilla-inbound/rev/56fd5d1934b7

(In reply to comment #27)
> > + nsPoint currentScrollPos = scrollableFrame->GetScrollPosition();
> > + nsRect visibleRectOfScrolledFrame = scrollableFrame->GetScrollPortRect();
> > + visibleRectOfScrolledFrame.MoveTo(currentScrollPos);
>
> This should be scrollableFrame->GetScrollPortRect() + curentScrollPos, right?

Oops, I changed to:

> 1.62 + nsRect visibleRectOfScrolledFrame =
> 1.63 + scrollableFrame->GetScrollPortRect() + scrollableFrame->GetScrollPosition();

This is same as my original code only when GetScrollPortRect() is positioned 0, 0. Is that always true??

(In reply to comment #29)
> This is same as my original code only when GetScrollPortRect() is positioned
> 0, 0. Is that always true??

It's usually true, but not always. Sometimes we have scrollbars on the left so the scrollport x is > 0. Also, when the scrolled element has a left border, the scrollport x is > 0.

Oh I see. Your original code was right and my suggestion was wrong. Please fix it, sorry :-(.

http://hg.mozilla.org/integration/mozilla-inbound/rev/0cf0239724c2

backed out, I'll reland it soon.

http://hg.mozilla.org/integration/mozilla-inbound/rev/28127a75bb29

relanded. thank you for your quick response, roc!

This (along with most things committed on Friday afternoon) was backed out of mozilla-inbound in order to clear up orange.

http://hg.mozilla.org/integration/mozilla-inbound/rev/42e432ef2bc4

http://hg.mozilla.org/mozilla-central/rev/42e432ef2bc4

i think it is better to remove the acceleration given by the distance from the area to scroll

trigger area and speed should be constant and slow independently from mouse position

but if user "shakes" the mouse the speed must increase (this is currently implemented and works good)

(In reply to comment #37)
> but if user "shakes" the mouse the speed must increase (this is currently
> implemented and works good)

I don't think so, see bug 672181.

Temporarily, backed out for risk management of mozilla8, see bug 675865.

@masayuki see bug 623177 too

thanks

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

Still a problem on Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:13.0) Gecko/20120221 Firefox/13.0a1 ID:20120221031301

Should "Target Milestone" be changed?

Masayuki,

Would you re-land the patch anytime soon?

No. There are some issues which must be fixed before landing.

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

After more than an year, it's time for a friendly ping:

Is there any schedule for the patch to land, or any other solution or workaround for this bug?

Until this bug is fixed you can use one of these workarounds:

- Use shift-arrow-down instead to select the text
- Select the text starting from the bottom - go upwards.
- Make the addon bar visible
- Make the find bar visible
- These addons can put a toolbar at the bottom:
  https://addons.mozilla.org/en-US/firefox/addon/split-pannel/
  https://addons.mozilla.org/en-US/firefox/addon/searchbastard/

Findbar is now top in Nightly24.0a1.
Australis removed the addon bar and Australis will land in near future.

The probability to encounter with this bug becomes higher :(

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

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

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

Another workaround which works only on FF29 and later is to install the "The Addon Bar (restored)" addon.

The addonbar is hidden in fullscreen-mode.

The only proper workaround is the bug being fixed.
It's only 4.5 years after the regression, give it a year or two.

@Elbart, You are correct that the addonbar is hidden in fullscreen-mode but the bug happens also in maximized-window mode so at least in this mode that workaround will work.

Firefox user for about 6 months. Didn't encounter this issue prior to Firefox 29 probably because of the add-on bar being enabled in previous versions. Adding this comment to hopefully to raise urgency of a fix. Being able to scroll while selecting on a netbook or smaller screen is pretty critical.

Not sure how to do it properly, but bug #157456 seems a duplicate of this one...

The scope of the bug 157456 is wider. The occurrences of the bug are not limited to full screen.

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

This was fixed by Bug 623432

Well, it's been 2 years after that comment.
The bug is still there...