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

Status: VERIFIED FIXED

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

Description

[Rodrigo Silveira [:rsilveira] [:rodms]]

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.

Comment 1

[Marco Mucci [:MarcoM]]

For consideration as a Defect or Change Story.

Comment 2

[Jim Mathies [:jimm]]

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

Comment 3

[Marco Mucci [:MarcoM]]

Hi Asa, would you like this Change Story added to V1?

Comment 4

[Asa Dotzler [:asa]]

Yes, please.

Comment 5

[Jim Mathies [:jimm]]

Looks like this will require some gfx work in widget theme code.

Comment 6

[Jim Mathies [:jimm]]

Attachment: wip

Comment 7

[Jim Mathies [:jimm]]

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.

Comment 8

[Jim Mathies [:jimm]]

Attachment: metro support v.1

Comment 9

[Jim Mathies [:jimm]]

I think I'm going to ditch the ifdefing here and expose the "mousemove pops the scrollbars" feature to all platforms.

Comment 10

[Jim Mathies [:jimm]]

Attachment: metro support v.2

Comment 11

[Jim Mathies [:jimm]]

try push in the works:

https://tbpl.mozilla.org/?tree=Try&rev=1b2f4b89f521

Comment 12

[Jim Mathies [:jimm]]

Attachment: metro support v.3

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.

Comment 13

[Markus Stange [:mstange]]

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?

Comment 14

[Jim Mathies [:jimm]]

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

Comment 15

[Jim Mathies [:jimm]]

Attachment: metro support v.4

Updated per suggestions.

Comment 16

[Jim Mathies [:jimm]]

Attachment: metro support v.4

Comment 17

[Markus Stange [: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.

Comment 18

[Jim Mathies [:jimm]]

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

Comment 19

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6132caf111

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for warnings-as-errors bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd826792000

https://tbpl.mozilla.org/php/getParsedLog.php?id=24793048&tree=Mozilla-Inbound

Comment 21

[Jim Mathies [:jimm]]

error: field 'mDisplayOnMouseMove' will be initialized after field 'mHScrollbarHovered' [-Werror,-Wreorder]

bah.

Comment 22

[Jim Mathies [:jimm]]

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=66e52e1a0245
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2299dce9dbd

Comment 23

[Phil Ringnalda (:philor)]

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

Comment 24

[Jim Mathies [:jimm]]

(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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/541db83c8f38

Comment 26

[Mihaela Velimiroviciu (:mihaelav)]

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.

Comment 27

[Samvedana (:Samvedana)]

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.