Scrollbar difficult to see on dark backgrounds

VERIFIED FIXED in Firefox 20

Status

()

Firefox for Android
Theme and Visual Design
--
enhancement
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: mcomella, Assigned: kats)

Tracking

Trunk
Firefox 20
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox18 wontfix, firefox19 affected, firefox20 verified)

Details

Attachments

(6 attachments, 3 obsolete attachments)

Created attachment 639914 [details]
The scrollbar is on that page... Really!

1) Open Fennec
2) Tap the URL bar and go to page "http://www.blackestate.co.nz/"
3) Scroll. (Note, the page should not fill your screen - if so, zoom)

Expected: Look at the scrollbar to see the location on page.
Actual: The scroll bar is too dark to see the current location on page.
Severity: normal → enhancement
Component: Theme → Theme and Visual Design
Product: Fennec → Firefox for Android
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected

Comment 1

6 years ago
Another example is the Ars Technica mobile site, when the "Light on Dark" site theme is selected (at the bottom of the page).
This would probably be a good first bug for somebody looking to contribute. The fix would be in mobile/android/base/gfx/ScrollbarLayer.java's constructor, where we create the bitmap that is used to draw the scrollbar. The fix would involve either updating the black circle that we use to have a white outline, or (more likely) replacing the programatically drawn bitmap with an actual image that is crafted to meet our needs. Some of the java GL code in the file might need to be updated to match.
Whiteboard: [good first bug][lang=java][mentor=kats]

Comment 3

6 years ago
Hi I'm new here, this seems to be a good one for me to start contribute. What kind of work should I do to deal with this bug, can u give me some clue? A lot thanks
While I am not the official mentor on this bug, I'll give you the very basics. I would first pull and build the code (https://wiki.mozilla.org/Mobile/Fennec/Android) if you have not done so already.

Then kats has posted a good starting description in the comment above yours. I would start there. 

If you need more assistance, you can comment in this bug and kats, the mentor, should be able top help you. Additionally, you can jump on IRC (https://wiki.mozilla.org/IRC) and you can get further help there. 

Good luck!

Comment 5

6 years ago
Thank u :) I would go check out those sites first
Duplicate of this bug: 717786
status-firefox17: --- → affected
status-firefox18: --- → affected

Comment 7

6 years ago
Hi guys,

I am new here and am it seems that I can try this problem out! Please let me know if anyone one is working on it? I am happy to collabrate !
Hi Udit, thanks for volunteering to take on this bug! Do you have an android build environment set up yet? If not, that's the first thing you should do - there are instructions at https://wiki.mozilla.org/Mobile/Fennec/Android on how to get set up, grab the code, and build it. If you have an android device that supports Firefox, see if you can get a build compiled locally and running on the device. If you don't have a device, you can use the android emulator that comes with the SDK instead - there are instructions at https://wiki.mozilla.org/Mobile/Fennec/Android/Emulator on how to run the APK in the emulator once you have built it.

Once you have that built and running (which might take a while, unfortunately - our build system is pretty complicated) I think the first thing you should try, just to make sure your build is set up correctly and you can make changes, is to go into the file mobile/android/base/gfx/ScrollbarLayer.java and change the BAR_SIZE value to something bigger (maybe 20) and see how that affects the look of the scrollbar.

As always if you have any questions or run into any problems, drop into #mobile on irc.mozilla.com (see https://wiki.mozilla.org/IRC for instructions on that) and fire away. My IRC handle is "kats", feel free to message me directly if you prefer.
Assignee: nobody → smartudit1

Comment 9

6 years ago
Hey Kartikaya,

Cool man! I don't have an android device :( but yeah I will download the emulator and try to build and compile the code. If in case any issue arises I will post here in comment !

Comment 10

6 years ago
Hey kartikaya,

I figured out that the fennec is not available for the windows environment. I will install the linux on my system and will go forward!

Updated

6 years ago
Duplicate of this bug: 795858

Comment 12

6 years ago
Hey Kartikaya, 

I have received my ubuntu. Will start with the installation as soon as possible. 

~Udit
Hi Udit, sorry for not replying earlier; I was away on vacation for the last week. Have you got a working build environment set up yet?
status-firefox19: --- → affected
Re-assigning to pushkar since I haven't heard back from Udit and I assume he's not working on this.
Assignee: smartudit1 → pushkar.singh93

Comment 15

5 years ago
Created attachment 678267 [details] [diff] [review]
Adds a glowing white boundary to the edges of the ScrollBar so that it is visible on dark backgrounds.

Scroll Bar visible on dark backgrounds.
Attachment #678267 - Flags: review?(bugmail.mozilla)

Comment 16

5 years ago
Created attachment 678268 [details]
Scroll Bar image after the above patch. Clearly Visible!
Comment on attachment 678267 [details] [diff] [review]
Adds a glowing white boundary to the edges of the ScrollBar so that it is visible on dark backgrounds.

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

