701351 - Vertical/Horizontal scrollbars missing with new compositor

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Aaron Train [:aaronmt]]

Regression due to the landing of: http://hg.mozilla.org/projects/birch/rev/eaf778e88070

--
20111110040927
http://hg.mozilla.org/projects/birch/rev/1cf9e4b19819
Samsung Galaxy SII (Android 2.3.3)

Comment 1

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

Attachment: (1/2) Back out the XUL scrollbar patch

Comment 2

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

Attachment: (2/2) (WIP) Amazing rectangular scrollbars

Not marking this for review yet as they are really ugly and should probably use images instead of an ugly translucent black box.

Comment 3

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

Attachment: Screenshot of current ugly scrollbars

This is what they look like with the first iteration of my patch.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Better than nothing. I'd get them reviewed/landed and see how they perform.

Comment 5

[Patrick Walton (:pcwalton)]

Comment on attachment 574674 [details] [diff] [review]
(2/2) (WIP) Amazing rectangular scrollbars

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

Depending on how UX wants this to look, these might end up being NinePatchTileLayers instead. But this is fine for now.

::: embedding/android/gfx/LayerRenderer.java
@@ +139,5 @@
>  
> +        gl.glEnable(GL10.GL_BLEND);
> +
> +        /* Draw the vertical scrollbar */
> +        if (pageRect.height() > screenSize.height) {

Factor this stuff out into a separate private method, perhaps? I don't want onDrawFrame() to get too big.

Comment 6

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

Attachment: (2/2) Amazing rectangular scrollbars (v2)

Update scrollbar patch - it's now rebased to birch tip, moved the heavy code into helper methods in ScrollbarLayer.java, and changed the BAR_SIZE to 8 so that it's a power of 2 and transparency works on pre-honeycomb as well.

Adding pcwalton for review again since it feels like there were significant changes.

Comment 7

[Patrick Walton (:pcwalton)]

Comment on attachment 575864 [details] [diff] [review]
(2/2) Amazing rectangular scrollbars (v2)

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

r=me with nits addressed.

::: mobile/android/base/gfx/ScrollbarLayer.java
@@ +71,5 @@
> +
> +    void drawVertical(GL10 gl, IntSize screenSize, Rect pageRect) {
> +        float barStart = (float)screenSize.height * (float)(0 - pageRect.top) / pageRect.height();
> +        float barEnd = (float)screenSize.height * (float)(screenSize.height - pageRect.top) / pageRect.height();
> +        float scale = Math.max(1.0f, (float)(barEnd - barStart) / BAR_SIZE);

barEnd and barStart are both floats already, no need to cast.

@@ +81,5 @@
> +
> +    void drawHorizontal(GL10 gl, IntSize screenSize, Rect pageRect) {
> +        float barStart = (float)screenSize.width * (float)(0 - pageRect.left) / pageRect.width();
> +        float barEnd = (float)screenSize.width * (float)(screenSize.width - pageRect.left) / pageRect.width();
> +        float scale = Math.max(1.0f, (float)(barEnd - barStart) / BAR_SIZE);

Ditto.

Comment 8

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

Landed blocky scrollbars. Leaving open for UX feedback and nicer-looking scrollbars.

https://hg.mozilla.org/projects/birch/rev/d099c3df6866
https://hg.mozilla.org/projects/birch/rev/77e488af39ff

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Kartikaya Gupta (:kats) from comment #8)
> Landed blocky scrollbars. Leaving open for UX feedback and nicer-looking
> scrollbars.
> 
> https://hg.mozilla.org/projects/birch/rev/d099c3df6866
> https://hg.mozilla.org/projects/birch/rev/77e488af39ff

I think it's better to open a new bug, when such feedback is forth coming. CC'ing Madhava and Ian. If they have thoughts about changes, they can file a bug with designs.

Comment 10

[Doug Turner (:dougt)]

we are sure that this didn't regress panning performance?  kats? patrick?

Comment 11

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

The FPS is still stable at 60, and the panning performance seems to be about the same. The repaints happen at less than 60 FPS anyway so this change shouldn't affect any existing bottlenecks.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

We need the scrollbars to fade away after panning is finished. Finger down == scrollbars viisble, Finger up == scrollbars fade away.

Comment 13

[Camelia Urian]

Build ID: Mozilla/5.0 (Android; Linux armv7l; rv:11.0a1) Gecko/20111122 Firefox/11.0a1 Fennec/11.0a1 - Native build
Device: HTC Desire Z
OS: Android 2.3

Horizontal and vertical scrollbars are displayed, but they are not hidden when panning finished. Should we open this bug or file a new bug for the "hide scrollbars" issue?

Comment 14

[Aaron Train [:aaronmt]]

Samsung Nexus S (Android 4.0.1)
20111123040207
http://hg.mozilla.org/projects/birch/rev/cd5725c23a13