Closed Bug 1223928 Opened 4 years ago Closed 4 years ago

Disable java scroll bars when C++APZ in enabled in Fennec

Categories

(Firefox for Android :: Toolbar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: rbarker, Assigned: kats)

References

Details

Attachments

(4 files, 11 obsolete files)

8.12 KB, patch
snorp
: review+
Details | Diff | Splinter Review
6.89 KB, patch
kats
: review+
Details | Diff | Splinter Review
8.20 KB, patch
snorp
: review+
Details | Diff | Splinter Review
20.69 KB, patch
snorp
: review+
Details | Diff | Splinter Review
When enabling C++APZ in Fennec, the Java rendered scroll bars should be disabled and properly styled gecko scroll bars should be used in their place.
The styling for the gecko scrollbars is in mobile/android/themes/core/content.css. We can probably copy the b2g styling for now, from b2g/chrome/content/content.css. Also we should make the scrollbars overlay scrollbars by setting the following prefs (also stolen from b2g.js):

pref("ui.showHideScrollbars", 1);
pref("ui.useOverlayScrollbars", 1);
pref("ui.scrollbarFadeBeginDelay", 450);
pref("ui.scrollbarFadeDuration", 0);

That should make the gecko scrollbars show up and work normally. The java scrollbars can be disabled if MOZ_ANDROID_APZ is true, in LayerRenderer.java. The gecko scrollbars should be display:none if MOZ_ANDROID_APZ is false; this can probably be achieved by making sure the content.css is run through a preprocessor. (We can't use the gecko scrollbars with JPZC because they won't get positioned correctly during scrolling).
Component: Panning and Zooming → Graphics, Panning and Zooming
Product: Core → Firefox for Android
Assignee: nobody → bugmail.mozilla
I just copied the styling wholesale from b2g. I had to change the fadeduration pref from 0 to nonzero though, since it was segfaulting on a division-by-zero in ScrollbarActivity.cpp. Not sure why that doesn't happen on b2g, I'll look into it.
Attachment #8687414 - Attachment is obsolete: true
Attachment #8688600 - Flags: review?(snorp)
Whoops, want to make sure that the xul scrollbars are *always* displayed, otherwise the reftests will complain.
Attachment #8688600 - Attachment is obsolete: true
Attachment #8688600 - Flags: review?(snorp)
Attachment #8688608 - Flags: review?(snorp)
Attachment #8688599 - Flags: review?(snorp) → review+
Comment on attachment 8688605 [details] [diff] [review]
Part 3 - Prevent the scrollbars from "floating" off the bottom

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