The main problem with this patch is that it only works for the vertical scrollbar. You're taking a horizontal strip of pixels (so there's a white pixel on the left/right edge) and scaling that up to be the body of the scrollbar. That works for the vertical scrollbar but not the horizontal one. You'll probably need to have two different BODY_TEX_COORDS, one for the vertical and one for the horizontal.

The other problem is that the scrollbar isn't as "pretty" as it could be. I'd like you to try making the border a bit thicker and/or turning off antialiasing when drawing the circle. Those might help make it look a bit better. Another option could be to make the scrollbar a bit bigger overall by bumping up BAR_SIZE to see if that makes it look better.
Attachment #678267 - Flags: review?(bugmail.mozilla) → review-
After some discussion on IRC, pushkarsingh and I figured it would probably end up looking better if we had an actual image for the scrollbar rather than trying to draw one in code. CC'ing ian who said he could make an image for us.
Keywords: uiwanted
Kats, is there a specific size needed for the image? Or if you could post the original (non-outlined) scrollbar graphic here, I can modify that.
The current scrollbar is 6 pixels wide (for the vertical scrollbar) with a pixel of padding on either side, so something around that size would be good. We're not using an image for the current scrollbar; we just draw a gray circle programmatically and then basically use that for the rounded scrollbar endcaps and fill in the middle with the same colour.

Ideally what I would like is an image in three parts: two rounded endcaps and a middle piece. We can programatically stretch the middle piece as needed when the scrollbar is longer/shorter. I guess actually we just need one rounded endcap; we can flip it to get the other one.
Created attachment 687953 [details]
Scroll Bar images

I've created the scroll bar in three parts so that the middle can be stretched.
top_end_cap.png
middle.png
bottom_end_cap.png

It has a 1px white stroke and is filled with #3a3a3a

Let me know if I need to make any changes, thx!
Created attachment 688279 [details]
Single scrollbar texture image

Thanks Eric, that looks great!

pushkar, I glued the three images together using ImageMagick into the attached scrollbar.png; you should be able to load this into the Bitmap in ScrollbarLayer.java and then use appropriate rects from it as textures for the scrollbar. The top section of the PNG (8px wide by 4px tall) is the top/left endcap, the middle section (8px wide by 3px tall) is the middle section and the bottom section (again 8px wide by 4px tall) is the right/bottom endcap.
Could we see a screenshot of this new scrollbar in use?

Thanks!

Comment 24

5 years ago
(In reply to Kartikaya Gupta (:kats) from comment #22)
> Created attachment 688279 [details]
> Single scrollbar texture image
> 
> Thanks Eric, that looks great!
> 
> pushkar, I glued the three images together using ImageMagick into the
> attached scrollbar.png; you should be able to load this into the Bitmap in
> ScrollbarLayer.java and then use appropriate rects from it as textures for
> the scrollbar. The top section of the PNG (8px wide by 4px tall) is the
> top/left endcap, the middle section (8px wide by 3px tall) is the middle
> section and the bottom section (again 8px wide by 4px tall) is the
> right/bottom endcap.

Thanks! I am on it.

Comment 25

5 years ago
Created attachment 695646 [details] [diff] [review]
Not the final Patch

Needs to be checked, The problem with the horizontal bar persists.
Attachment #695646 - Flags: review?(bugmail.mozilla)
Comment on attachment 695646 [details] [diff] [review]
Not the final Patch

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

After discussion with Pushkar on IRC it looks like switching ScrollbarLayer to use an image is much more tricky than I had originally thought. I'll be taking over the bug and putting up a patch. Totally my fault for underestimating the work here :(
Attachment #695646 - Flags: review?(bugmail.mozilla) → review-
Created attachment 696133 [details] [diff] [review]
(1/2) Cleanup

No functional changes in this patch.
Attachment #696133 - Flags: review?(chrislord.net)
Created attachment 696134 [details] [diff] [review]
(2/2) Switch to using the two-tone image

It's a little clunky but if you have suggestions on how to improve it I'm all ears.
Attachment #678267 - Attachment is obsolete: true
Attachment #695646 - Attachment is obsolete: true
Attachment #696134 - Flags: review?(chrislord.net)
Created attachment 696135 [details]
What the scrollbars look like

Screenshot of a page with a dark background showing the new scrollbars.
Attachment #678268 - Attachment is obsolete: true
Keywords: uiwanted
Whiteboard: [good first bug][lang=java][mentor=kats] → [lang=java][mentor=kats]
Comment on attachment 696133 [details] [diff] [review]
(1/2) Cleanup

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

That's quite a reduction. Looks good to me, I assume it's tested.

::: mobile/android/base/gfx/ScrollbarLayer.java
@@ +75,5 @@
>          mVertical = vertical;
>          mRenderer = renderer;
>  
>          IntSize size = image.getSize();
> +        Bitmap bitmap = Bitmap.createBitmap(size.width, size.height, Bitmap.Config.ARGB_8888);

If we're creating a bitmap here and never reusing it, we may as well call recycle on it too.
Attachment #696133 - Flags: review?(chrislord.net) → review+
Comment on attachment 696134 [details] [diff] [review]
(2/2) Switch to using the two-tone image

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

I approve of replacing the generated bitmap with an image, seems pointless to have all that code if the parameters never change - it'd be nice to just draw flipped instead of copying the bitmap though, but I guess it's such a trivial amount of memory that it's not really a big deal. It'd be nice to reuse the texture to avoid an extra texture bind, but we're not careful about our GL commands in the slightest at the moment, so no need to add complication here if you don't want to. Looks good to me, assume it's tested and works.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +140,5 @@
> +        Bitmap scrollbarImage = view.getScrollbarImage();
> +        IntSize size = new IntSize(scrollbarImage.getWidth(), scrollbarImage.getHeight());
> +        scrollbarImage = expandCanvasToPowerOfTwo(scrollbarImage, size);
> +        mVertScrollLayer = new ScrollbarLayer(this, scrollbarImage, size, true);
> +        mHorizScrollLayer = new ScrollbarLayer(this, diagonalFlip(scrollbarImage), new IntSize(size.height, size.width), false);

Rather than creating a new flipped bitmap, why not draw it flipped when horizontal?

@@ +162,5 @@
> +            return image;
> +        }
> +        // make the bitmap size a power-of-two in both dimensions if it's not already.
> +        Bitmap potImage = Bitmap.createBitmap(potSize.width, potSize.height, image.getConfig());
> +        new Canvas(potImage).drawBitmap(image, new Matrix(), null);

