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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Randall Barker [:rbarker]]

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.

Comment 1

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

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

Comment 2

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

Attachment: Part 1 - Disable java scrollbars with C++ APZ

Comment 3

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

Attachment: Part 2 - Enable gecko scrollbars with C++ APZ

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.

Comment 4

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

Attachment: Part 3 - Prevent the scrollbars from "floating" off the bottom (partial fix)

Comment 5

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

Attachment: Part 1 - Disable java scrollbars with C++ APZ

Comment 6

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

Attachment: Part 2 - Enable gecko scrollbars with C++ APZ

Comment 7

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

Attachment: Part 3 - Prevent the scrollbars from "floating" off the bottom

Comment 8

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

Attachment: Part 2 - Enable gecko scrollbars with C++ APZ

Whoops, want to make sure that the xul scrollbars are *always* displayed, otherwise the reftests will complain.

Comment 9

[Botond Ballo [:botond]]

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.

Comment 10

[Botond Ballo [:botond]]

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.

Comment 11

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

Attachment: Part 2 - Enable gecko scrollbars with C++ APZ (v2)

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.

Comment 12

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

Attachment: Part 3 - Prevent the scrollbars from "floating" off the bottom

Updated with comments, carrying r+

Comment 13

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

Try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ba20a55d2f8

Comment 14

[Nick Alexander :nalexander [he/him]]

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.

Comment 15

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

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

Comment 16

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

Attachment: Part 2 - Enable gecko scrollbars with C++ APZ (v3)

Now with scrollbar-nonapz.css

Comment 17

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

Attachment: Part 4 - Update reftest.list files

Comment 18

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

Attachment: Part 4 - Update reftest.list files (v2)

Comment 19

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

Attachment: Part 4 - Update reftest.list files (v2)

Whoops, wrong patch

Comment 20

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

Attachment: Part 4 - Update reftest.list files (v3)

The final one

Comment 21

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

Attachment: Part 4 - Update reftest.list files (v4)

The final final one

Comment 22

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

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.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aae51a1e3419
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d654090848
https://hg.mozilla.org/integration/mozilla-inbound/rev/b60bba443efe
https://hg.mozilla.org/integration/mozilla-inbound/rev/474012a19e10

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aae51a1e3419
https://hg.mozilla.org/mozilla-central/rev/b5d654090848
https://hg.mozilla.org/mozilla-central/rev/b60bba443efe
https://hg.mozilla.org/mozilla-central/rev/474012a19e10

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/748531bf0272

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/748531bf0272