1164879 - Use system UI for request desktop site checkbox

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: 2015-05-14 02.06.10.png

Request desktop site gets used quite a bit I'd say. 

Currently, we use a yellow checkbox that I don't think we use anywhere else.. I could be wrong!

But, for example in the Share location? doorhanger, we have a check too and these should be consistent. For starters, let's just use the system UI for switches in this place to unify them.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: Screen Shot 2015-09-08 at 4.39.54 PM.png

This is what I'm thinking. Perhaps we can loop in the those caret updates in here too. (the icons next to "Page" and "Tools".

But I'd like to get a build to see how those feel first. :)

Comment 2

[Prateek Arora]

Attachment: _20150921_221051.JPG

How does this look ? I think I got the tint wrong. I used #363B40. Let me know if I need to change that. If this works, I can fix carats next. The only thing in carats is that they should be a bit thick right ? I suppose we can use home_group_collapsed.png for that

Patch in next comment. 

PS: The PNGs, are they correctly added via export command ? I mean I get how patch works for text files, not sure I get it for images. (Maybe this is the reason for one of the patches in bug1155860 not working)

Comment 3

[Prateek Arora]

Attachment: bug1164879.patch

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

Drive by comment. Looks like in Anthony's mock up he used a grey and in your screenshot the color is black (#000). Michael or Anthony should be able to provide the grey we use.

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Prateek Arora from comment #2)
> Created attachment 8663729 [details]
> _20150921_221051.JPG
> 
> How does this look ? I think I got the tint wrong. I used #363B40. Let me
> know if I need to change that. 

I used the "Toolbar icon grey" here (#5F6368)

> If this works, I can fix carats next. The
> only thing in carats is that they should be a bit thick right ? I suppose we
> can use home_group_collapsed.png for that

Actually, let's keep the scope in this bug to be just about the check box. We can file another bug for the carets later. I may have gotten ahead of myself there. So, don't worry about those in this bug for now :)

> Patch in next comment. 
> 
> PS: The PNGs, are they correctly added via export command ? I mean I get how
> patch works for text files, not sure I get it for images. (Maybe this is the
> reason for one of the patches in bug1155860 not working)

Mcomella may be able to help here? Mike, I just grabbed it from the Material designs icon. and sized it to 18dp. Can we do that here? 

