Closed Bug 890006 Opened 7 years ago Closed 7 years ago

Scrollable toolbar leaves behind a gray background on Android 2.3

Categories

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

23 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 25
Tracking Status
firefox23 + verified
firefox24 + verified
firefox25 + verified
fennec 23+ ---

People

(Reporter: jrmuizel, Assigned: sriram)

Details

(Keywords: reproducible)

Attachments

(2 files)

This looks really bad. It doesn't seem to be happening on Android 4.x but perhaps it happens on all Android 2.x
It happened on my galaxy s2 with android 2.3, but there is no bug after I upgraded to android 4.1.
tracking-fennec: --- → ?
This does look really bad on the Galaxy Q running 2.3 as well.
Keywords: reproducible
OS: Mac OS X → Android
Hardware: x86 → ARM
Version: unspecified → Firefox 23
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.
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.
Flags: needinfo?(sriram)
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.
tracking-fennec: ? → 23+
Flags: needinfo?(kbrosnan)
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.
Flags: needinfo?(sriram)
Priority: -- → P1
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.
Flags: needinfo?(sriram)
Is the blue we see related to the overscroll blue color of the LayerView?
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...)
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.
Flags: needinfo?(sriram)
Sriarm understands what is going on here. Clearing the regressionwindow-wanted.
Flags: needinfo?(kbrosnan)
Attached patch PatchSplinter Review
After quite a lot of hit and miss, I finally looked into the android code to see that it does something with clipping. Phew!!!
Attachment #774907 - Flags: review?(wjohnston)
Looks like you're on this Sriram, so assigning - thanks for sorting it out!
Assignee: nobody → sriram
Status: NEW → ASSIGNED
Attachment #774907 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/9844a9e9c9c4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
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
Attachment #774907 - Flags: approval-mozilla-beta?
Attachment #774907 - Flags: approval-mozilla-aurora?
(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.
Verified on today's nightly on my Galaxy Q running 2.3
Status: RESOLVED → VERIFIED
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.
Attachment #777223 - Flags: approval-mozilla-beta?
Attachment #777223 - Flags: approval-mozilla-aurora?
Comment on attachment 774907 [details] [diff] [review]
Patch

Posted a new patch.
Attachment #774907 - Flags: approval-mozilla-beta?
Attachment #774907 - Flags: approval-mozilla-aurora?
Attachment #777223 - Flags: approval-mozilla-beta?
Attachment #777223 - Flags: approval-mozilla-beta+
Attachment #777223 - Flags: approval-mozilla-aurora?
Attachment #777223 - Flags: approval-mozilla-aurora+
Verified fixed on:
Build: Firefox for Android 25.0a1(2013-07-21)
Device: Samsung Galaxy R
Android: 2.3.4
Verified fixed on:
Build: Firefox for Android 23.0b8(2013-07-23)
Device: Samsung Galaxy R
Android: 2.3.4
You need to log in before you can comment on or make changes to this bug.