980500 - Mouse scrollbars no longer displayed on content

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Comment 1

[Jim Mathies [:jimm]]

When in precise input mode, we should see scrollbars. Currently these aren't displaying.

Comment 2

[Jim Mathies [:jimm]]

2-28 - visible
3-01 - missing

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=58eca03214a6&tochange=8abc76dedec2

Comment 3

[Jim Mathies [:jimm]]

Confirmed backing out bug 959847 fixes this. tn, any ideas? Our scrollbars have completely disappeared. Hopefully there's an easy fix.

Comment 4

[Timothy Nikkel (:tnikkel)]

Guh, we now early exit in ScrollFrameHelper::BuildDisplayList before we get to the scroll bar adding code. I should have thought of this sooner. (This is the same reason b2g doesn't have scroll bars.) Not sure what to do yet, but we'll need to come up with something.

Comment 5

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

We could go to the approach we had discussed earlier where the scrollbar layers are a child of the scrollable container layer, and then we unapply the async transform on them.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: add scrollbars hack

One way to fix this to add the scrollbars as child layers of the scrollable layer. We then need APZC to treat them sort of like fixed position content and compensate for for being children of the scrolling layer so they stay in the same place on screen. This patch just adds a hack to add the scrollbar layers so that kats can experiment with the APZC side.

Comment 7

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

Attachment: APZC side patch

It's pretty straightforward to make the scrollbars render properly if the scrollbar layers are children of the scrollable container layer rather than siblings. The attached patch accomplishes this for the top-level scrollbars in the b2g browser that appear with tn's patch above.

Comment 8

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

How much work will it be to clean up the layout half? Is your intention here that the scrollbars would be children for subdocuments only, and remain siblings for other scrollframes? Or will it change to be children for all scroll layers? If it's the former I'll have to account for that in the APZ patch (not a lot of work, just need to check both parents and siblings). If it's the latter then we'll have to make sure the interaction with bug 967844 is also taken care of since that will change the layer structure for the "other scrollframes".

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> How much work will it be to clean up the layout half? Is your intention here
> that the scrollbars would be children for subdocuments only, and remain
> siblings for other scrollframes? Or will it change to be children for all
> scroll layers? If it's the former I'll have to account for that in the APZ
> patch (not a lot of work, just need to check both parents and siblings). If
> it's the latter then we'll have to make sure the interaction with bug 967844
> is also taken care of since that will change the layer structure for the
> "other scrollframes".

Shouldn't be much work at all. I was thinking of making them children only for root scroll frames, but I see how that is less consistent. We can do it the other way if it makes things considerably easier.

Comment 10

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

(In reply to Timothy Nikkel (:tn) from comment #9)
> Shouldn't be much work at all. I was thinking of making them children only
> for root scroll frames, but I see how that is less consistent. We can do it
> the other way if it makes things considerably easier.

It's not hard on the APZ side whichever way we go. But I think because of bug 967844, which might the container layer in some cases, it makes sense to leave it as-is (i.e. sibling) for non-root scrollframes, even though it's inconsistent.

Also, can you let me know what cleanup you had in mind here? If it's not hard I can probably do it.

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: part 1 - add scrollbars

I think this will do. This is the version suitable for uplifting. We can remove the if that disables scroll bars on the root root scroll frame on trunk whenever we decide to.

Comment 12

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

Attachment: part 2 - update scrollbar transform code

Updated the APZC side patch to deal with both sibling and parent situations. Mostly just refactoring the code to avoid duplication and adding a couple of extra things.

:tn, who should review the layout side patch? I tested the patches together and they work perfectly, as far as I can tell. Marketplace search results get a scrollbar, subframes (e.g. settings app) still have their scrollbar, but root content (e.g. pages in browser) still don't have a scrollbar.

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8404520 [details] [diff] [review]
part 2 - update scrollbar transform code

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +608,5 @@
> +    // on the actual content, we need to ensure that the APZC has updated any pending animations
> +    // to the current frame timestamp before we extract the transforms from it. The code in this
> +    // block accomplishes that and throws away the temp variables.
> +    // TODO: it might be cleaner to do a pass through the layer tree to advance all the APZC
> +    // transforms before updating the layer shadow transforms. That will allow removal of this code.

Can we condition this block on aScrollbarIsChild? We didn't seem to need it when the scrollbar could only be a previous sibling.

@@ +618,5 @@
> +  gfx3DMatrix asyncTransform = gfx3DMatrix(apzc->GetCurrentAsyncTransform());
> +  gfx3DMatrix nontransientTransform = apzc->GetNontransientAsyncTransform();
> +  gfx3DMatrix transientTransform = asyncTransform * nontransientTransform.Inverse();
> +
> +  Matrix4x4 scrollbarTransform;

I think a comment explaining why we are doing the calculations we are doing here would greatly enhance the understandability of this code. I propose:

"transientTransform represents the amount by which we have scrolled and zoomed since the last paint. 
  - The scroll thumb needs to be scaled in the direction of scrolling by the inverse 
    of the scale portion of transientTransform (representing the zoom), since zooming 
    in decreases the fraction of the scrollable rect that is in view. 
  - It needs to be translated in opposite direction of the translation portion of 
    transientTransform (representing the scroll), since scrolling down, which 
    translates the layer content up, should result in moving the content down.
    The amount of the translation to the scroll thumb should be such that the ratio
    of the translation to the size of the scroll port is the same as the ratio
    of the scroll amount to the size of the scrollable rect."

@@ +645,5 @@
> +  }
> +
> +  // GetTransform already takes the pre- and post-scale into account.  Since we
> +  // will apply the pre- and post-scale again when computing the effective
> +  // transform, we must apply the inverses here.

Can this be avoided if we use GetBaseTransform() rather than GetTransform() above?

@@ +683,2 @@
>  
> +  // If we didn't find a sibling, look for a parent

Do we know whether the scrollable layer will be a direct parent? If so, we shouldn't have a loop. If not, please s/parent/ancestor and s/child/descendant in comments and variable names.

Note: the transform-adjusting code ApplyAsyncTransformToScrollbarForContent seems to assume it's a direct parent.

::: gfx/layers/composite/AsyncCompositionManager.h
@@ +123,5 @@
>    // controller wants another animation frame.
>    bool ApplyAsyncContentTransformToTree(TimeStamp aCurrentFrame, Layer* aLayer,
>                                          bool* aWantNextFrame);
>    /**
>     * Update the shadow trasnform for aLayer assuming that is a scrollbar,

nit: typo

Comment 14

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

(In reply to Botond Ballo [:botond] from comment #13)
> Can we condition this block on aScrollbarIsChild? We didn't seem to need it
> when the scrollbar could only be a previous sibling.

Done. For the record I would have preferred to leave it un-conditioned because it is more robust to changes in tree-walking in ApplyAsyncContentTransformToTree but I don't feel strongly enough to argue.

> @@ +618,5 @@
> I think a comment explaining why we are doing the calculations we are doing
> here would greatly enhance the understandability of this code.

Added your suggested text with minor modifications.

> Can this be avoided if we use GetBaseTransform() rather than GetTransform()
> above?

Using GetBaseTransform() would mean the pre- and post-scale amounts get applied "around" the scrollbar transform and targetUntransform. i.e. with the patch as-is, after setting the shadow transform, when somebody calls GetTransform(), they will get:

pre * invpre * scrollbarTransform * GetTransform() * targetUntransform * invpost * post
= scrollbarTransform * pre * GetBaseTransform() * post * targetUntransform

whereas if we use GetBaseTransform() then we get:
pre * scrollbarTransform * GetBaseTransform() * targetUntransform * post

which is a different result. We could modify scrollbarTransform and targetUntransform to account for this but I think it is clearer (and consistent with the rest of the code) to do it this way.

> @@ +683,2 @@
> >  
> > +  // If we didn't find a sibling, look for a parent
> 
> Do we know whether the scrollable layer will be a direct parent? If so, we
> shouldn't have a loop. If not, please s/parent/ancestor and
> s/child/descendant in comments and variable names.
>
> Note: the transform-adjusting code ApplyAsyncTransformToScrollbarForContent
> seems to assume it's a direct parent.
> 

I don't know enough about layer building to answer that definitively, but I think it would only be a direct parent. And you're right that the transform adjustment is probably wrong if it's not the case. Removed the loop.

> nit: typo

Fixed

Comment 15

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

Attachment: part 2 - update scrollbar transform code (v2)

updated patch, carrying r+

Comment 16

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

Attachment: part 2 - update scrollbar transform code (v3)

try push made me realize i lost the call to LayerHasNonContainerDescendants when i refactored the code. put that back at the top of LayerIsContainerForScrollbarTarget so that it behaves the same as it did before.

Comment 17

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

New try push is at https://tbpl.mozilla.org/?tree=Try&rev=d762b0802460 (includes patches from bug 982888 as well)

Comment 18

[Timothy Nikkel (:tnikkel)]

Botond non-trivially bit-rotted the first patch.

Comment 19

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #18)
> Botond non-trivially bit-rotted the first patch.

The scroll frame BuildDisplayList functions (ScrollFrameHelper::BuildDisplayList, nsSubDocumentFrame::BuildDisplayList) seem to be pretty hot spots for APZ work. Kats' Part 2 patch from bug 982888 also touches the same code.

Comment 20

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

Attachment: part 1 - add scrollbars (unbitrotted)

Unbitrotted by inlining the use of usingDisplayPort that was shuffled around before. Tested on-device and it works fine.

Comment 21

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/254c675a98c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4f0e5e5ee0

Comment 22

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #20)
> Created attachment 8405680 [details] [diff] [review]
> part 1 - add scrollbars (unbitrotted)
> 
> Unbitrotted by inlining the use of usingDisplayPort that was shuffled around
> before. Tested on-device and it works fine.

This works but only because we only consider root scroll frames here (otherwise we might _create_ a display port lower down).

Comment 23

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

Thanks for fixing the errant ; that I forgot to qref out! (And for landing).

(In reply to Timothy Nikkel (:tn) from comment #22)
> This works but only because we only consider root scroll frames here
> (otherwise we might _create_ a display port lower down).

Agreed; hopefully this won't cause any problems down the road..

Comment 24

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> Thanks for fixing the errant ; that I forgot to qref out! (And for landing).

I actually didn't see your patch until after I landed.

Comment 25

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

I should have figured from the timestamps... Good thing we came up with the same method of unbitrotting then. :)

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/254c675a98c5
https://hg.mozilla.org/mozilla-central/rev/3d4f0e5e5ee0

Comment 28

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

Requesting 1.4+ to carry over the flag from bug 992441 which was duped to this one.

Comment 29

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

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/5fe425b4b351
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/717023539205