Closed
Bug 817820
Opened 12 years ago
Closed 11 years ago
Defect - Cursor should change to reflect that "middle-click scrolling" (autoscroll) is active
Categories
(Firefox for Metro Graveyard :: Input, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: TimAbraldes, Assigned: emtwo)
References
Details
(Whiteboard: [mentor=mbrubeck@mozilla.com][good first bug][lang=css] feature=defect c=tbd u=tbd p=1)
Attachments
(1 file, 2 obsolete files)
8.15 KB,
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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
Whiteboard: [metro-mvp?]
Updated•12 years ago
|
Blocks: metrov1triage
Comment 2•12 years ago
|
||
Or we simply disable auto-scroll for Metro.
Updated•12 years ago
|
Summary: Cursor should change to reflect that "middle-click scrolling" is active → Cursor should change to reflect that "middle-click scrolling" (autoscroll) is active
Updated•12 years ago
|
Comment 3•12 years ago
|
||
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?
Flags: needinfo?(mbrubeck)
Comment 4•12 years ago
|
||
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.
Whiteboard: [metro-mvp?]
Comment 5•12 years ago
|
||
(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.
Flags: needinfo?(mbrubeck)
Whiteboard: [mentor=mbrubeck@mozilla.com][good first bug][lang=css]
Updated•11 years ago
|
Priority: -- → P2
Whiteboard: [mentor=mbrubeck@mozilla.com][good first bug][lang=css] → [mentor=mbrubeck@mozilla.com][good first bug][lang=css] feature=defect c=tbd u=tbd p=0
Comment 6•11 years ago
|
||
p=1
Assignee | ||
Updated•11 years ago
|
QA Contact: msamuel
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → msamuel
QA Contact: msamuel
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #760445 -
Flags: review?(mbrubeck)
Comment 8•11 years ago
|
||
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.
Attachment #760445 -
Flags: review?(mbrubeck) → review-
Assignee | ||
Comment 9•11 years ago
|
||
Implemented the 2nd approach described in the last comment.
Attachment #760445 -
Attachment is obsolete: true
Assignee | ||
Comment 10•11 years ago
|
||
Implemented the 2nd approach described in the last comment.
Attachment #761042 -
Attachment is obsolete: true
Attachment #761044 -
Flags: review?(mbrubeck)
Comment 11•11 years ago
|
||
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.
Attachment #761044 -
Flags: review?(mbrubeck) → review+
Assignee | ||
Comment 12•11 years ago
|
||
Updated•11 years ago
|
Updated•11 years ago
|
Summary: Cursor should change to reflect that "middle-click scrolling" (autoscroll) is active → Defect - Cursor should change to reflect that "middle-click scrolling" (autoscroll) is active
Whiteboard: [mentor=mbrubeck@mozilla.com][good first bug][lang=css] feature=defect c=tbd u=tbd p=0 → [mentor=mbrubeck@mozilla.com][good first bug][lang=css] feature=defect c=tbd u=tbd p=1
Updated•11 years ago
|
Status: NEW → ASSIGNED
Comment 13•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 14•11 years ago
|
||
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
Status: RESOLVED → VERIFIED
Comment 15•11 years ago
|
||
This doesn't map to a story we specified because we inherited the feature and have no story for it.
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•