(In reply to Kevin Brosnan [:kbrosnan] from comment #4)
> Drive by comment. Looks like in Anthony's mock up he used a grey and in your
> screenshot the color is black (#000). Michael or Anthony should be able to
> provide the grey we use.

Yeah, it does look a bit dark. Try our "Toolbar icon grey" (#5F6368) that's the one I used in the spec and the color we normally use for any icons on top of the "Toolbar grey"

Comment 6

[Prateek Arora]

Attachment: _20150921_235746.JPG

How does this look ? 

Anthony, thanks for the color. I did the same thing, took them out of lollipop platform folder.

Earlier I was going for a tint option, but somehow it was not being reflected.
This time, I went with simple png only. I converted png's overlay color using Android Asset manager.

One thing, you said, you resized it to 18dp. I had already build when I read the comment. I had it at (hdpi: 48X48, that makes it 32 dp I think). 

But on app, the size seems ok I guess. 

Let me know if this is major issue. I will make it 18dp  and then submit new patch for review

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Prateek Arora from comment #6)
> Created attachment 8663778 [details]
> _20150921_235746.JPG
> 
> How does this look ? 
> 
> Anthony, thanks for the color. I did the same thing, took them out of
> lollipop platform folder.
> 
> Earlier I was going for a tint option, but somehow it was not being
> reflected.
> This time, I went with simple png only. I converted png's overlay color
> using Android Asset manager.
> 
> One thing, you said, you resized it to 18dp. I had already build when I read
> the comment. I had it at (hdpi: 48X48, that makes it 32 dp I think). 
> 
> But on app, the size seems ok I guess. 
> 
> Let me know if this is major issue. I will make it 18dp  and then submit new
> patch for review

Looking good! 

Though, I'm not sure what you have is 32dp. Unless my files are wrong here. Can you make it 18dp like the spec and attach a screenshot so I can see? 

Thanks!

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Prateek Arora from comment #2)
> PS: The PNGs, are they correctly added via export command ? I mean I get how
> patch works for text files, not sure I get it for images. (Maybe this is the
> reason for one of the patches in bug1155860 not working)

The pngs get added to the patches as base64 blobs – if you look at the raw text of a patch file, you'll see what I mean.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Worth noting that you should compress any pngs you add to the repo. On OS X, I use ImageOptim and I use trimage on Linux.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8663730 [details] [diff] [review]
bug1164879.patch

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

Is there a way to do this without adding an image asset? Can we just lean on the system's default checkbox here?

Also, you shouldn't include xxxhdpi – we don't support it atm (beyond the launcher icon, afaik).

::: mobile/android/base/resources/drawable/menu_item_uncheck_state.xml
@@ +5,5 @@
> +
> +<bitmap
> +    xmlns:android="http://schemas.android.com/apk/res/android"
> +    android:src="@drawable/menu_item_uncheck"
> +    android:tint="#363B40"/>

I think tint only works on Views. As you can see at [1], there's no mention of an android:tint attribute.

[1]: http://developer.android.com/guide/topics/resources/drawable-resource.html#Bitmap

Comment 11

[Prateek Arora]

Attachment: _20150922_223344.JPG

There you go Anthony ! resized to 18dp

Mike, 

>Is there a way to do this without adding an image asset? Can we just lean on >the system's default checkbox here?

I can cook up a build with default checkbox icon. I tried that. But even in that case I dont think on lollipop the icon would look like this. What I mean is, this isnt the actuall checkbox, so the checkbox theme that has actual checkbox icon wont apply, I think.

Let me get a screenshot of that build for you.

>Also, you shouldn't include xxxhdpi – we don't support it atm (beyond the >launcher icon, afaik).

Done ! Removed from XXXHDPI


>I think tint only works on Views. As you can see at [1], there's no mention of >an android:tint attribute.

Actually, when I input completely random color, like blue or something, it worked. But didn't work on different shades of black. I took the 'tint on bitmap' idea from android's implementation only.(in their btn check xml). So, I think tint does work, but weirdly.

Two things coming up. 
1.Patch with 18dp png icon (removed from XXXHDPI) and compressed !
2. Screenie using default checkbox resource

Comment 12

[Prateek Arora]

Attachment: bug1164879.patch

Here is the latest patch. Originally, the menu_item_check icons were not present even in XXHDPI. This patch just changes the 4 pngs.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Prateek Arora from comment #12)
> Here is the latest patch. Originally, the menu_item_check icons were not
> present even in XXHDPI. This patch just changes the 4 pngs.

We used to not ship xxhdpi but now we do so if you're adding assets, you should add it to xxhdpi too.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Prateek Arora from comment #11)
> I can cook up a build with default checkbox icon. I tried that.

I'm guessing it was hard and generally not worth it?

> But even in that case I dont think on lollipop the icon would look like this.

I'd expect we're fine having API level specific checkboxes (though now that we inherit from Theme.AppCompat (bug 1201206), maybe they'll be the most up-to-date checkboxes).

> What I
> mean is, this isnt the actuall checkbox, so the checkbox theme that has
> actual checkbox icon wont apply, I think.

Oh weird. It's sad though because we lose the animation. :(

> Actually, when I input completely random color, like blue or something, it
> worked. But didn't work on different shades of black. I took the 'tint on
> bitmap' idea from android's implementation only.(in their btn check xml).
> So, I think tint does work, but weirdly.

Yeah, strange. I guess we could dig around the Android source, but it'd just be edification at this point since the drawables are not used elsewhere. :P

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8664333 [details]
_20150922_223344.JPG

fwiw, it's awkwardly tiny in my local build.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8664350 [details] [diff] [review]
bug1164879.patch

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

These assets are compressed, right Prateek?

Assuming:
* Using the system checkbox is not worth the effort
* These assets are compressed
* Antlam likes the size

Looks good to me! :)

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8541b32b480b

Comment 17

[Prateek Arora]

> These assets are compressed, right Prateek?
> 
Yes, using trimage. But I would need to add xxhdpi as well right ?

> Assuming:
> * Using the system checkbox is not worth the effort
> * These assets are compressed
> * Antlam likes the size
> 
> Looks good to me! :)

You know, ever since the first try, I had a feeling that checkbox appears somewhat small. I could zoom into screenshot all I want, but it does appear small at first sight. 

I made the size 18dp. Maybe Anthony can confirm if it looks ok. 

I will post a patch with xxhdpi as well then.

Comment 18

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Prateek Arora from comment #17)
> > These assets are compressed, right Prateek?
> > 
> Yes, using trimage. But I would need to add xxhdpi as well right ?
> 
> > Assuming:
> > * Using the system checkbox is not worth the effort
> > * These assets are compressed
> > * Antlam likes the size
> > 
> > Looks good to me! :)
> 
> You know, ever since the first try, I had a feeling that checkbox appears
> somewhat small. I could zoom into screenshot all I want, but it does appear
> small at first sight. 
> 
> I made the size 18dp. Maybe Anthony can confirm if it looks ok. 
> 
> I will post a patch with xxhdpi as well then.

That does look a bit small. I think you guys are right. Can we try 20dp? 

Thanks Prateek!

Comment 19

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8664333 [details]
_20150922_223344.JPG

tis really small... But it looked bigger in my mock. Not sure what's going on but let's try 20 dp.

Comment 20

[Prateek Arora]

Attachment: size_fixed_with_Devframe.png

Check this out ! Its definitely bigger, but I hope doesn't look too big.

It's still 18dp.

Comment 21

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Hey Prateek!

I think the screenshot above is before our convos on IRC. Since you mentioned there was a lot of weird things going on, can you upload a new one after our "step back" approach you mentioned? That is, another screenshot of *actually* 18dp sized correctly?

Comment 22

[Prateek Arora]

Hi Guys,
So here is the step by step approach(es) I followed : (too complex to be written here, so I made a doc)
https://docs.google.com/document/d/1YcEzEKY9o_IJYVpq7-hRiUWtfUnZVSE9-NACCDiW6xM/edit?usp=sharing

Let me know your views on this.

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Thanks for the rundown, Prateek. Provided Anthony is okay with the UX, I have two comments:
  1) Just a reminder that the images you trimmed should be compressed
  2) You can use inset drawables [1] to avoid adding padding directly to the pngs (in this case, the caret). Can you come up with a solution using them?

