Last Comment Bug 695569 - Scroll indicators for web content
: Scroll indicators for web content
Status: VERIFIED FIXED
[QA-]
:
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)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-18 17:01 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2016-07-29 14:20 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Add scroll indicators (6.53 KB, patch)
2011-10-28 14:04 PDT, Kartikaya Gupta (email:kats@mozilla.com)
bugmail: review-
Details | Diff | Splinter Review
Add scroll indicators (v2) (6.85 KB, patch)
2011-10-31 09:59 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review-
Details | Diff | Splinter Review
(1/3) Add scroll indicators (v3) (6.83 KB, patch)
2011-10-31 11:59 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Splinter Review
(2/3) Remove unneeded CSS styles (144.35 KB, patch)
2011-10-31 12:00 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Splinter Review
(3/3) Stop runaway scrolling behaviour (1.49 KB, patch)
2011-10-31 12:01 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Splinter Review
(4/3) Make scroll indicators transparent on gingerbread/honeycomb (2.27 KB, patch)
2011-11-01 12:39 PDT, Kartikaya Gupta (email:kats@mozilla.com)
mark.finkle: review+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2011-10-18 17:01:15 PDT
I don't see any scroll indicators, while scrolling with Native Fennec.
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-28 14:04:25 PDT
Created attachment 570357 [details] [diff] [review]
Add scroll indicators
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-28 14:12:18 PDT
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:kats@mozilla.com) 2011-10-31 09:59:38 PDT
Created attachment 570743 [details] [diff] [review]
Add scroll indicators (v2)
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-31 10:59:36 PDT
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:kats@mozilla.com) 2011-10-31 11:59:55 PDT
Created attachment 570780 [details] [diff] [review]
(1/3) Add scroll indicators (v3)

Update with JS refactoring as requested.
Comment 6 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-31 12:00:20 PDT
Created attachment 570781 [details] [diff] [review]
(2/3) Remove unneeded CSS styles
Comment 7 Kartikaya Gupta (email:kats@mozilla.com) 2011-10-31 12:01:03 PDT
Created attachment 570782 [details] [diff] [review]
(3/3) Stop runaway scrolling behaviour
Comment 8 Chris Lord [:cwiiis] 2011-10-31 13:23:52 PDT
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 10 Aaron Train [:aaronmt] 2011-11-01 06:27:06 PDT
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:kats@mozilla.com) 2011-11-01 12:39:08 PDT
Created attachment 571108 [details] [diff] [review]
(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?) 2011-11-02 09:23:29 PDT
opacity and XUL normally leads to some small performance issues. We'll need to watch out for that.
Comment 13 Kartikaya Gupta (email:kats@mozilla.com) 2011-11-02 13:02:18 PDT
https://hg.mozilla.org/projects/birch/rev/885ea82d6820

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

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