Mouse scrollbars no longer displayed on content

RESOLVED FIXED in Firefox 30

Status

()

defect
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimm, Assigned: kats)

Tracking

({regression})

Trunk
mozilla31
x86_64
Windows 8.1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-b2g:1.4+, firefox30+ fixed, firefox31 fixed, b2g-v1.3 unaffected, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
When in precise input mode, we should see scrollbars. Currently these aren't displaying.
Blocks: metrobacklog
Whiteboard: [triage]
(Reporter)

Comment 3

5 years ago
Confirmed backing out bug 959847 fixes this. tn, any ideas? Our scrollbars have completely disappeared. Hopefully there's an easy fix.
Blocks: 959847
Component: Browser → Layout
Product: Firefox for Metro → Core
(Reporter)

Updated

5 years ago
Blocks: 913052
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.
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.
Posted patch add scrollbars hack (obsolete) — Splinter Review
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.
Flags: needinfo?(bugmail.mozilla)
Posted patch APZC side patch (obsolete) — Splinter Review
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.
Flags: needinfo?(bugmail.mozilla)
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".
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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.
Blocks: 992441
Posted patch part 1 - add scrollbars (obsolete) — Splinter Review
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.
Attachment #8388382 - Attachment is obsolete: true
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.
Assignee: nobody → bugmail.mozilla
Attachment #8400454 - Attachment is obsolete: true
Attachment #8404520 - Flags: review?(botond)
Attachment #8404451 - Flags: review?(roc)
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
Attachment #8404520 - Flags: review?(botond) → review+
(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
updated patch, carrying r+
Attachment #8404520 - Attachment is obsolete: true
Attachment #8405213 - Flags: review+
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.
Attachment #8405213 - Attachment is obsolete: true
Attachment #8405227 - Flags: review+
(Assignee)

Comment 17

5 years ago
greentry
New try push is at https://tbpl.mozilla.org/?tree=Try&rev=d762b0802460 (includes patches from bug 982888 as well)
Keywords: checkin-needed
Whiteboard: [triage]
Attachment #8404451 - Attachment description: add scrollbars → part 1 - add scrollbars
Botond non-trivially bit-rotted the first patch.
Keywords: checkin-needed
(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.
Unbitrotted by inlining the use of usingDisplayPort that was shuffled around before. Tested on-device and it works fine.
Attachment #8404451 - Attachment is obsolete: true
Attachment #8405680 - Flags: review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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).
Blocks: 995519
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..
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
I should have figured from the timestamps... Good thing we came up with the same method of unbitrotting then. :)
https://hg.mozilla.org/mozilla-central/rev/254c675a98c5
https://hg.mozilla.org/mozilla-central/rev/3d4f0e5e5ee0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
No longer blocks: 992441
Duplicate of this bug: 992441
Requesting 1.4+ to carry over the flag from bug 992441 which was duped to this one.
blocking-b2g: --- → 1.4?
Whiteboard: [bake on m-c until april 17]
blocking-b2g: 1.4? → 1.4+
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.