531102 - Change Fennec Hildon theme to use pixels for dimensions

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Bug 527750 and bug 530931 require that we stop using "mm" and start using "px" in our theme.

A test build with the theme exists here:
http://people.mozilla.com/~mfinkle/fennec/fennec-1.0b6pre.en-US.linux-gnueabi-arm.tar

A patch based on that will will be attached soon.

Comment 1

[Madhava Enros [:madhava]]

Some things noticed when testing the build

1. search provider bar doesn't seem to be pannable

2. very small type on first run page ( http://www.flickr.com/photos/madhava_work/4134150658/ )

3. are the black borders on fields thicker than before? ( http://www.flickr.com/photos/madhava_work/4133424369/ )

Comment 2

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

Attachment: patch

Sorry gavin! This patch converts our "mm" theme to "px". I used a few guidelines picked up from the hildon 2.2 UI guidelines:

margins/paddings should fall into: 4px, 8px, 16px and 32px groups. Most of our margins/paddings already fell very close to these groups.

font sizes should fall into: 24px and 18px groups. Ours already did, but I added a 14px group for the rare small fonts we have.

Other than that, I converted our "mm" to the closest sensible "px" value.

I fixed all places I found "mm" being used, which included the firstrun and about CSS too.

The bugs listed in comment 1 are fixed too.

Comment 3

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

Note: the N900 and N810 are using the same theme for now. It looks good enough to me on both.

Also, a test build with the above patch applied (and flat chrome) can be found here:
http://people.mozilla.com/~mfinkle/fennec/fennec-1.0b6pre.en-US.linux-gnueabi-arm.tar

Comment 4

[Madhava Enros [:madhava]]

new build fixes 1 and 3 from comment 1.

Re #2 - the text is definitely bigger, but is not too big -- it pushes the content below almost entirely off the screen.  The top of that content has be visible above the page fold.

Comment 5

[Madhava Enros [:madhava]]

illustration of the problem in comment 4
http://www.flickr.com/photos/madhava_work/4133721747/

Comment 6

[Madhava Enros [:madhava]]

also, the build seems to freeze up fairly frequently

Comment 7

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

(In reply to comment #5)
> illustration of the problem in comment 4
> http://www.flickr.com/photos/madhava_work/4133721747/

I'll bump the font sizes down in that CSS to better match the old sizes: 24px -> 20px and 18px -> 16px

Comment 8

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

(In reply to comment #6)
> also, the build seems to freeze up fairly frequently

Treat the build as "not official". I could have some old crap in the objdir. The theme was the only part I was focusing on.

Comment 9

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

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

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

> .searchengine {
>   -moz-box-orient: horizontal;
>-  min-width: 8mm;
>+  min-width: 140px !important;
> }

this seems like a disproportionate increase, but per IRC it is needed for panning to keep working (which I confirmed is true). Seems like we should get a bug on file for figuring out why that is... Also, why is the !important now needed?

>-  min-height: 6mm; /* row size */
>+  min-height: 0px; /* row size */

what's the reasoning here?

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

>-  color: #ddd !important;
>+  color: #7e7e7e !important;

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

>+  min-width: 64px !important;

Are there other bugs that cover these changes?

Comment 10

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

(In reply to comment #9)

> > .searchengine {
> >   -moz-box-orient: horizontal;
> >-  min-width: 8mm;
> >+  min-width: 140px !important;
> > }
> 
> this seems like a disproportionate increase, but per IRC it is needed for
> panning to keep working (which I confirmed is true). Seems like we should get a
> bug on file for figuring out why that is... Also, why is the !important now
> needed?

I'll file a bug on the panning issue.

!important is needed for http://mxr.mozilla.org/mobile-browser/source/themes/hildon/platform.css#250

> 
> >-  min-height: 6mm; /* row size */
> >+  min-height: 0px; /* row size */
> 
> what's the reasoning here?

Hmm, no idea. I think I typoed the "7" - it should be "70px"

> >-  color: #ddd !important;
> >+  color: #7e7e7e !important;

See bug 524206

> >+  min-width: 64px !important;

I was thinking of bug 517416 when I did this. I thought there was another too, but I can't find it now. We typically use 64px as out "touchable" toolbar button size. Previously it was only 40px wide - too small and frustrating to try to "hit".

Comment 11

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

> 
> >-  min-height: 6mm; /* row size */
> >+  min-height: 0px; /* row size */
> 
> what's the reasoning here?

Actually, we don't want to force the row height. We already have 4px top/bottom margin on this. Making the min-height 70px just increases the margin too much. The actual button sizes is unaffected.

I am removing the min-height from the rule

Comment 12

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

pushed:
https://hg.mozilla.org/mobile-browser/rev/914a3f23bbb2