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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: alice0775, Assigned: masayuki)

Tracking

(Depends on 2 bugs)

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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.
Duplicate of this bug: 645547
Summary: Drag selection does not work properly in fullscreen mode → Drag selection scrolling does not work properly in fullscreen mode
OS: Windows 7 → All
Duplicate of this bug: 645855
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
Assignee: nobody → chrisccoulson
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
Attachment #522841 - Attachment is obsolete: true
Attachment #523611 - Flags: review?(roc)
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!
Updated patch
Attachment #523611 - Attachment is obsolete: true
Attachment #523611 - Flags: review?(roc)
Attachment #525563 - Flags: review?(roc)
Duplicate of this bug: 650678
Duplicate of this bug: 636206
Duplicate of this bug: 658160
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.
Summary: Drag selection scrolling does not work properly in fullscreen mode → Drag selection scrolling does not work properly in fullscreen and maximized mode
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.
Assignee: chrisccoulson → masayuki
Status: NEW → ASSIGNED
Depends on: 552707
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.
Posted patch Patch v3.0 (obsolete) — Splinter Review
testing on tryserver...
Attachment #525563 - Attachment is obsolete: true
Posted patch Patch v3.0.1 (obsolete) — Splinter Review
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.
Attachment #544683 - Attachment is obsolete: true
Attachment #544738 - Flags: ui-review?(faaborg)
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 [details] [diff] [review]
Patch v3.0.1

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

sorry about the delay, approach sounds good
Attachment #544738 - Flags: ui-review?(faaborg) → ui-review+
Posted patch Patch v3.1Splinter Review
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.
Attachment #544738 - Attachment is obsolete: true
Attachment #546015 - Flags: review?(roc)
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?
Attachment #546015 - Flags: review?(roc) → review+
(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 :-(.
This (along with most things committed on Friday afternoon) was backed out of mozilla-inbound in order to clear up orange.
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/42e432ef2bc4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Depends on: 672181
Depends on: 672398
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla8 → mozilla9
@masayuki see bug 623177 too

thanks
Duplicate of this bug: 708477
Blocks: 629673
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?
Target Milestone: mozilla9 → ---
Masayuki,

Would you re-land the patch anytime soon?
No. There are some issues which must be fixed before landing.
Duplicate of this bug: 629673
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 :(
Duplicate of this bug: 909189
Duplicate of this bug: 959238
Duplicate of this bug: 997925
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.
Duplicate of this bug: 113294
This was fixed by Bug 623432
Status: REOPENED → RESOLVED
Closed: 8 years ago3 years ago
Resolution: --- → WORKSFORME
Resolution: WORKSFORME → FIXED
You need to log in before you can comment on or make changes to this bug.