814435 - Display scroll bars when async pan and zoom is turned on

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Carlos Martínez Toral [:carlosmartinez]]

Tested with unagi with:

gaia 620399e
gecko 85d0121

STR:
1-Open browser app
2-Surf to slashdot.org
3-Double tap to zoom
4-Scroll up & down and left & rigth

Expected result --> Scroll bars should appear in the right side and down side of the screen
Actual result --> No scroll bar is shown

Comment 1

[Ben Francis [:benfrancis]]

Were these intentionally removed when async pan and zoom was turned on, or is this a regression?

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

Hi Josh, can you confirm comment#1 from Ben? is it the new design or a regression? thanks

Comment 3

[David Scravaglieri [:scravag]]

Renominate if needed

Comment 4

[Josh Carpenter [:jcarpenter]]

(In reply to Joe Cheng from comment #2)
> Hi Josh, can you confirm comment#1 from Ben? is it the new design or a
> regression? thanks

Scroll bars should appear. Per Ben, this sounds like either a result of async pan-zoom being turned on, or a regression.

Comment 5

[Angela Hubenya]

defect is still reproducing using Unagi, build 20130104070203

Comment 6

[amiller]

UCID: browser-000
https://moztrap.mozilla.org/results/case/61262/

Comment 7

[Matt Brandt [:mbrandt]]

Reproducible on the 2013-01-12 eng unagi build:
- build identifier 20130112063240
- git commit df38c1bb813029f3ccfa4a997fb1529b..

Comment 8

[Angela Hubenya]

Repros on Unagi device Build ID: 20130115070201   using the december 5th kernel

Comment 9

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I believe we should be tracking this bug for the future.  renoming for tracking b2g-18

Comment 10

[mdaniloff]

this still reproduces on:
Unagi Build ID: 20130225070200
Dec 5th Kernel
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3a5a27992a75
Gaia: 5691a16fff8e1403c75ed9d6f3a443b7e58198c6

Comment 11

[mdaniloff]

This still repros on Inari Build ID: 20130417070205
Kernel Date: Feb 21
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/6bac24e14538
Gaia: 2d048a9bdae54e4ec7d48326c2130591c8b869b6

When the user loads the URL of a long horizontal webpage and presses return to load the page,no vertical and/or horizontal scroll bars appear

UCID:Browser-000
Testcase: https://moztrap.mozilla.org/runtests/run/1105/env/316/?&pagenumber=1&pagesize=20&sortfield=order&sortdirection=asc&filter-id=1739

Comment 12

[Ben Francis [:benfrancis]]

If I use the facebook app I get scroll bars, if I load the same web site in the browser I don't. I'm guessing they're not enabled for iframes which are using APZC.

Comment 14

[John Hammink]

What is the decision on this feature?  Do we want this or not - I can't ascertain that from the discussion here nor in the product requirements.

According to Josh's comment 4, scroll bars should appear.   To nom or not to nom?

Comment 15

[John Hammink]

Let's look at more data

slahdot.org enables/disables scrollbars, and has only a desktop sites according to different criteria, but IMO, mobile sites should probably have some kind a scrollbar by default.

Trying out most of the top sites according to Alexa.com, and not a single one  brings up any sort of scrollbar.  I'm inclined to renom this bug, but still waiting for clarification.

Comment 16

[Peter Dolanjski [:pdol]]

Moving NI to Chris and Francis, who are the Browser PM and Browser UX going forward.

Comment 17

[Francis Djabri [:djabber]]

Please renominate this bug as this is something we definitely want for the browser from both a product and UX perspective.

Comment 19

[John Hammink]

According to Josh in Comment #4, scroll bars should appear.   So nomming for Koi.

Comment 20

[Ben Francis [:benfrancis]]

John, do you really believe that this is a blocker, as in if this was the last bug we had we would delay the 1.2 release for it?

We're trying to make "blocking" actually mean blocking in 1.2 and use the browser backlog for general prioritisation of work https://www.pivotaltracker.com/s/projects/867301

Comment 21

[John Hammink]

Happy to denom this, if it's the case that this is no longer a requirement (at one point, I believe it was, although searching through our requirements haystack for the original referring one is a bloody mess).

Thanks for the link on Pivotal tracker btw. 

Should we de-nom then and deactivate the regression testcase that keeps hitting this (for half a year now)?

I'll go ahead and do exactly this, let's pick this up later if we decide it matters.

Comment 22

[Angela Hubenya]

Test case 1739 has been disabled in moztrap per comment 21

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Now that APZ is supposed to be used for all apps I want to leverage this issue. It seems bad to loose all scrollbars once async scroll will be the default behavior for apps.

Comment 24

[Andreas Gal :gal]

This wants an owner or it will turn into a long-pole.

Comment 25

[Andreas Gal :gal]

Attachment: Part 1: Draw scrollbars when compositing scrollable container layers.

This draws simple scroll bars for scrollable container layers. This is totally untested so far. Still missing: hiding the axis that can't scroll (if any), fade-out animation, rounded corners.

Comment 26

[Andreas Gal :gal]

Attachment: Part 2: Auto-hide the axis that isn't scrollable.

Comment 27

[Andreas Gal :gal]

If anyone gets assigned to this, please grab my patches and continue and don't wait for me. I am on and off and traveling next week.

Comment 28

[Andreas Gal :gal]

Attachment: Draw scrollbars with proper offset and length. Auto-hide when we don't scroll along that axis. The transform is wrong so this jumps around unexpectedly.

Comment 29

[Andreas Gal :gal]

GetEffectiveTransform() seems to include the scroll transform. I need the transform of the layer rect without that. No idea how to do that.

Comment 30

[Andreas Gal :gal]

Milan, I need some help here. I can't figure out what rect and transform to use. I don't think mCompositionBounds is necessarily correct, and EffectiveTransform includes the scroll offset. Kats probably can give some guidance here, or anyone else who knows the code.

Comment 31

[Milan Sreckovic [:milan] (needinfo for best results)]

Kats should be back today.  Vivien, I made this a 1.3 blocker, based on your comment 23, but if you think it should not be, please revert it back.

Comment 32

[Andreas Gal :gal]

So mCompositionRect is probably the right rect to use here. Its the size of the scrollable region in screen pixels, independent of zoom. Its (0,0) based, even when the url bar comes in, so the transform is what moves it into the right place.

Comment 33

[Andreas Gal :gal]

I added ScreenPoint mScrollOffset; to LayerComposite to store the scroll offset we calculate in SampleContentTransformForFrame. That scroll offset is (0,0) based but definitely resolution dependent.

Comment 34

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Milan Sreckovic [:milan] from comment #31)
> Kats should be back today.  Vivien, I made this a 1.3 blocker, based on your
> comment 23, but if you think it should not be, please revert it back.

I think it should be, thanks Milan.

Comment 35

[Andreas Gal :gal]

Attachment: Alternative approach (scrollbars drawn entirely in compositor)

Comment 36

[Andreas Gal :gal]

Its not a temporary quick jump. Once wrong the bars hang out in the wrong position.

Comment 37

[Andreas Gal :gal]

Sorry kats, all you from here on. I will poke a bit but unlikely I will figure this out.

Comment 38

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

I can reproduce the scrollbars ending up in the center. STR:

1. Apply patch from this bug
2. Start B2G browser and go to people.mozilla.com/~kgupta/grid.html (or any other page really)
3. Zoom out and wait for the repaint

This is happening because the composition bounds are incorrect when zoomed out; this is a known issue tracked in bug 935219. I will take a crack at that today to see if I can fix it up.

Comment 39

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

Attachment: Turn off non-APZC scrollbars (WIP)

Botond is continuing to work on bug 935219. In the meantime I took a quick look at the pre-APZC scrollbars in gaia apps. Vivien pointed to me to where they are turned on and styled so I was able to turn them off. This results in one set of scrollbars rather than two, but also breaks scrolling unless we back out bug 825692. The attached patch does that. To summarize the various behaviours (with APZC enabled):

* Current gaia master: scrollbars are drawn by layout and so don't update async. Input fields are not scrollable.

* Gaia master + andreas' patch: two sets of scrollbars are drawn, one from layout and one in the compositor. Input fields are not scrollable.

* Gaia master + andreas' patch + attached patch: scrollbars are drawn in the compositor. Input fields are layerized and scrollable. Input fields also have scrollbars.

Comment 40

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #39)
> Input fields also
> have scrollbars.

This artifact is making me think that perhaps drawing the scrollbars entirely in the compositor may not be the best idea. That is, if we have cases (such as input fields) which should be scrollable but not display scrollbars, and we are drawing the scrollbars entirely in the compositor, then we need to annotate the layers as to whether or not they should display scrollbars. Plus, all the styling that we can currently do with scrollbars no longer works.

Another approach might be to have the scrollbars generated in layout as they are now, but force them onto their own layers and then apply an additional transform to them in the compositor based on the async transform in the APZC. Layerizing the scrollbars (assuming they're not already) will make them more expensive but we might want them layerized anyway to have them fade in and out smoothly.

Comment 41

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

Filed bug 946408 for the input fields not being scrollable.

Comment 42

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #40)
> 
> Another approach might be to have the scrollbars generated in layout as they
> are now, but force them onto their own layers and then apply an additional
> transform to them in the compositor based on the async transform in the
> APZC. Layerizing the scrollbars (assuming they're not already) will make
> them more expensive but we might want them layerized anyway to have them
> fade in and out smoothly.

I think scrollbars has been moved into their own layer in https://bug813041.bugzilla.mozilla.org/attachment.cgi?id=828520

Comment 43

[Andreas Gal :gal]

We will never get proper responsiveness for scrollbars if we don't draw them entirely where the decisions on scrolling are made (which is the compositor).

Comment 44

[Andreas Gal :gal]

Annotating scrollable layers that aren't supposed to have scroll bars is not difficult I think. We need a quick plan here. I don't really care about the precise approach but we have 4 days left and we are making zero progress here.

Comment 45

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

Attachment: Part 1 - Annotate layers for scrollbars

Comment 46

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

Attachment: Part 2 - Always layerize overlay scrollbars

Comment 47

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

Attachment: Part 3 - Set a shadow transform on the scrollbar layer

Comment 48

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

With these three patches the scrollbars in Gaia work as before, and track async panning as well. The only problem that I can see is that the fade animation is still based on when gecko repaints rather than the async panning. That is, the scrollbars will start to fade out while panning, and will un-fade when gecko repaints, which makes it seem random.

Comment 49

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

Comment on attachment 8343166 [details] [diff] [review]
Part 3 - Set a shadow transform on the scrollbar layer

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +569,5 @@
> +                      1);
> +      transform.ScalePost(1.0f/aLayer->GetPostXScale(),
> +                          1.0f/aLayer->GetPostYScale(),
> +                          1);
> +      aLayer->AsLayerComposite()->SetShadowTransform(transform);

Oh, I need a break statement here, so that it only uses the first scrollTarget. Will fix locally.

Comment 50

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

Attachment: Part 3 - Set a shadow transform on the scrollbar layer

(Added the break statement)

Comment 51

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

Attachment: Part 4 - Prevent the scrollbar fading out while user is scrolling

Cheating, but it works.

Comment 52

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8343190 [details] [diff] [review]
Part 4 - Prevent the scrollbar fading out while user is scrolling

I can't say I like that but I can live with it for this deadline. Could we reduce a bit the 500ms delay just it does not seems like it takes forever ?

Comment 53

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

Sure, I can reduce it. 500ms is what we use on Fennec and it seems to work well so I thought it would be fine here. 300ms should be fine too.

Comment 54

[Andreas Gal :gal]

If we have scrollbars in both directions are we creating one big layer with mostly transparent pixels?

Comment 55

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8343165 [details] [diff] [review]
Part 2 - Always layerize overlay scrollbars

Do we want to limit the scope of this change? Only certain platforms or where it's needed maybe?

Comment 56

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

(In reply to Andreas Gal :gal from comment #54)
> If we have scrollbars in both directions are we creating one big layer with
> mostly transparent pixels?

No, we should end up with separate layers for the vertical and horizontal scrollbars.

(In reply to Timothy Nikkel (:tn) from comment #55)
> Do we want to limit the scope of this change? Only certain platforms or
> where it's needed maybe?

Yeah. I can ifdef it for B2G but I was hoping you'd have suggestions on a more appropriate condition to use. Should I check for usingDisplayPort? That should identify only the scroll frames that have an APZC although it seems like checking for a side-effect that may happen in other scenarios too.

Comment 57

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

No longer need the dependency if we go with the new patch set.

Comment 58

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

Attachment: Part 4 - Prevent the scrollbar fading out while user is scrolling

I found the right way to do this. Turned out to be pretty straightforward so I hooked it up.

Comment 59

[Botond Ballo [:botond]]

Comment on attachment 8343772 [details] [diff] [review]
Part 4 - Prevent the scrollbar fading out while user is scrolling

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

Looks good. One comment: is there a reason the GeckoContentController interface takes a full ScrollableLayerGuid but we only pass the scroll id over PBrowser? If not, I'd prefer the two to be consistent.

Comment 60

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #56)
> (In reply to Timothy Nikkel (:tn) from comment #55)
> > Do we want to limit the scope of this change? Only certain platforms or
> > where it's needed maybe?
> 
> Yeah. I can ifdef it for B2G but I was hoping you'd have suggestions on a
> more appropriate condition to use. Should I check for usingDisplayPort? That
> should identify only the scroll frames that have an APZC although it seems
> like checking for a side-effect that may happen in other scenarios too.

I'm not sure off the top of my head. B2G only for now sounds okay until we figure out a better idea.

Comment 61

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

(In reply to Botond Ballo [:botond] from comment #59)
> Looks good. One comment: is there a reason the GeckoContentController
> interface takes a full ScrollableLayerGuid but we only pass the scroll id
> over PBrowser? If not, I'd prefer the two to be consistent.

At the level of the GeckoContentController interface, what the notification will be used for is unknown, and so it makes sense to have it take a ScrollableLayerGuid. That way the implementor knows exactly which APZC instance is being transformed, and they can do whatever they need to. I originally passed the ScrollableLayerGuid across the PBrowser interface too but then I realized that no other function does that, and in fact the layers id is meaningless in TabChild. Since it didn't make sense to pass the layers id I also took out the presshell id because it wasn't being used. I can put these back but it seems unnecessary to me, as this is really more of an implementation detail than a real cross-module interface.

Comment 62

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

Attachment: Part 2 - Always layerize overlay scrollbars on B2G

Comment 63

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #61)
> (In reply to Botond Ballo [:botond] from comment #59)
> > Looks good. One comment: is there a reason the GeckoContentController
> > interface takes a full ScrollableLayerGuid but we only pass the scroll id
> > over PBrowser? If not, I'd prefer the two to be consistent.
> 
> At the level of the GeckoContentController interface, what the notification
> will be used for is unknown, and so it makes sense to have it take a
> ScrollableLayerGuid. That way the implementor knows exactly which APZC
> instance is being transformed, and they can do whatever they need to. I
> originally passed the ScrollableLayerGuid across the PBrowser interface too
> but then I realized that no other function does that, and in fact the layers
> id is meaningless in TabChild. Since it didn't make sense to pass the layers
> id I also took out the presshell id because it wasn't being used. I can put
> these back but it seems unnecessary to me, as this is really more of an
> implementation detail than a real cross-module interface.

That sounds reasonable, let's keep it as it is then.

(As we discussed on IRC, technically passing in the layers id to GeckoContentController is redundant as there is already a separate GeckoContentController instance for each layers id, but we can pass a ScrollableLayerGuid for convenience.)

Comment 64

[Benoit Girard (:BenWa)]

Comment on attachment 8343188 [details] [diff] [review]
Part 3 - Set a shadow transform on the scrollbar layer

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +530,5 @@
> +    // If this layer corresponds to a scrollbar, then search backwards through the
> +    // siblings until we find a container layer with an APZC. That should
> +    // (hopefully) be the content that this scrollbar is for. Pick up the
> +    // transient async transform from that layer and use it to update the
> +    // scrollbar position.

I'm a bit unsure about this. We're assuming that if we have a scrollbar layer then we have a layer for the scrollable content. I don't know if this is true and if there aren't race conditions (i.e. the scroll layer goes inactive before the scrollbar because we're fading out the scrollbar).

Can you convince me?

Comment 65

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

(In reply to Benoit Girard (:BenWa) from comment #64)
> I'm a bit unsure about this. We're assuming that if we have a scrollbar
> layer then we have a layer for the scrollable content. I don't know if this
> is true and if there aren't race conditions (i.e. the scroll layer goes
> inactive before the scrollbar because we're fading out the scrollbar).
> 
> Can you convince me?

If the layer for the scrollable content disappears then that means that we can't be asynchronously scrolling it, so we don't need to apply any async transform to the scrollbars. We can just leave them wherever Gecko painted them because that will be correct.

The only problematic scenario I can think of is if that loop actually finds some *other* scrollable layer that happens to be a sibling of the scroll target layer. I don't know if that can happen in practice and if it does, how often. I imagine that it will not be a problem in practice. If it is a problem we can probably deal with it by adding the ViewID onto the scrollbar layers in layout, and then match those with the ViewID on the scroll target.

Thoughts?

Comment 66

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #65)

> The only problematic scenario I can think of is if that loop actually finds
> some *other* scrollable layer that happens to be a sibling of the scroll
> target layer.

It's the type of bugs I'm worried about. Where we match another layer.

> I don't know if that can happen in practice and if it does,
> how often. I imagine that it will not be a problem in practice. If it is a
> problem we can probably deal with it by adding the ViewID onto the scrollbar
> layers in layout, and then match those with the ViewID on the scroll target.
> 
> Thoughts?

For me this sounds like your admitting that it can happen and its things like that which cause flaky code which APZC already has a ton of. IMO we should deal with this now.

Comment 67

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

Attachment: Part 1 - Annotate layers for scrollbars (v2)

Fair enough. I've updated part 1 to also propagate the ViewID. I changed how the data was stored on the layer since I couldn't stuff it into the flags any more. Mostly just copied what the sticky position data was doing. Re-requesting review from tn for some minor changes to the layout bits as well.

Comment 68

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

Attachment: Part 3 - Set a shadow transform on the scrollbar layer (v2)

Updated to ensure it finds the right target layer.

Comment 69

[Benoit Girard (:BenWa)]

Comment on attachment 8343978 [details] [diff] [review]
Part 1 - Annotate layers for scrollbars (v2)

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

::: gfx/layers/Layers.h
@@ +1398,5 @@
> +  struct ScrollbarData {
> +    FrameMetrics::ViewID mScrollId;
> +    ScrollDirection mDirection;
> +  };
> +  nsAutoPtr<ScrollbarData> mScrollbarData;

This structure is really small. It's probably smaller then a nsAutoPtr or nearly. For small structs we should avoid heap allocations IMO.

If you disagree at least add MOZ_COUNT_CTOR.

Comment 70

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

Attachment: Part 1 - Annotate layers for scrollbars (v3)

Updated to not use the nsAutoPtr struct and just have a mIsScrollbar bool instead. Carrying BenWa's r+ and tn's r?.

Comment 71

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8344321 [details] [diff] [review]
Part 1 - Annotate layers for scrollbars (v3)

nsDisplayOwnLayer turning into a catch all for miscellaneous things starts to feel like the wrong way, but we can clean it up the next time we need to add to it, no need to hold this back from landing.

Comment 72

[Benoit Girard (:BenWa)]

Comment on attachment 8343979 [details] [diff] [review]
Part 3 - Set a shadow transform on the scrollbar layer (v2)

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

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +524,5 @@
>      appliedTransform = true;
>    }
>  
> +  if (aLayer->GetIsScrollbar()) {
> +    // If this layer corresponds to a scrollbar, then search backwards through the

Optional:
Consider making the contents of this if() into it's own function. It has a well defined purpose and this function is already to long.

Comment 73

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

(In reply to Benoit Girard (:BenWa) from comment #72)
> Optional:
> Consider making the contents of this if() into it's own function. It has a
> well defined purpose and this function is already to long.

I did this. Also I forgot to initialize mIsScrollbar in the Layer constructor in part 1 so I did that.

Try push at https://tbpl.mozilla.org/?tree=Try&rev=f29bc60b87b2, will land if it looks good.

Comment 74

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

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/9eb611dbbe97
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/4c81aaa4763e
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/75178cf263ae
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/c25e3a7c7e2f

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9eb611dbbe97
https://hg.mozilla.org/mozilla-central/rev/4c81aaa4763e
https://hg.mozilla.org/mozilla-central/rev/75178cf263ae
https://hg.mozilla.org/mozilla-central/rev/c25e3a7c7e2f

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cd7afa66a5b4
https://hg.mozilla.org/releases/mozilla-aurora/rev/434453c80f43
https://hg.mozilla.org/releases/mozilla-aurora/rev/d00e4939aecb
https://hg.mozilla.org/releases/mozilla-aurora/rev/fa19b7565b48