494551 - Use images for buttons in main chrome UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

We tried using a 3D CSS3-based theme to create image-less buttons, but performance regressed. So, we did a quick replace with a flat CSS-based theme and performance got a lot better.

Now we are going back to using images to create nice 3D buttons.

Comment 1

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

Attachment: patch for xul and widget changes

This patch contains the structural part of the changes. Changes to the XUL and JS needed to better support a them with both images (pixel dependent) and physical sizes in millimeters (pixel independent).

I added code to WidgetStack to allow for updating the location of unfrozen widgets, which we need since we don't really know the pixel sizes of things until they are rendered. We don't hard code sizes in XUL anymore, whenever possible.

Comment 2

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

Attachment: patch for hildon theme changes

This patch has the required changes to the hildon theme. Changes to CSS and new images.

Once we get this part reviewed, I can create a copy of the finished patch for WinCE.

Comment 3

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

Comment on attachment 379966 [details] [diff] [review]
patch for hildon theme changes

I wonder whether a Toolbar.png-like approach with image-regions would be better for perf. I don't recall whether we've done any testing or reached any conclusions there.

some minor issues:
-padding for the identity box seems broken for non-SSL (text doesn't line up, is too close to image)
-tool-panel-open does't have an active image, should it?

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

> #tool-panel-open {
>+  margin-right: -40px; /* big number just to get padding margins to kick in */

> #tool-panel-close {
>+  margin-left: -40px; /* big number just to get padding margins to kick in */

I don't really understand these comments, maybe you should elaborate?

> #tabs {

>+  padding: 0;
>+  padding-top: 0; /* align to top of web content */

Why are both needed?

> richlistitem[type="documenttab"] {
>-  -moz-appearance: none;

Do we really not want that anymore?

>diff --git a/themes/hildon/browser.css.rej b/themes/hildon/browser.css.rej
>new file mode 100644
>--- /dev/null
>+++ b/themes/hildon/browser.css.rej

Er, don't land this :)

>diff --git a/themes/hildon/jar.mn b/themes/hildon/jar.mn

>-  images/tag.png                       (images/tag.png)

Seems like there's still a reference to this one in browser.css:
./browser.css:  list-style-image: url("chrome://browser/skin/images/tag.png");

though I guess we don't use it because of bug 470829 anyways.

>+  images/unlock-40.png                 (images/unlock-40.png)
>+  images/tab-default.png               (images/tab-default.png)
>+  images/tab-active.png                (images/tab-active.png)

These appear to be unused?

Comment 4

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

(In reply to comment #3)
> (From update of attachment 379966 [details] [diff] [review])
> I wonder whether a Toolbar.png-like approach with image-regions would be better
> for perf. I don't recall whether we've done any testing or reached any
> conclusions there.

I think the Toolbar.png like approach would be ok, if it was vertically designed. Horizontal image regions can cause slowdowns because the memory is not contiguous. See Stuart for more details.

> some minor issues:
> -padding for the identity box seems broken for non-SSL (text doesn't line up,
> is too close to image)

Fixing in bug 489433 (larry)

> -tool-panel-open does't have an active image, should it?

Nope, we never show it active. We also never show tool-panel-close in the active state. The images are designed to handle the correct "look"

> >diff --git a/themes/hildon/browser.css b/themes/hildon/browser.css
> 
> > #tool-panel-open {
> >+  margin-right: -40px; /* big number just to get padding margins to kick in */
> 
> > #tool-panel-close {
> >+  margin-left: -40px; /* big number just to get padding margins to kick in */
> 
> I don't really understand these comments, maybe you should elaborate?

Adding more, but here's the details: These buttons are designed to "run off" the edge of the window. *-open is in a right-side strip, *-close is in a left-side strip. The strips have -moz-box-align such that the alignment is opposite of the edge the buttons should "run-off". I use a too big negative margin to just make sure the buttons are clipped by the edge of the hbox (maybe I could use overflow-x somehow instead). Mainly an experiment that gave me the intended effect. Could probably be done better.

> > #tabs {
> 
> >+  padding: 0;
> >+  padding-top: 0; /* align to top of web content */
> 
> Why are both needed?

They're not. Removed padding-top

> > richlistitem[type="documenttab"] {
> >-  -moz-appearance: none;
> 
> Do we really not want that anymore?

Oops. Added back

> >diff --git a/themes/hildon/browser.css.rej b/themes/hildon/browser.css.rej
> >new file mode 100644
> >--- /dev/null
> >+++ b/themes/hildon/browser.css.rej
> 
> Er, don't land this :)

Oops. Removed. A little too hg addremove happy

> >diff --git a/themes/hildon/jar.mn b/themes/hildon/jar.mn
> 
> >-  images/tag.png                       (images/tag.png)
> 
> Seems like there's still a reference to this one in browser.css:
> ./browser.css:  list-style-image: url("chrome://browser/skin/images/tag.png");
> 
> though I guess we don't use it because of bug 470829 anyways.

I changed the reference to "tag-default-30.png". Also changed the "starred" imgae to "star-default-30.png". These are both in prep for bug 470829.

> >+  images/unlock-40.png                 (images/unlock-40.png)
> >+  images/tab-default.png               (images/tab-default.png)
> >+  images/tab-active.png                (images/tab-active.png)
> 
> These appear to be unused?

the unlock-40.png and lock-40.png are for the new larry (bug 489433). The "tab-*.png" images have been removed since we switched to non-images for styling them.

Comment 5

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

http://hg.mozilla.org/mobile-browser/rev/d5db27d4be1a
http://hg.mozilla.org/mobile-browser/rev/ccb35acec348

Comment 6

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

Forgot I need to post the WinCE part of the patch too

Comment 7

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

Found a silly CSS error. pushed fix:
http://hg.mozilla.org/mobile-browser/rev/87e6f4cf0dc4

Comment 8

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

Attachment: patch for wince theme changes

This is a patch for the themes\wince changes:
* all images should match those used in hildon
* browser.css and platform.css should match (except for the very top of platform.css where we have some maemo font names)
* jar.mn has the new list of images

no new changes were made. just bringing the wince side up to date.

Comment 9

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

http://hg.mozilla.org/mobile-browser/rev/64ce55ad512e