bit lame that drawBitmap requires a Matrix parameter, but the Matrix class doesn't have a static identity matrix... Never mind.
Attachment #696134 - Flags: review?(chrislord.net) → review+
(In reply to Chris Lord [:cwiiis] from comment #31)
> I approve of replacing the generated bitmap with an image, seems pointless
> to have all that code if the parameters never change - it'd be nice to just
> draw flipped instead of copying the bitmap though, but I guess it's such a
> trivial amount of memory that it's not really a big deal. It'd be nice to
> reuse the texture to avoid an extra texture bind, but we're not careful
> about our GL commands in the slightest at the moment, so no need to add
> complication here if you don't want to. Looks good to me, assume it's tested
> and works.

Yeah I agree it would be nice to not create the extra bitmap/texture. I did originally try to do it that way but it ended up way more complicated and required a second implementation of fillRectCoordBuffer to map the corners differently and so on. This was much simpler, so I went with this approach.

> @@ +162,5 @@
> > +            return image;
> > +        }
> > +        // make the bitmap size a power-of-two in both dimensions if it's not already.
> > +        Bitmap potImage = Bitmap.createBitmap(potSize.width, potSize.height, image.getConfig());
> > +        new Canvas(potImage).drawBitmap(image, new Matrix(), null);
> 
> bit lame that drawBitmap requires a Matrix parameter, but the Matrix class
> doesn't have a static identity matrix... Never mind.

Yeah.. and passing in null for the matrix ends up scaling it up by 2x for some reason. That shouldn't happen but it does anyway.
https://hg.mozilla.org/integration/mozilla-inbound/rev/97542ba6b0aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/d462bf431ae4
Assignee: pushkar.singh93 → bugmail.mozilla
status-firefox13: affected → ---
status-firefox14: affected → ---
status-firefox15: affected → ---
status-firefox16: affected → ---
status-firefox17: affected → ---
status-firefox18: affected → wontfix
status-firefox20: --- → fixed
Whiteboard: [lang=java][mentor=kats]

Comment 34

5 years ago
https://hg.mozilla.org/mozilla-central/rev/97542ba6b0aa
https://hg.mozilla.org/mozilla-central/rev/d462bf431ae4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20

Comment 35

5 years ago
Firefox 20.0a1 (20123-01-02)
Device: Galaxy Nexus 
OS: Android 4.1.1 

Using Site from description I get the same image as in c#29. The scrollbar is now visible. 

Will this be pushed to Aurora too? Fx 19? If not we can set bug as Verified Fixed.
status-firefox20: fixed → verified
I think we'll just let this ride the trains. The change is fairly large and I don't think it's really worth uplifting.

Comment 37

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #36)
> I think we'll just let this ride the trains. The change is fairly large and
> I don't think it's really worth uplifting.

Thanks. I agree with that even more the merge will be done next week. 

Based on Comment #35 I'll set the bug to Verified Fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.