Last Comment Bug 701351 - Vertical/Horizontal scrollbars missing with new compositor
: Vertical/Horizontal scrollbars missing with new compositor
Status: VERIFIED FIXED
: regression, uiwanted
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P2 normal (vote)
: ---
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
Depends on:
Blocks: native_droid_panning 704784
  Show dependency treegraph
 
Reported: 2011-11-10 06:21 PST by Aaron Train [:aaronmt]
Modified: 2012-01-09 11:01 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
(1/2) Back out the XUL scrollbar patch (6.83 KB, patch)
2011-11-15 13:34 PST, Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Splinter Review
(2/2) (WIP) Amazing rectangular scrollbars (8.47 KB, patch)
2011-11-15 13:35 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
Details | Diff | Splinter Review
Screenshot of current ugly scrollbars (237.14 KB, image/jpeg)
2011-11-15 13:43 PST, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details
(2/2) Amazing rectangular scrollbars (v2) (8.96 KB, patch)
2011-11-21 08:07 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
Details | Diff | Splinter Review

Description Aaron Train [:aaronmt] 2011-11-10 06:21:50 PST
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:kats@mozilla.com) 2011-11-15 13:34:21 PST
Created attachment 574673 [details] [diff] [review]
(1/2) Back out the XUL scrollbar patch
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-15 13:35:33 PST
Created attachment 574674 [details] [diff] [review]
(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:kats@mozilla.com) 2011-11-15 13:43:27 PST
Created attachment 574680 [details]
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?) 2011-11-15 14:09:42 PST
Better than nothing. I'd get them reviewed/landed and see how they perform.
Comment 5 Patrick Walton (:pcwalton) 2011-11-17 10:24:31 PST
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:kats@mozilla.com) 2011-11-21 08:07:00 PST
Created attachment 575864 [details] [diff] [review]
(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) 2011-11-21 13:44:14 PST
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:kats@mozilla.com) 2011-11-21 15:17:04 PST
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?) 2011-11-21 17:02:50 PST
(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) 2011-11-21 22:31:34 PST
we are sure that this didn't regress panning performance?  kats? patrick?
Comment 11 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-22 07:33:23 PST
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?) 2011-11-22 08:08:40 PST
We need the scrollbars to fade away after panning is finished. Finger down == scrollbars viisble, Finger up == scrollbars fade away.
Comment 13 Camelia Urian 2011-11-23 02:42:05 PST
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] 2011-11-23 06:50:47 PST
Samsung Nexus S (Android 4.0.1)
20111123040207
http://hg.mozilla.org/projects/birch/rev/cd5725c23a13

Note You need to log in before you can comment on or make changes to this bug.