890006 - Scrollable toolbar leaves behind a gray background on Android 2.3

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect, P1)

Description

[Jeff Muizelaar [:jrmuizel]]

Here's a video of it happening:
https://www.dropbox.com/s/h2wgqgd23uigpql/2013-07-03%2015.31.36.mp4

Comment 1

[Jeff Muizelaar [:jrmuizel]]

This looks really bad. It doesn't seem to be happening on Android 4.x but perhaps it happens on all Android 2.x

Comment 2

[Timothy Nikkel (:tnikkel)]

It happened on my galaxy s2 with android 2.3, but there is no bug after I upgraded to android 4.1.

Comment 3

[Kartikaya Gupta [:kats]]

This does look really bad on the Galaxy Q running 2.3 as well.

Comment 4

[Chris Lord [:cwiiis]]

I'm pretty certain this worked on Android < 4.0 at other points, so I'm suspecting this could be caused by UI layout and off-screen rendering during scrolling UI stuff...

It's either that (which is fixable), or it's some weird SurfaceView issue (which may or may not be work-around-able). Cc'ing mfinkle and sriram to see if they have any cycles to take a quick look at this.

Comment 5

[Chris Lord [:cwiiis]]

Sriram, do you have cycles to spend some time looking at this this evening? If not, I'll spend tomorrow on it. I strongly suspect this to be resolvable in front-end code.

Comment 6

[Chris Lord [:cwiiis]]

Jim Chen tested the Nightly build from March 8th where this feature originally landed, and this bug didn't appear (you can enable the feature with the browser.chrome.dynamictoolbar pref - a restart may be required in earlier versions)

It'd be good to get a regression window on this... As far as I remember, the initial version of this patch still worked in fundamentally the same way as it does now, so hopefully it's something quite small that's caused this.

Comment 7

[Sriram Ramasubramanian [:sriram]]

From what I see, the BrowserToolbar is behaving as expected. The toolbar scrolls up by the required amount the each time. There is no logic to snap it back (or change the margin and remove the "scrollTo()" effect) in BrowserToolbar. So, after dismissing the BrowserToolbar (by scrolling up), it stays with the "scrollY" value as it should be.

For anything below 4.0, there won't be corruption due to scrolling. This would happen in 4.0, if we are scrolling something and android has nothing to draw below it. (Note: If this corruption had to happen in <4.0 devices, we would see dark gray repeated on the right).

I don't understand how the LayerView's viewport works. There are few margin calculations that's probably increasing the size of the viewport.

I feel this has something to do with the LayerView and its animator logic. I see LayerView snapping back at the end, and the page showing up properly. Probably, during the process of scrolling, some update isn't happening.

Comment 8

[Chris Lord [:cwiiis]]

The question isn't about the behaviour of the scrolling (which is correct), but that blank area. Layout-wise, the LayerView occupies the whole screen and the toolbar is drawn on top of it - there is no animation of the LayerView at all. 

In terms of the appearance of movement of the LayerView,done via transforming layers native-side, and it works correctly - it does no clipping (which is why you would see the page showing through the translucent areas of the curve before a background was added to the toolbar - I forget the bug number for this).

Sriram, could you have a look at removing the background of the toolbar and seeing if that fixes it? If so, an easy fix sounds like manually drawing a background, and only drawing the visible rect of the background rather than painting the whole surface.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Is the blue we see related to the overscroll blue color of the LayerView?

Comment 10

[Chris Lord [:cwiiis]]

Just for clarity, after speaking on IRC, Sriram confirmed that the blue is indeed the toolbar or other related widget and not the drawing of the LayerView (it reflects the colour change when going into private mode, where as the LayerView rendering does not - which is a bug that I keep forgetting to file...)

Comment 11

[Sriram Ramasubramanian [:sriram]]

I tried creating a small app to simulate the scroll behavior. scrollX, scrollY on a view is apply only for its contents. This doesn't affect the background. In this case, we scroll the view by certain pixels. This causes the content to slide away. However, the background is still drawn (as Cwiiis mentioned in comment 8).

Removing the background isn't a simple task. The background has to be drawn for personas. I'll try the approach of drawing the background ourselves based on the scroll value.

Comment 12

[Kevin Brosnan [Ex-Mozilla]]

Sriarm understands what is going on here. Clearing the regressionwindow-wanted.

Comment 13

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

After quite a lot of hit and miss, I finally looked into the android code to see that it does something with clipping. Phew!!!

Comment 14

[Chris Lord [:cwiiis]]

Looks like you're on this Sriram, so assigning - thanks for sorting it out!

Comment 15

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9844a9e9c9c4

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9844a9e9c9c4

Comment 17

[Chris Lord [:cwiiis]]

Comment on attachment 774907 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Ugly grey area on Android 2.x when dynamic toolbar disappears
User impact if declined: Glitchy experience that makes you feel bad
Testing completed (on m-c, etc.): Tested by Sriram locally
Risk to taking this patch (and alternatives if risky): Low risk, not sure what of
String or IDL/UUID changes made by this patch: None

Comment 18

[Sriram Ramasubramanian [:sriram]]

(In reply to Chris Lord [:cwiiis] from comment #17)
> Comment on attachment 774907 [details] [diff] [review]
> Patch
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Ugly grey area on Android 2.x when
> dynamic toolbar disappears
> User impact if declined: Glitchy experience that makes you feel bad
> Testing completed (on m-c, etc.): Tested by Sriram locally
> Risk to taking this patch (and alternatives if risky): Low risk, not sure
> what of
> String or IDL/UUID changes made by this patch: None

This might actually require a different patch for aurora/beta as they don't carry the browser_toolbar.xml changes. If the release management is fine with applying these changes here: https://hg.mozilla.org/releases/mozilla-aurora/file/f2b49f275dff/mobile/android/base/resources/layout/browser_toolbar.xml I can do that. If not, I'll post new patches for aurora and beta tomorrow.

Comment 19

[Kartikaya Gupta [:kats]]

Verified on today's nightly on my Galaxy Q running 2.3

Comment 20

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Aurora and Beta

This patch is rebased for aurora and beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Animating tab counter.
User impact if declined: Blue bar will be shown in android 2.3 devices.
Testing completed (on m-c, etc.): Verified on 07/17
Risk to taking this patch (and alternatives if risky): None.
String or IDL/UUID changes made by this patch: None.

Comment 21

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 774907 [details] [diff] [review]
Patch

Posted a new patch.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d374bb5a41a7
https://hg.mozilla.org/releases/mozilla-beta/rev/4ff094eeafcf

Comment 23

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 25.0a1(2013-07-21)
Device: Samsung Galaxy R
Android: 2.3.4

Comment 24

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Build: Firefox for Android 23.0b8(2013-07-23)
Device: Samsung Galaxy R
Android: 2.3.4