934811 - Subframe scrollbars should not appear with subframe APZC enabled

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Pan and Zoom, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

From bug 913052:

(In reply to Jim Mathies [:jimm] from comment #4)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> > I've noticed that in subframes (like scrollable divs) there are scrollbars.
> > I'm not sure why they show up there but not in the main content.
> 
> Actually that's a bug, we turn the standard mouse scrollbars off in content
> when the user is interacting with touch. Back when we handled scrolling in
> the front end, we drew our own touch scroll indicators (a slim line on the
> right of the view representing position). Those stopped working once we
> switched to apz. We can probably get them going again by listening to scroll
> messages.
> 
> Widget scrollbars in subframes should also be hidden by the front end css
> changes we make. If they are showing up, we should file a bug on getting rid
> of them.

This bug is for hiding the scrollbars in subframes.

Comment 1

[Stephen A Pohl [:spohl] (OOO until 3/25)]

Attachment: Patch

Asking for some preliminary feedback to see if this is going in the right direction at all. This hides scrollbars when the input level is "imprecise".

Feedback around method names and whether or not static variables should have an 's'-prefix would also be appreciated. Thanks!

Comment 2

[Jim Mathies [:jimm]]

I wonder if we can do this through css. Seeking matt's feedback, he understand how the platform css stuff works better than I do.

For the main frame, we kill scrollbars via front end css we apply based on a selector that gets set when the observer fires - 
http://mxr.mozilla.org/mozilla-central/source/browser/metro/theme/browser.css#330

I'm wondering if we can do the same for subframes, scrollable divs, text areas, etc..

Comment 3

[Matt Brubeck (:mbrubeck)]

We used to do something like this in cursor.css, which is applied only in imprecise/touch mode.  We removed that in bug 881067 because it was causing a bug:
http://hg.mozilla.org/mozilla-central/rev/76553702b21b

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 831823 [details] [diff] [review]
Patch

We're going to try taking a front end / css approach here.

Comment 5

[Stephen A Pohl [:spohl] (OOO until 3/25)]

Conversations on IRC say that we may need a change in a css file that deals with content (maybe content.css). Something like:

div[input="imprecise"] {
  overflow: hidden !important;
}

This alone isn't sufficient for the following reasons:
1. [input="imprecise"] doesn't currently work in content.css, i.e. overflow:hidden isn't applied to divs when touch input is used.
2. If overflow:hidden is applied unconditionally to the div on the page (i.e. without [input="imprecise"]), the scrollbars disappear correctly but the div is no longer scrollable.

At this point I'm not comfortable keeping this assigned to myself for my lack of knowledge of our front-end code.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

For the record, I don't think that using overflow:hidden to hide the scrollbars is a good idea because it's basically abusing the overflow property to get a side-effect. I believe bug 945260 is a result of the same problem, now that we respect overflow:hidden in APZC.

Comment 7

[Stephen A Pohl [:spohl] (OOO until 3/25)]

Comment on attachment 831823 [details] [diff] [review]
Patch

Looks like we might be coming back to this patch. Kats, would you be able to answer the questions from comment 1? I.e. do the method names seem reasonable, and do static variables take an 's' prefix? Anything else you'd like to see changed? Thanks!

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 831823 [details] [diff] [review]
Patch

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

::: widget/windows/WinUtils.cpp
@@ +33,5 @@
>  #include "imgIEncoder.h"
>  #include "nsIThread.h"
>  #include "MainThreadUtils.h"
>  #include "gfxColor.h"
> +#include "winrt/MetroInput.h"

You'll need to wrap winrt related code in WinUtils in #ifdef MOZ_METRO/#endif, otherwise I'm pretty sure you'll break non-metro enabled builds.

::: widget/windows/winrt/MetroInput.cpp
@@ +207,5 @@
>  namespace widget {
>  namespace winrt {
>  
> +MetroInput::InputPrecisionLevel MetroInput::sCurrentInputLevel =
> +  MetroInput::InputPrecisionLevel::LEVEL_IMPRECISE;

The naming here is fine.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 831823 [details] [diff] [review]
Patch

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

The gfx bits look fine to me. You'll probably to get tn to review the nsGfxScrollFrame.cpp change.

Comment 10

[Stephen A Pohl [:spohl] (OOO until 3/25)]

Attachment: Part 1: Patch

Updated for current trunk and addressed Jim's feedback.

:tn, if you could review the nsGfxScrollFrame changes, I'd appreciate it.

Will kick off try builds shortly.

Comment 11

[Stephen A Pohl [:spohl] (OOO until 3/25)]

Attachment: Part 2: Remove overflow:hidden from CSS

Not sure if we want to do this at the same time. I've confirmed that the patch works correctly with root scroll frames and hides the scrollbars when touch input is used.

Comment 12

[Stephen A Pohl [:spohl] (OOO until 3/25)]

Part 1 only:
https://tbpl.mozilla.org/?tree=Try&rev=82ee00808088

Part 1 and 2:
https://tbpl.mozilla.org/?tree=Try&rev=c69425b3e118

Comment 13

[Stephen A Pohl [:spohl] (OOO until 3/25)]

Comment on attachment 8341323 [details] [diff] [review]
Part 1: Patch

This is currently failing try. Might be tests that need to be updated. Will look into it tomorrow.

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 8341325 [details] [diff] [review]
Part 2: Remove overflow:hidden from CSS

This landed in bug 945260 yesterday to get scrolling working again.

Although we might be able to clean this up some more by removing 'imprecise' on the deck.

Comment 15

[Stephen A Pohl [:spohl] (OOO until 3/25)]

Attachment: Patch

Okay, this just needed an additional check in nsNativeThemeWin::ShouldHideScrollbars to make sure that we were running in metro mode. Try is now green:
https://tbpl.mozilla.org/?tree=Try&rev=5d3795c1292a

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 8341794 [details] [diff] [review]
Patch

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

Works great. Tested on various form inputs, didn't see any native scrollbars.

::: widget/windows/WinUtils.cpp
@@ +1184,5 @@
> +bool
> +WinUtils::IsInputModeImprecise()
> +{
> +#ifdef MOZ_METRO
> +  if (IsWin8OrLater()) {

I'm a little nervous about calls into winrt from desktop code here. Maybe we should move the runtime environment check you have in theme code into this call to be safe(r)?

Comment 17

[Stephen A Pohl [:spohl] (OOO until 3/25)]

Attachment: Patch

Thanks, Jim and Timothy! I've made the change that you suggested, Jim. I've changed the method name in WinUtils from IsInputModeImprecise to ShouldHideScrollbars to better describe what the method is responsible for. Setting back to r? to get another quick look before I land this. Carrying over Timothy's r+.

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 8341816 [details] [diff] [review]
Patch

ship it!

Comment 19

[Stephen A Pohl [:spohl] (OOO until 3/25)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80ae75938371

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80ae75938371