817820 - Defect - Cursor should change to reflect that "middle-click scrolling" (autoscroll) is active

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Input, defect, P2)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

On desktop and metro Firefox, middle-clicking causes the browser to enter a state where moving the mouse scrolls the page.

On desktop Firefox, we change the cursor to reflect that we are in "scroll up/down" or "scroll up/down/left/right" mode.

On Metro Firefox the cursor does not change, so it is not immediately obvious that we have entered this special scrolling mode.

We should update the cursor to reflect whether we are in a special "scroll on mouse move" mode.

Comment 1

[Jim Mathies [:jimm]]

We need to provide a themed autoscroll element toolkit can lookup via getElementById. Currently it ties to create a popup child window, which fails.

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#850

http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/global.css#299

Comment 2

[Asa Dotzler [:asa]]

Or we simply disable auto-scroll for Metro.

Comment 3

[Allison Naaktgeboren :ally]

Disabling the mousescroll invalidates bugs like Bug 811429. 

Will this be a problem for our testers who don't have touch enabled?

Should we kill autoscroll for v1?

Comment 4

[Jim Mathies [:jimm]]

All we need here is an image. If we want, we can steal desktops. Don't see why we wouldn't try to get this in, especially since a majority of our initial user base is using the mouse.

Comment 5

[Matt Brubeck (:mbrubeck)]

(In reply to :Ally Naaktgeboren from comment #3)
> Disabling the mousescroll invalidates bugs like Bug 811429.

Auto-scroll and wheel scroll are separate, though I think they share some code/events under the hood.  We don't need any images to support wheel scroll.

I agree with jimm that this is a good easy polish fix.  Probably a P3 or P4, maybe a good starter bug for someone.

Comment 6

[Brian R. Bondy [:bbondy]]

p=1

Comment 7

[Marina Samuel [:emtwo]]

Attachment: v1: Add middle-click autoscroller icon in metro.

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 760445 [details] [diff] [review]
v1: Add middle-click autoscroller icon in metro.

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

Thanks, this is working great in my testing!  I'd just like to change where some of it is implemented in order to reduce coupling a bit.  See below for details.

::: browser/metro/theme/browser.css
@@ +1031,5 @@
> +  background-origin: padding-box;
> +  background-clip: padding-box;
> +%ifdef XP_WIN
> +  background-position: right top;
> +%endif

Please remove the ifdef here, and just use the "XP_WIN" style unconditionally.  (For Metro Firefox, we don't need to style things differently for non-Windows platforms.)

@@ +1040,5 @@
> +%ifdef XP_WIN
> +  background-position: right center;
> +%else
> +  background-position: left center;
> +%endif

Same here -- keep the "right center" line and remove the "else" case.  And similarly, below.

::: toolkit/content/widgets/browser.xml
@@ +575,5 @@
> +                if (this._isMetroBrowser) {
> +                  this._autoScrollPopup.hidden = "true";
> +                } else {
> +                  this._autoScrollPopup.hidePopup();
> +                }

I'd like to avoid adding Metro-specific code to this toolkit binding, and I suspect the toolkit peers would too.  I'd prefer one of these two approaches:

1) Move the code that touches the autoscroll popup into some isolated methods in toolkit's browser.xml, then override those in Metro's browser.xml, or

2) give our "popup" element a small XBL binding that implements the showPopup and hidePopup methods from nsIDOMXULPopupElement, so it will act like a normal popup and work with the existing browser binding code.

I think I like the second option better, but feel free to implement whichever one seems simpler.  Let me know if you need any clarification or whatnot.

Comment 9

[Marina Samuel [:emtwo]]

Attachment: v2: Add middle-click autoscroller icon in metro.

Implemented the 2nd approach described in the last comment.

Comment 10

[Marina Samuel [:emtwo]]

Attachment: v2: Add middle-click autoscroller icon in metro.

Implemented the 2nd approach described in the last comment.

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 761044 [details] [diff] [review]
v2: Add middle-click autoscroller icon in metro.

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

Thanks!

::: browser/metro/base/content/bindings/popup.xml
@@ +3,5 @@
> +<bindings xmlns="http://www.mozilla.org/xbl"
> +          xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +  <binding id="element-popup">
> +
> +    <implementation inherits="nsIPopupBoxObject">

"inherits" should be "implements" here.

Comment 12

[Marina Samuel [:emtwo]]

pushed to m-i https://hg.mozilla.org/integration/mozilla-inbound/rev/7f78ad0debff

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7f78ad0debff

Comment 14

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130625 Firefox/25.0

Verified that the middle-click autoscroller icon is displayed when mouse middle button is clicked and the page scrolls correctly, both horizontally and vertically

Comment 15

[Asa Dotzler [:asa]]

This doesn't map to a story we specified because we inherited the feature and have no story for it.