1210862 - Consolidate corner radius in dimens

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

We have a few different corner sizes [1]. button_corner_radius seems to have been our attempt to consolidate but it's 3dp, not 4dp like I thought was our standard.

Anthony, is there a standard corner radius? Are there any exceptions to that?

If so, let's fix it!

[1]: https://mxr.mozilla.org/mozilla-central/search?string=radius&find=dimens.xml&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 1

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

It's a bit hard for me to understand what visual element these lines of code refer to specifically but from my latest designs, 4dp should be the standard. 

And that's where I'll be starting my designs from unless there's a compelling reason to deviate.

From that list, could you tell me what the follow are?

 - "button_corner_radius" 3dp
 - "tablet_browser_toolbar_menu_item_corner_radius" 5dp
 - "toast_button_corner_radius" 2dp
 - "fxaccount_corner_radius" 3dp

Comment 2

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

(In reply to Anthony Lam (:antlam) from comment #1)
> From that list, could you tell me what the follow are?

It would take a long time to create a comprehensive list but...

>  - "button_corner_radius" 3dp

https://mxr.mozilla.org/mozilla-central/search?string=[^_]button_corner_radius&regexp=on&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Looks like the buttons in the share overlay, the setup button in the remote tabs page, and the zoomed view.

>  - "tablet_browser_toolbar_menu_item_corner_radius" 5dp

https://mxr.mozilla.org/mozilla-central/search?string=tablet_browser_toolbar_menu_item_corner_radius&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

The pressed area on the tablet toolbar/tab strip buttons.

>  - "toast_button_corner_radius" 2dp

https://mxr.mozilla.org/mozilla-central/search?string=toast_button_corner_radius&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Toasts.

>  - "fxaccount_corner_radius" 3dp

https://mxr.mozilla.org/mozilla-central/search?string=fxaccount_corner_radius&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Everything round in fxaccount.

Comment 3

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

(In reply to Michael Comella (:mcomella) from comment #2)
> (In reply to Anthony Lam (:antlam) from comment #1)
> > From that list, could you tell me what the follow are?
> 
> It would take a long time to create a comprehensive list but...

Thanks! this really helps. 

> >  - "button_corner_radius" 3dp
> 
> https://mxr.mozilla.org/mozilla-central/
> search?string=[^_]button_corner_radius&regexp=on&find=mobile%2Fandroid&findi=
> &filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
> 
> Looks like the buttons in the share overlay, the setup button in the remote
> tabs page, and the zoomed view.

We could probably unify and update these to be 4dp everywhere

> >  - "tablet_browser_toolbar_menu_item_corner_radius" 5dp
> 
> https://mxr.mozilla.org/mozilla-central/
> search?string=tablet_browser_toolbar_menu_item_corner_radius&find=mobile%2Fan
> droid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
> 
> The pressed area on the tablet toolbar/tab strip buttons.

As above, probably best we update to the 4dp. 

> >  - "toast_button_corner_radius" 2dp
> 
> https://mxr.mozilla.org/mozilla-central/
> search?string=toast_button_corner_radius&find=mobile%2Fandroid&findi=&filter=
> ^[^\0]*%24&hitlimit=&tree=mozilla-central
> 
> Toasts.

Could you elaborate a bit more here? Our toasts have fully rounded sides so I'm not sure where "2dp" is coming into play.

> >  - "fxaccount_corner_radius" 3dp
> 
> https://mxr.mozilla.org/mozilla-central/
> search?string=fxaccount_corner_radius&find=mobile%2Fandroid&findi=&filter=^[^
> \0]*%24&hitlimit=&tree=mozilla-central
> 
> Everything round in fxaccount.

This one's a bit more finicky. There's more moving parts here so maybe we can leave this for now and avoid causing some chaos.

All in all, it might be best if we can get on Vidyo. These mostly sound straight forward from a UI stand point. :) Let's ping on IRC

Comment 4

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

I'll throw up a build for the first two.

(In reply to Anthony Lam (:antlam) from comment #3)
> Could you elaborate a bit more here? Our toasts have fully rounded sides so
> I'm not sure where "2dp" is coming into play.

It's apparently 2dp on GB & 24dp on KitKat+ – I don't have a pre-KK device to test on now.

> > Everything round in fxaccount.
> 
> This one's a bit more finicky. There's more moving parts here so maybe we
> can leave this for now and avoid causing some chaos.

Which moving parts?

Comment 5

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

$: ag "android:radius=\"[0-9]"
drawable/search_suggestion_button.xml
9:            <corners android:radius="4dp"/>
...

drawable/tab_thumbnail.xml
14:            <corners android:radius="3dp"/>
...

Search suggestions have a radius of 4dp and tab thumbnails have a radius of 3dp – should we update these too?

Comment 6

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

Build w/ tablet_browser_toolbar_menu_item_corner_radius & button_corner_radius = 4dp:
  https://people.mozilla.com/~mcomella/apks/mcomella-1210862_01.apk

Comment 7

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

Attachment: MozReview Request: Bug 1210862 - Rename button_corner_radius -> standard_corner_radius & update value. r=ally

Bug 1210862 - Rename button_corner_radius -> standard_corner_radius & update value. r=ally

Comment 8

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

Attachment: MozReview Request: Bug 1210862 - Move tablet corner radius to standard_button_radius. r=ally

Bug 1210862 - Move tablet corner radius to standard_button_radius. r=ally

Comment 9

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

(In reply to Michael Comella (:mcomella) from comment #4)
> > > Everything round in fxaccount.
> > 
> > This one's a bit more finicky. There's more moving parts here so maybe we
> > can leave this for now and avoid causing some chaos.
> 
> Which moving parts?

I'm just not confident the UI is exactly as I left it since the original design and implementation. I'm pretty sure it's been touched by others since so I'm hesitant to say we should just change this one.


(In reply to Michael Comella (:mcomella) from comment #5)
> $: ag "android:radius=\"[0-9]"
> drawable/search_suggestion_button.xml
> 9:            <corners android:radius="4dp"/>
> ...
> 
> drawable/tab_thumbnail.xml
> 14:            <corners android:radius="3dp"/>
> ...
> 
> Search suggestions have a radius of 4dp and tab thumbnails have a radius of
> 3dp – should we update these too?

WFM - let's do it

Comment 10

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

Comment on attachment 8670860 [details]
MozReview Request: Bug 1210862 - Rename button_corner_radius -> standard_corner_radius & update value. r=ally

Bug 1210862 - Rename button_corner_radius -> standard_corner_radius & update value. r=ally

Comment 11

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

Comment on attachment 8670861 [details]
MozReview Request: Bug 1210862 - Move tablet corner radius to standard_button_radius. r=ally

Bug 1210862 - Move tablet corner radius to standard_button_radius. r=ally

Comment 12

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

Attachment: MozReview Request: Bug 1210862 - Move search suggestion button & tab thumbnail radius to standard. r=ally

Bug 1210862 - Move search suggestion button & tab thumbnail radius to standard. r=ally

Comment 13

[Allison Naaktgeboren :ally]

Comment on attachment 8670860 [details]
MozReview Request: Bug 1210862 - Rename button_corner_radius -> standard_corner_radius & update value. r=ally

https://reviewboard.mozilla.org/r/21465/#review19437

One of those bugs where the hard work all comes before the patch. :)

Comment 14

[Allison Naaktgeboren :ally]

Comment on attachment 8670861 [details]
MozReview Request: Bug 1210862 - Move tablet corner radius to standard_button_radius. r=ally

https://reviewboard.mozilla.org/r/21467/#review19439

Thanks!

::: mobile/android/base/resources/values/dimens.xml
(Diff revision 2)
> -    <dimen name="tablet_browser_toolbar_menu_item_corner_radius">5dp</dimen>

I suspect I might be grateful you've done this work when I start Big Button Mode.

Comment 15

[Allison Naaktgeboren :ally]

Comment on attachment 8671540 [details]
MozReview Request: Bug 1210862 - Move search suggestion button & tab thumbnail radius to standard. r=ally

https://reviewboard.mozilla.org/r/21585/#review19441

::: mobile/android/base/resources/drawable/tab_thumbnail.xml:14
(Diff revision 1)
> -            <corners android:radius="3dp"/>            
> +            <corners android:radius="@dimen/standard_corner_radius"/>

thanks for getting rid of that whitespace while you're here

Thanks!

Comment 16

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

https://hg.mozilla.org/integration/fx-team/rev/d53aaea8928228b4c86bfd2b76a34ae96963bdd7
Bug 1210862 - Rename button_corner_radius -> standard_corner_radius & update value. r=ally

https://hg.mozilla.org/integration/fx-team/rev/92ae0a8c5183ec01d9afd3ebb62e77e2dd57fdf2
Bug 1210862 - Move tablet corner radius to standard_button_radius. r=ally

https://hg.mozilla.org/integration/fx-team/rev/e82e79d46b917ccb8a4083130c4ee9c21d85ca2f
Bug 1210862 - Move search suggestion button & tab thumbnail radius to standard. r=ally

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d53aaea89282
https://hg.mozilla.org/mozilla-central/rev/92ae0a8c5183
https://hg.mozilla.org/mozilla-central/rev/e82e79d46b91