695569 - Scroll indicators for web content

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

I don't see any scroll indicators, while scrolling with Native Fennec.

Comment 1

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

Attachment: Add scroll indicators

Comment 2

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

Comment on attachment 570357 [details] [diff] [review]
Add scroll indicators

Actually, this patch doesn't work in all cases. I need to test/debug further. Feedback welcome though.

Comment 3

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

Attachment: Add scroll indicators (v2)

Comment 4

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

Comment on attachment 570743 [details] [diff] [review]
Add scroll indicators (v2)


>+  updateScrollbarsFor: function(aElement) {
>+    // only draw the scrollbars if we're scrolling the root content element
>+    if (aElement != this.selectedBrowser.contentDocument.documentElement &&
>+        aElement != this.selectedBrowser.contentDocument.body)
>+      return;

use a local:

let contentDoc = this.selectedBrowser.contentDocument

it's a tiny bit faster and you can leave the condition on a single line

>diff --git a/mobile/chrome/content/browser.xul b/mobile/chrome/content/browser.xul

>+<?xml-stylesheet href="chrome://browser/skin/browser.css" type="text/css"?>

I'd be happier if we gutted the CSS file to only have in it what we need.

Also, I saw a bug where panning to the top or the bottom would get stuck in a "panning" mode and the bars would not hide. Might not be your bug at all. Could be the reason we see text selection when panning too.


r- for the CSS file cleanup

Comment 5

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

Attachment: (1/3) Add scroll indicators (v3)

Update with JS refactoring as requested.

Comment 6

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

Attachment: (2/3) Remove unneeded CSS styles

Comment 7

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

Attachment: (3/3) Stop runaway scrolling behaviour

Comment 8

[Chris Lord [:cwiiis]]

Comment on attachment 570782 [details] [diff] [review]
(3/3) Stop runaway scrolling behaviour

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

Looks good to me.

Comment 9

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

https://hg.mozilla.org/projects/birch/rev/0aeaa4995892
https://hg.mozilla.org/projects/birch/rev/e968742b325d
https://hg.mozilla.org/projects/birch/rev/8f32c0c1285c

Comment 10

[Aaron Train [:aaronmt]]

Should the scroll-indicators have a slight opacity to them so you can still see web content? 

Verified Fixed

20111101040211
http://hg.mozilla.org/projects/birch/rev/7203d86d5868
-- Samsung Galaxy SII (Android 2.3.4)

Comment 11

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

Attachment: (4/3) Make scroll indicators transparent on gingerbread/honeycomb

Not sure if the colors will change later, but for now i'm just making the gingerbread/honeycomb versions the same as the default.

Comment 12

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

opacity and XUL normally leads to some small performance issues. We'll need to watch out for that.

Comment 13

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

https://hg.mozilla.org/projects/birch/rev/885ea82d6820

We can revisit this once pcwalton's stuff lands to see if it slows down performance.