495466 - new theme nitpicking

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

Attachment: theme nitpicking

see attached image;  all picked nits lettered, for easier tracking in comments

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

in addition, I see these problems:

1) favicon is smaller and doesn't fill the space provided.  Please increment size.
2) bookmarks-> "manage" appears much smaller than new buttons.  Please increase size of the manage button to come close to or match the size of the other buttons
3) in preferences, the yes/no buttons are to far right.  On the n810 screen it appears they are off the screen.
4) in general, the preferences look dull compared to the flashy UI pieces we have.

I am not sure if this should be in this bug or a separate bug:
after bookmarking a website, click on the yellow star and you are not taken to the edit bookmark dialog as expected.

Comment 2

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

(In reply to comment #0)
> Created an attachment (id=380448) [details]
> theme nitpicking
> 
> see attached image;  all picked nits lettered, for easier tracking in comments

a - This should be part of the endcap image, like the SSL and EV images. Put on martell's list
b - I can add more padding, but at some point it is only chewing up usable space. Currently it's 0.5mm (or 4.4px on n810). Want to make it 1mm (8.8px on n810)?
c - Same as b for padding. As for thumbnails, should we make them smaller to fit full screen, non-fullscreen - or can we leave the size alone and make the number displayed be dynamic, based on the available space?
d - OK
e - OK, but no shadow under the URLbar. It will conflict with the sidebars
g - OK, but how much? Currently 0.5mm (4.4px on n810). Want to try 1mm here too?
h - toolstrip size is based on padding, not hardcoded. Again, 0.5mm is used. Want to try 1mm?

Comment 3

[Madhava Enros [:madhava]]

(In reply to comment #2)

> a - This should be part of the endcap image, like the SSL and EV images. Put on martell's list

OK

> b - I can add more padding, but at some point it is only chewing up usable
> space. Currently it's 0.5mm (or 4.4px on n810). Want to make it 1mm (8.8px on
> n810)?

it chews up usable space, but only until it's scrolled off the screen.  And having the padding makes the field feel more rooted in something.  We could try 1 mm, but I suspect we might want even more.  I think we could live with a bit less than what's in the mockup.

> c - Same as b for padding. As for thumbnails, should we make them smaller to
> fit full screen, non-fullscreen - or can we leave the size alone and make the
> number displayed be dynamic, based on the available space?

Well, having the number be dynamic is the way to go, but I think we should be able to fit 4 + the button when it's fullscreen.


> d - OK
> e - OK, but no shadow under the URLbar. It will conflict with the sidebars

understood

> g - OK, but how much? Currently 0.5mm (4.4px on n810). Want to try 1mm here
> too?
> h - toolstrip size is based on padding, not hardcoded. Again, 0.5mm is used.
> Want to try 1mm?

For both of these -- I'd ramp it up until it looks like the mockup.  I think Sean's got a good looking size there.

Comment 4

[Madhava Enros [:madhava]]

Actually - just noticed another thing.  The thumbnails aren't quite the right aspect ratio - the contents looks a little squeezed, horizontally.

Comment 5

[Madhava Enros [:madhava]]

Also - rows, wherever they are (awesomebar rows, rows in prefs/addons/downloads) should all be a touch-height (7-8mm) tall.

Comment 6

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

Attachment: WIP - has most of the fixes; still needs a few padding tweaks

This patch is almost ready. Just need to add the padding fixes for the sidebars.

Comment 7

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

(In reply to comment #4)
> Actually - just noticed another thing.  The thumbnails aren't quite the right
> aspect ratio - the contents looks a little squeezed, horizontally.

Aspect ratios change per device, even on the same device with full/non-full screen differences. How do we want to handle it?

I am assuming you mean the aspect ratio of the webcontent viewport

Given n810 is 800x480 (fullscreen), we could save 15 pixels per thumbnail from the current size: 100x75 -> 100x60

Comment 8

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

(In reply to comment #3)
> (In reply to comment #2)

> > b - I can add more padding, but at some point it is only chewing up usable
> > space. Currently it's 0.5mm (or 4.4px on n810). Want to make it 1mm (8.8px on
> > n810)?
> 
> it chews up usable space, but only until it's scrolled off the screen.  And
> having the padding makes the field feel more rooted in something.  We could try
> 1 mm, but I suspect we might want even more.  I think we could live with a bit
> less than what's in the mockup.

I meant the space for the urlbar text, but we should be fine - even in portrait.

> 
> > c - Same as b for padding. As for thumbnails, should we make them smaller to
> > fit full screen, non-fullscreen - or can we leave the size alone and make the
> > number displayed be dynamic, based on the available space?
> 
> Well, having the number be dynamic is the way to go, but I think we should be
> able to fit 4 + the button when it's fullscreen.

Fixed padding. Tab sizing and/or dynamic spacing is another bug

> > g - OK, but how much? Currently 0.5mm (4.4px on n810). Want to try 1mm here
> > too?
> > h - toolstrip size is based on padding, not hardcoded. Again, 0.5mm is used.
> > Want to try 1mm?
> 
> For both of these -- I'd ramp it up until it looks like the mockup.  I think
> Sean's got a good looking size there.

Increased padding and button spacing. Not quite matched with martell's mockup, but close

Comment 9

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

(In reply to comment #5)
> Also - rows, wherever they are (awesomebar rows, rows in
> prefs/addons/downloads) should all be a touch-height (7-8mm) tall.

fixed row sizing. awesomebar row content is not centered. we can fix that in bug 470829

Comment 10

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

(In reply to comment #1)
> in addition, I see these problems:
> 
> 1) favicon is smaller and doesn't fill the space provided.  Please increment
> size.

scaled to 30px. we'll need a new throbber

> 2) bookmarks-> "manage" appears much smaller than new buttons.  Please increase
> size of the manage button to come close to or match the size of the other
> buttons

Fixed

> 3) in preferences, the yes/no buttons are to far right.  On the n810 screen it
> appears they are off the screen.

Increased the size of the radios. Should be better

Comment 11

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

Attachment: patch

Same as previous patch with the padding and spacing fixes in place. General guidelines:
* edge-to-element spacing: 2.2mm
* element-to-element spacing: 2.2mm
* row size: 14.4mm
* primary UI button size (toolbarbutton): 14.4mm
* secondary button size (button and radio): 12mm

These are the values in the CSS. Actual measurement size is about half the CSS value. Might be related to bug 449283.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 380571 [details] [diff] [review]
patch

What's the reasoning behind the various /* keep pixels */ comments? Just for borders?

What is /* core spacing */ supposed to signify?

I tend to prefer using align/pack attributes in the XUL rather than specifying that in CSS, but I don't feel strongly about that.

>diff --git a/themes/hildon/browser.css b/themes/hildon/browser.css

>-toolbarbutton {

>-  /* override default toolbarbutton padding/margin */

Keep this comment in the copy?

> /* favicon images are 16x16 */
> #urlbar-image-box {
>-  max-width: 24px;
>-  max-height: 24px;
>-  min-width: 24px;
>-  min-height: 24px;
>-  margin: 2px;
> }

>-/* hack to align with 1st grid/row */
>+/* hack to align with 1st grid/row
> .bookmark-item-image {
>   margin-top: 1mm;
> }
>-
>+*/

Get rid of these entirely, assuming this was intentional?

>diff --git a/themes/hildon/platform.css b/themes/hildon/platform.css

>-radio .radio-check-box1, .radio-check {
>+radio .radio-check-box1, .radio-icon {

radio-icon doesn't seem to exist either?

Comment 13

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

(In reply to comment #12)
> (From update of attachment 380571 [details] [diff] [review])
> What's the reasoning behind the various /* keep pixels */ comments? Just for
> borders?

Since we are moving to "mm" for as many things as possible, I wanted to call out when something needed to remain "px". This is usually because the pixel button images are involved somehow.

> What is /* core spacing */ supposed to signify?

I tried to use this in places where the "standard" padding or spacing size is used. I did this after changing the dimension 3 times and needing to remember all the places where the number needed to be changed. I did the same thing for "row size", "primary UI size" and "button size". CSS variables would be great for this.

> I tend to prefer using align/pack attributes in the XUL rather than specifying
> that in CSS, but I don't feel strongly about that.

I did it in CSS in case portrait/landscape or screen size needed to rearrange this. But, yeah, I normally do it in XUL too.
 
> >-toolbarbutton {
> 
> >-  /* override default toolbarbutton padding/margin */
> 
> Keep this comment in the copy?

Thought about it, but decided that nearly everything in platform.css was overriding the default. It seemed obvious, so I removed it.

> > /* favicon images are 16x16 */
> > #urlbar-image-box {
> >-  max-width: 24px;
> >-  max-height: 24px;
> >-  min-width: 24px;
> >-  min-height: 24px;
> >-  margin: 2px;
> > }
> 
> >-/* hack to align with 1st grid/row */
> >+/* hack to align with 1st grid/row
> > .bookmark-item-image {
> >   margin-top: 1mm;
> > }
> >-
> >+*/
> 
> Get rid of these entirely, assuming this was intentional?

Oops. Removed.

> >diff --git a/themes/hildon/platform.css b/themes/hildon/platform.css
> 
> >-radio .radio-check-box1, .radio-check {
> >+radio .radio-check-box1, .radio-icon {
> 
> radio-icon doesn't seem to exist either?

actually, on windows .radio-check-box1 exists and on linux/mac, .radio-check is needed. But .radio-icon exists on all 3 platforms. Basically, I am trying to hide the 2 <image>s that appear in the radio XBL binding. Switched to:

radio .radio-check-box1, radio .radio-icon, radio .radio-check {
  display: none;
}

Comment 14

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

http://hg.mozilla.org/mobile-browser/rev/b61aee6ed25d

bookmark editor and tab thumbnail sizing were spun off as separate bugs