[1]: http://developer.android.com/guide/topics/resources/drawable-resource.html#Inset

Comment 24

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Hey Prateek,

So, I'd like to keep this scope to be just about the check box. We can file another bug for the carets themselves :)

As for this icon. I think it best if we stick to what the icon pack supplies since I could see us using more of these icons in the near future. It will be easier to account for them in my designs since all of them will be squares, and I imagine this could simplify things for development too.

I talked to mcomella on IRC and even though we have an automated process for removing padding, it could be unpredictable if we have an icon that's off centered by design. 

tl;dr

Let's keep working with the asset that has padding built in. 

I'm not sure what's happening in the "scenario 1" but I think we should continue down that path. In "scenario 1", we should figure out what's going on there since that doesn't look like it's 18dp even though it's set at 18 dp.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sorry for making you put in all that effort to remove the padding, Prateek! We really appreciate all the iterations you made! I didn't consider the possibility of Anthony taking into account the standard padding on these icons and, if Anthony designs with them in mind, it should require less engineering effort. This is also the approach that Anthony wanted to take.

That being said, I'm still hesitant about this choice, given the inconsistency and the few cons:
  1) It's harder to re-use images with padding
  2) It takes up slightly more asset space

But we'll see how it works out.

Anthony, should the actual content area of the checkbox be 18dp then, or should it be 18dp including the padding? It's easier for engineering if you give the size including padding.

Comment 26

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

it should be 18 dp, without having to do anything to the asset provided by https://www.google.com/design/icons/ 

But it does look too small when I compare the screenshot in comment 22 that Prateek provided with my designs. I talked to Prateek and I recall there being some weird issues going on with the sizing. But I'm not sure if the issues were ever resolved.

Comment 27

[Prateek Arora]

