Closed Bug 934811 Opened 6 years ago Closed 6 years ago

Subframe scrollbars should not appear with subframe APZC enabled

Categories

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

x86
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: kats, Assigned: spohl)

References

()

Details

(Whiteboard: [block28])

Attachments

(1 file, 4 obsolete files)

From bug 913052:

(In reply to Jim Mathies [:jimm] from comment #4)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Whiteboard: [block28] → [block28] p=2
Attached patch Patch (obsolete) — Splinter Review
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!
Attachment #831823 - Flags: feedback?(jmathies)
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..
Flags: needinfo?(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
Flags: needinfo?(mbrubeck)
Comment on attachment 831823 [details] [diff] [review]
Patch

We're going to try taking a front end / css approach here.
Attachment #831823 - Flags: feedback?(jmathies) → feedback-
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.
Assignee: spohl.mozilla.bugs → nobody
Status: ASSIGNED → NEW
Whiteboard: [block28] p=2 → [block28]
No longer blocks: 915723
Blocks: metro-apzc
Whiteboard: [block28] → [release28]
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 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!
Attachment #831823 - Flags: feedback- → feedback?(bugmail.mozilla)
Blocks: 945260
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 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.
Attachment #831823 - Flags: feedback?(bugmail.mozilla) → feedback+
Attached patch Part 1: Patch (obsolete) — Splinter Review
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.
Assignee: nobody → spohl.mozilla.bugs
Attachment #831823 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8341323 - Flags: review?(tnikkel)
Attachment #8341323 - Flags: review?(jmathies)
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.
Attachment #8341325 - Flags: review?(jmathies)
Attachment #8341323 - Attachment description: Patch → Part 1: Patch
Attachment #8341325 - Attachment description: Remove overflow:hidden from CSS → Part 2: Remove overflow:hidden from CSS
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.
Attachment #8341323 - Flags: review?(tnikkel)
Attachment #8341323 - Flags: review?(jmathies)
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.
Attachment #8341325 - Flags: review?(jmathies)
Attachment #8341325 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
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
Attachment #8341323 - Attachment is obsolete: true
Attachment #8341794 - Flags: review?(tnikkel)
Attachment #8341794 - Flags: review?(jmathies)
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)?
Attachment #8341794 - Flags: review?(jmathies) → review+
Attachment #8341794 - Flags: review?(tnikkel) → review+
Attached patch PatchSplinter Review
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+.
Attachment #8341794 - Attachment is obsolete: true
Attachment #8341816 - Flags: review?(jmathies)
Attachment #8341816 - Flags: review+
Whiteboard: [release28] → [block28]
Comment on attachment 8341816 [details] [diff] [review]
Patch

ship it!
Attachment #8341816 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/mozilla-central/rev/80ae75938371
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.