690584 - [TABLETUI] Control panel needs action bar in Honeycomb

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

The control panel in honeycomb needs an action bar that takes back to the tabs.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

This patch adds the required action bar to the honeycomb theme.
This also hides it from other themes.
As per other native applications in honeycomb, only the icon representing the product will be clickable. The title will be a label - which cannot be clickable.

Comment 2

[Wesley Johnston (:wesj)]

Comment on attachment 563573 [details] [diff] [review]
WIP

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

hg add the new image. Mostly need to move to using classes instead.

::: mobile/chrome/content/browser.xul
@@ +411,5 @@
>           style="-moz-stack-sizing: ignore" left="10000">
> +      <hbox id="back-bar">
> +        <button id="settings-back" image="chrome://branding/content/logo.png" command="cmd_panel"/>
> +        <label id="settings-label" value="&actionbar.default;"/>
> +      </hbox>

We should use classes for most of this styling if possible. That way we can reuse the styles in other dialogs.

::: mobile/themes/core/honeycomb/browser.css
@@ +451,5 @@
> +
> +#settings-back {
> +  -moz-user-focus: ignore;
> +  margin: 0;
> +  padding: 0 @padding_large@ 0 -moz-calc(2 * @padding_large@) !important;

The 2*padding_large works because settings-back happens to be about the right width here? If thats the case (I'm guessing?) Maybe a pixel value for that with a comment explaining where it comes from would be better.

@@ +464,5 @@
> +  background-color: @color_background_highlight@;
> +}
> +
> +#settings-label {
> +  padding: @padding_xxxnormal@ 0 0;

I'd rather use -moz-box-align on the back-bar if possible for this. There's a few times I haven't been abel to find any good way to position a label with that.

::: mobile/themes/core/honeycomb/platform.css
@@ +879,5 @@
>    }
>    
>    #panel-container {
>      -moz-box-pack: center;
> +    padding-bottom: @padding_xlarge@;

Why do we have two different rules for panel-container in here?

Comment 3

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

Comment on attachment 563573 [details] [diff] [review]
WIP

>diff --git a/mobile/chrome/content/browser.xul b/mobile/chrome/content/browser.xul

>+      <hbox id="back-bar">
>+        <button id="settings-back" image="chrome://branding/content/logo.png" command="cmd_panel"/>
>+        <label id="settings-label" value="&actionbar.default;"/>
>+      </hbox>

These IDs are too generic. Lets use "panel-actionbar", "panel-actionbar-button", "panel-actionbar-label"

>diff --git a/mobile/themes/core/honeycomb/browser.css b/mobile/themes/core/honeycomb/browser.css

>+#settings-back .button-icon {
>+  height: 48px;

Set width too, just to be explicit. Is 48px the natural size of the logo?

>+  skin/honeycomb/images/settings-back.png             (honeycomb/images/settings-back.png)

settings-back-hdpi.png

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patches has the following changes:
1. The id names have been changed.
2. The id of label has been changed to be a class. I feel it is better for the button to have an id and the label to have a class - for resuability.
3. The padding has been changed a bit to have an equal padding action-bar and a variable padding panel-container-inner.
4. A width is not added for the button, as our logo is not of equal height and width. The aspect-ratio screws up, hence only height is used.

Other clarifications:
1. The height of 48px is as per Ian's mockup.
2. I wouldn't want to use pixels in -moz-calc() as I'm not sure how this would affect different devices' dpi.

Queries:
1. The Android Market app uses a similar theme for phones and tablet. Shall we preserve it for honeycomb? (Currently it does in this patch).

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 564317 [details] [diff] [review]
Patch

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

::: mobile/chrome/content/browser.xul
@@ +411,5 @@
>           style="-moz-stack-sizing: ignore" left="10000">
> +      <hbox id="panel-actionbar">
> +        <button id="panel-actionbar-button" image="chrome://branding/content/logo.png" command="cmd_panel"/>
> +        <label class="panel-actionbar-label" value="&actionbar.default;"/>
> +      </hbox>

I still very much feel like you should add some classes here and use them for styling. I'd like to use class="actionbar" for the hbox, and "button-actionbar" for the buttons (like we do on the main actionbar). In fact... maybe we could use a toolbar element here instead to match what's used for the main browser a bit better too.

::: mobile/themes/core/honeycomb/browser.css
@@ +445,5 @@
>    list-style-image: url("chrome://browser/skin/images/settings-default-hdpi.png");
>  }
>  
> +#panel-actionbar {
> +  padding: @padding_normal@ @padding_large@;

using "-moz-box-align: center;" here rather than the padding below works for me.

@@ +451,5 @@
> +
> +#panel-actionbar-button {
> +  -moz-user-focus: ignore;
> +  margin: 0;
> +  padding: 0 @padding_large@ 0 -moz-calc(2 * @padding_large@) !important;

I still think we should use padding_large + width_of_image here. That way we can ensure the button fits regardless of dpi.

@@ +464,5 @@
> +  background-color: @color_background_highlight@;
> +}
> +
> +.panel-actionbar-label {
> +  padding: @padding_xxxnormal@ 0 0;

Kill this (see above).

Comment 6

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

Comment on attachment 564317 [details] [diff] [review]
Patch

I agree with Wes about using classes. Feel free to keep the IDs too, but classes can be reused incase we end up with more nuttons in the actionbar (it could happen)

Also, "chrome://branding/content/logo.png" is 145x105 and will be squeezed to get  to 48px. I'd rather use "chrome://branding/content/favicon32.png" for now. Yes, I know the fennec favicon32 is actually 30px. We still need to fix that. Please assume 32px for the image and use favicon32.png until we get something different.

Comment 7

[Sriram Ramasubramanian [:sriram]]

Gone native!