(In reply to Anthony Lam (:antlam) from comment #26)
> it should be 18 dp, without having to do anything to the asset provided by
> https://www.google.com/design/icons/ 
> 
> But it does look too small when I compare the screenshot in comment 22 that
> Prateek provided with my designs. I talked to Prateek and I recall there
> being some weird issues going on with the sizing. But I'm not sure if the
> issues were ever resolved.

So, the comment22 was made after starting from scratch to avoid any unaccounted errors. 

Scenario 1 in comment22 is exactly what Anthony wants, except for the fact that it is appearing small. 

So, we need to investigate how we can make the icon look "good" (like last scenario, or the first mockup by anthony), without resorting to editing the icon's intrinsic padding. Right Anthony ?

I will work with Mike, to see how we can make Scenario1 work !

Comment 28

[Prateek Arora]

Holy Crap ! I just went to the link you gave !!! I have been using the ones from 
android/platform/res .... directory of my SDK. 

Just downloaded the ones present at the link, padding is definitely less than the ones present in sdk dir. This will surely solve the problem. So, we will be using this directly, just resized to 18dp.
Scenario1 -- solved

So sorry guys, to have wasted your time, such a silly mistake !!

Comment 29

[Prateek Arora]

Attachment: bug1164879_final.patch

This should do it.

Screenie : https://drive.google.com/file/d/0B8Ygm1nYMNJ3VDU5YzdvYzdqWFk/view?usp=sharing

Please ignore the font in sreenie, that is because of Custom ROM

Comment 30

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Looks good! Thanks Prateek.

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

No worries – there was just a miscommunication here. It happens!

Comment 32

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9046c57e755

Comment 33

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8667430 [details] [diff] [review]
bug1164879_final.patch

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

r+ assuming these assets are compressed (i.e. you ran ImageOptim or trimage over these images).

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7ded98d9f5b

By the way, if you want to apply for try access [1] (i.e. level 1 commit access), I'd be happy to vouch for you. See [2] as an example bug to file.

[1]: https://www.mozilla.org/en-US/about/governance/policies/commit/
[2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1175275

Comment 34

[Prateek Arora]

Attachment: bug1164879_compressed.patch

> r+ assuming these assets are compressed (i.e. you ran ImageOptim or trimage
> over these images).

Oops ! In the excitement of getting PNGs finally right ! forgot to compress them. Its compressed now. Saw some errors in try server result. Related to AppCompat, ActionBar etc.


> By the way, if you want to apply for try access [1] (i.e. level 1 commit
> access), I'd be happy to vouch for you. See [2] as an example bug to file.
> 
> [1]: https://www.mozilla.org/en-US/about/governance/policies/commit/
> [2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1175275

I would love to :) Thanks !

Also, if you can suggest some other bugs as well. Found some like bug1056031 and bug582581. bug582581 seems quite interesting, and you recently became mentor for that. I think I'll take that, if you can provide some pointers

Comment 35

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8669133 [details] [diff] [review]
bug1164879_compressed.patch

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

(In reply to Prateek Arora from comment #34)
> > r+ assuming these assets are compressed (i.e. you ran ImageOptim or trimage
> > over these images).
> 
> Oops ! In the excitement of getting PNGs finally right ! forgot to compress
> them. Its compressed now.

If I give an r+, you can actually just fix the nits I added and carry the r+ forward (i.e. upload a new patch with the fixes and put an r+ on it yourself, adding a comment explaining why). That actually applies in this case as well! :)

If you wanted to get rereviewed because you made some new changes, there was a difficult rebase, etc., you're also welcome to do that.

> Saw some errors in try server result. Related to
> AppCompat, ActionBar etc.

But I should at least resend the try.

> Also, if you can suggest some other bugs as well. Found some like bug1056031
> and bug582581. bug582581 seems quite interesting, and you recently became
> mentor for that. I think I'll take that, if you can provide some pointers

These all seem reasonable – do you still want me to suggest bugs? [1] is a list of polish bugs I like to pull from though we have some larger mentor projects available as well (e.g. new tablet, login page fixes).

I'm not sure you have context on bug 582581 but if you're still interested, NI me in that bug and I'll dig in to help you figure out what you need to know.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=fennec-polish

Comment 36

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a56740dfe0e3

Comment 37

[Prateek Arora]

I think it compiled correctly, right ? 

Mike, Shall I add checkin-needed now ?

Comment 38

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Prateek Arora from comment #37)
> I think it compiled correctly, right ? 

Compiled and passed the tests!

> Mike, Shall I add checkin-needed now ?

Yep! Thanks Prateek!

If you're looking for more to do, may I suggest bug 1213486? It's high priority and will need to be done by November 2nd(ish), before merge. If that isn't interesting to you, perhaps bug 1208577? Anthony would also like this by merge, but I'm not as concerned if it doesn't make it by merge.

Comment 39

[Prateek Arora]

Adding checkin-needed 
Try server results : https://treeherder.mozilla.org/#/jobs?repo=try&revision=a56740dfe0e3

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3ba8f63403ab

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3ba8f63403ab

Comment 42

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-10-19-15-30-22.png

Tested using:
Device: LG G4 (Android 5.1)
Build: Firefox for Android 44.0a1 (2015-10-19)