644621 - Drag selection scrolling does not work properly in fullscreen and maximized mode

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Alice0775 White]

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.

Comment 3

[Chris Coulson]

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

Comment 4

[Chris Coulson]

Attachment: Fix selection autoscroll in fullscreen

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 6

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

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

Comment 7

[Chris Coulson]

(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

Comment 8

[Chris Coulson]

Attachment: Fix selection autoscroll in fullscreen

Comment 9

[Chris Coulson]

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

Comment 10

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

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

Comment 11

[Alice0775 White]

This also related to Bug 636206

Comment 12

[Chris Coulson]

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

Comment 13

[Chris Coulson]

Attachment: Fix selection autoscroll in fullscreen (v2)

Updated patch

Comment 17

[Caspy7]

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.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v3.0

testing on tryserver...

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 25

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 544738 [details] [diff] [review]
Patch v3.0.1

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

sorry about the delay, approach sounds good

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

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

Comment on attachment 546015 [details] [diff] [review]
Patch v3.1

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

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?

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 30

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

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

Comment 31

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

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

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

backed out, I'll reland it soon.

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

relanded. thank you for your quick response, roc!

Comment 34

[Joe Drew (not getting mail)]

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

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 36

[Joe Drew (not getting mail)]

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

Comment 37

[Giorgio]

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)

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 40

[Giorgio]

@masayuki see bug 623177 too

thanks

Comment 42

[alex_mayorga]

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?

Comment 43

[alex_mayorga]

Masayuki,

Would you re-land the patch anytime soon?

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 46

[MestreLion]

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?

Comment 47

[LL25255252]

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/

Comment 48

[Alice0775 White]

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

Comment 52

[LL25255252]

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

Comment 53

[Elbart]

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.

Comment 54

[LL25255252]

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

Comment 55

[james0r]

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.

Comment 56

[Bruno G.]

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

Comment 57

[Nicolas Barbulesco]

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

Comment 59

[Alice0775 White]

This was fixed by Bug 623432