Change - Scroll bars should act like in other metro apps when using mouse

VERIFIED FIXED in Firefox 25

Status

defect
P1
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: rsilveira, Assigned: jimm)

Tracking

Trunk
Firefox 25
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [shovel-ready] feature=change u=metro_firefox_user c=content_features p=2)

Attachments

(1 attachment, 5 obsolete attachments)

On other metro apps, the scroll bars are hidden until you move the mouse. Then they are visible for a second or so after the mouse is idle.

Bug 636564 can be helpful.
Blocks: 880298
Whiteboard: [shovel-ready]
For consideration as a Defect or Change Story.
Pushed a backout of the scrollbars patch to try, doesn't look like it's causing a lot of our failures. Unblocking.

https://tbpl.mozilla.org/?tree=Try&rev=dcc32bf72f3f&showall=1
No longer blocks: 880298
Hi Asa, would you like this Change Story added to V1?
Blocks: metrov1defect&change
No longer blocks: metrov1triage
Flags: needinfo?(asa)
Summary: Scroll bars should act like in other metro apps when using mouse → Change - Scroll bars should act like in other metro apps when using mouse
Whiteboard: [shovel-ready] → [shovel-ready] feature=change u=metro_firefox_user c=content_features p=0
Yes, please.
Flags: needinfo?(asa)
Priority: -- → P1
Looks like this will require some gfx work in widget theme code.
Whiteboard: [shovel-ready] feature=change u=metro_firefox_user c=content_features p=0 → [shovel-ready] feature=change u=metro_firefox_user c=content_features p=5
Posted patch wip (obsolete) — Splinter Review
Assignee: nobody → jmathies
Issues to sort out with this patch applied:

1) when panning with touch, we display both the front end scroll indicators (a small gray line) and the native scrollbars. IE will use scroll indicators when users interact with touch, and flip to native scroll bars when users interact with mouse. Technically I think this might be separate bug, I need to test without my wip applied.

2) scrollbar fade out values are too fast for Win8.

Other than these two issues, things look pretty good.
Blocks: 888329
Whiteboard: [shovel-ready] feature=change u=metro_firefox_user c=content_features p=5 → [shovel-ready] feature=change u=metro_firefox_user c=content_features p=1
Posted patch metro support v.1 (obsolete) — Splinter Review
Attachment #768970 - Attachment is obsolete: true
I think I'm going to ditch the ifdefing here and expose the "mousemove pops the scrollbars" feature to all platforms.
Posted patch metro support v.2 (obsolete) — Splinter Review
Attachment #769098 - Attachment is obsolete: true
Blocks: metrov1it10
Whiteboard: [shovel-ready] feature=change u=metro_firefox_user c=content_features p=1 → [shovel-ready] feature=change u=metro_firefox_user c=content_features p=2
Posted patch metro support v.3 (obsolete) — Splinter Review
I switched to using a pref for the mousemove displays scrollbars options. This seems to be working pretty well in metro land, and passed try as well.
Attachment #769112 - Attachment is obsolete: true
Attachment #769235 - Flags: review?(mstange)
No longer blocks: metrov1defect&change
Status: NEW → ASSIGNED
QA Contact: jbecerra
Comment on attachment 769235 [details] [diff] [review]
metro support v.3

I think nsLookAndFeel metrics would be better suited for this than a pref. Can you convert kScrollbarFadeBeginDelay + kScrollbarFadeDuration to metrics too, so that we don't need ifdefs for them?
(In reply to Markus Stange [:mstange] from comment #13)
> Comment on attachment 769235 [details] [diff] [review]
> metro support v.3
> 
> I think nsLookAndFeel metrics would be better suited for this than a pref.
> Can you convert kScrollbarFadeBeginDelay + kScrollbarFadeDuration to metrics
> too, so that we don't need ifdefs for them?

Sure, sounds good.
Posted patch metro support v.4 (obsolete) — Splinter Review
Updated per suggestions.
Attachment #769235 - Attachment is obsolete: true
Attachment #769235 - Flags: review?(mstange)
Attachment #769407 - Flags: review?(mstange)
Attachment #769407 - Attachment is obsolete: true
Attachment #769407 - Flags: review?(mstange)
Attachment #769423 - Flags: review?(mstange)
Comment on attachment 769423 [details] [diff] [review]
metro support v.4

The interaction between mListeningForMouseMove and mListeningForEvents is a little confusing to me. Maybe it would be clearer to split the Start/StopListeningForEvents methods into Start/StopListeningForScrollAreaEvents and Start/StopListeningForScrollbarEvents and then move the sDisplayOnMouseMove checks outside to the callers of the old Start/StopListeningForEvents methods.
Also, I'm not sure whether it's really worth caching the lookandfeel values.

But r=me either way.
Attachment #769423 - Flags: review?(mstange) → review+
No longer blocks: 888329
(In reply to Markus Stange [:mstange] from comment #17)
> Comment on attachment 769423 [details] [diff] [review]
> metro support v.4
> 
> The interaction between mListeningForMouseMove and mListeningForEvents is a
> little confusing to me. Maybe it would be clearer to split the
> Start/StopListeningForEvents methods into
> Start/StopListeningForScrollAreaEvents and
> Start/StopListeningForScrollbarEvents and then move the sDisplayOnMouseMove
> checks outside to the callers of the old Start/StopListeningForEvents
> methods.
> Also, I'm not sure whether it's really worth caching the lookandfeel values.
> 
> But r=me either way.

Went ahead and updated to this. The caching was left over from a previous patch, in the end it wasn't necessary so I removed it too.
error: field 'mDisplayOnMouseMove' will be initialized after field 'mHScrollbarHovered' [-Werror,-Wreorder]

bah.
Substantially better in the post-backout https://hg.mozilla.org/integration/mozilla-inbound/rev/77bd60758c53 version, that got down to just two instances of "ASSERTION: fade begin timer shouldn't be running: '!mFadeBeginTimer'" in https://tbpl.mozilla.org/php/getParsedLog.php?id=24815809&tree=Mozilla-Inbound&full=1#error0

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/77f2e8fc86ee
(In reply to Phil Ringnalda (:philor) from comment #23)
> Substantially better in the post-backout
> https://hg.mozilla.org/integration/mozilla-inbound/rev/77bd60758c53 version,
> that got down to just two instances of "ASSERTION: fade begin timer
> shouldn't be running: '!mFadeBeginTimer'" in
> https://tbpl.mozilla.org/php/getParsedLog.php?id=24815809&tree=Mozilla-
> Inbound&full=1#error0
> 
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/77f2e8fc86ee

https://hg.mozilla.org/integration/mozilla-inbound/rev/541db83c8f38
https://hg.mozilla.org/mozilla-central/rev/541db83c8f38
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Depends on: 889781
Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130715 Firefox/25.0

I verified that the scrollbars do hide until you move the mouse and they dismiss shortly after the mouse is idle.
Still, some issues were discovered:
1. After the scroll bars hide, there is some blank space remaining on the screen instead of them - logged bug #894350
2. The style of the scrollbars when moving the mouse is the same as when scrolling with the mousewheel. For other metro apps (e.g. Store), the look of the scrollbars is different when using the mouse scroll, the bar is thinner - logged bug 894361

I'm marking this bug as Verified since the initial issue is resolved and the follow up issues are tracked in separate bugs.
Status: RESOLVED → VERIFIED
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130816030205
Built from http://hg.mozilla.org/mozilla-central/rev/1ed5a88cd4d0

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0 and comment26 got expected result.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.