javascript:void(0);

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +746,5 @@
> +              aNode->GetScrollbarTargetContainerId() == aRootScrollId);
> +    });
> +  if (scrollbar) {
> +    TranslateShadowLayer(scrollbar, gfxPoint(0, -aFixedLayerMargins.bottom), true);
> +    if (scrollbar->GetParent()) {

Please add a comment explaining that GetParent() here is a container layer for the scrollbar track (?) which holds the clip rect for the scroll bar (which needs to be expanded); otherwise people might think it's the root scrollable layer (that's what I thought when I first read the code).

@@ +1333,5 @@
>      bool foundRoot = false;
>      if (ApplyAsyncContentTransformToTree(root, &foundRoot)) {
>  #if defined(MOZ_ANDROID_APZ)
>        MOZ_ASSERT(foundRoot);
> +      if (foundRoot && mFixedLayerMargins != ScreenMargin()) {

It would be nice to do this adjustment inside ApplyAsyncContentTransformToTree, but as we discussed that's not easy to do right now, mostly because of the hacky way we find the root (set aOutFoundRoot) in that function. However, as that hack is hopefully temporary (as per bug 1201529 comment 6), please add a comment reminding us to try to move this into AACTTT (or otherwise make it nicer) once the hack is removed.
Attachment #8688605 - Flags: review?(botond) → review+
Comment on attachment 8688605 [details] [diff] [review]
Part 3 - Prevent the scrollbars from "floating" off the bottom

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +741,5 @@
> +                            const ScreenMargin& aFixedLayerMargins)
> +{
> +  Layer* scrollbar = BreadthFirstSearch(aRoot,
> +    [aRootScrollId](Layer* aNode) {
> +      return (aNode->GetScrollbarDirection() == Layer::HORIZONTAL &&

Oh, please also mention that technically the vertical scrollbar should be scaled a bit as well to account for the extra room, but we're choosing not to do that because it's a lot of effort for a very small visual effect.
Rebased part 2 on top of bug 1208759 which removed the preprocessing for content.css. f?nalexander also to make sure I did the preprocessing the way he intended me to do it as per our IRC discussion.
Attachment #8688608 - Attachment is obsolete: true
Attachment #8688608 - Flags: review?(snorp)
Attachment #8688653 - Flags: review?(snorp)
Attachment #8688653 - Flags: feedback?(nalexander)
Updated with comments, carrying r+
Attachment #8688605 - Attachment is obsolete: true
Attachment #8688654 - Flags: review+
Comment on attachment 8688653 [details] [diff] [review]
Part 2 - Enable gecko scrollbars with C++ APZ (v2)

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

lgtm.

::: mobile/android/themes/core/content.css
@@ +68,5 @@
>    display: none;
>  }
>  
> +xul|thumb {
> +  background-color: rgba(0, 0, 0, 0.4) !important;

Did you mean to change the color?

::: mobile/android/themes/core/jar.mn
@@ +28,5 @@
>    skin/aboutReaderControls.css              (aboutReaderControls.css)
>    skin/aboutSupport.css                     (aboutSupport.css)
>    skin/browser.css                          (browser.css)
>    skin/content.css                          (content.css)
> +#ifdef MOZ_ANDROID_APZ

This works for me if it works for you.

I think the @include of scrollbar.css will be confusing if there's a scrollbar.css in the directory, so prefer scrollbar-{apz,nonapz}.css, so there's always a choice.
Attachment #8688653 - Flags: feedback?(nalexander) → feedback+
(In reply to Nick Alexander :nalexander from comment #14)
> Comment on attachment 8688653 [details] [diff] [review]
> Part 2 - Enable gecko scrollbars with C++ APZ (v2)
> > +xul|thumb {
> > +  background-color: rgba(0, 0, 0, 0.4) !important;
> 
> Did you mean to change the color?

Yeah, I copied it from b2g. AFAIK the old value was used in XUL fennec and hasn't been used since, so it's not really something that we need to necessarily keep.

> >    skin/content.css                          (content.css)
> > +#ifdef MOZ_ANDROID_APZ
> 
> This works for me if it works for you.
> 
> I think the @include of scrollbar.css will be confusing if there's a
> scrollbar.css in the directory, so prefer scrollbar-{apz,nonapz}.css, so
> there's always a choice.

Ah, good point. I'll rename that file and update the jar.mn accordingly.
Now with scrollbar-nonapz.css
Attachment #8688653 - Attachment is obsolete: true
Attachment #8688653 - Flags: review?(snorp)
Attachment #8688685 - Flags: review?(snorp)
Attachment #8688685 - Flags: review?(snorp) → review+
Whoops, wrong patch
Attachment #8689162 - Attachment is obsolete: true
The final one
Attachment #8689168 - Attachment is obsolete: true
The final final one
Attachment #8689234 - Attachment is obsolete: true
Comment on attachment 8689270 [details] [diff] [review]
Part 4 - Update reftest.list files (v4)

Randall's latest try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d144ba4c4f56 with this version looks good. Flagging for review.
Attachment #8689270 - Flags: review?(snorp)
Attachment #8689270 - Flags: review?(snorp) → review+
Depends on: 1227599
You need to log in before you can comment on or make changes to this bug.