The default bug view has changed. See this FAQ.

Pretty up the Control Panels

VERIFIED FIXED in Firefox 9

Status

Fennec Graveyard
General
P3
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: nhirata, Assigned: sriram)

Tracking

(Blocks: 1 bug, {verified-aurora})

Trunk
Firefox 9
ARM
Android
verified-aurora
Dependency tree / graph

Details

Attachments

(12 attachments, 8 obsolete attachments)

156.75 KB, image/png
Details
114.87 KB, application/zip
Details
91.34 KB, image/png
Details
42.79 KB, image/png
Details
19.90 KB, application/zip
Details
62.46 KB, image/png
Details
48.77 KB, image/png
Details
86.86 KB, image/png
Details
52.35 KB, image/png
Details
56.00 KB, image/png
Details
12.55 KB, image/png
Details
34.24 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
Created attachment 556127 [details]
screenshot

Error console with the All, Messages, Warnings, Errors does not seem to match the color selection scheme for the Panel selections on the left hand side.

Mozilla/5.0 (Android; Linux armv7I; rv9.0a1) Gecko/20110826 Firefox/9.0a1 Fennec/9.0a1
Device: Toshiba Thrive
OS Android 3.1
Blocks: 655762, 653136

Updated

6 years ago
Priority: -- → P3
Ian, how should these look in honeycomb?
Keywords: uiwanted
Ian provided this mock-up:
https://secure.flickr.com/photos/61892693@N03/6101214910/in/photostream/lightbox/
Depends on: 685309
Created attachment 559171 [details]
Font / colour specs

Mockup for landscape mode http://www.flickr.com/photos/61892693@N03/6101214910/in/photostream/lightbox/
Mockup for portrait mode http://www.flickr.com/photos/61892693@N03/6101205906/in/photostream/lightbox/

Specs are attached here. Visual assets will follow shortly.
Created attachment 559177 [details]
Graphic assets for control panels
(Assignee)

Updated

6 years ago
Assignee: nobody → sriram
Created attachment 559529 [details]
New backgrounds to hopefully fix our banding issues

Comment 6

6 years ago
Today's Nightly doesn't show the Control Panel as the one in this bug.

Mozilla/5.0 (Android; Linux armv7l; rv:9.0a 1)Gecko/20110909 Firefox/9.0a1 Fennec/9.0a1
Samsung Galaxy Tab 10.1
Created attachment 559588 [details]
gmail on honeycomb

This shows the GMail setting/preferences page. I am posting this since we are using a similar to GMail, white-chrome style in the main application area. Wouldn't we want to use a preferences UI that is white-chrome too?
Also, it's no secret that I don't like gradients and large images. Especially large gradient images. :)
Sure, we could do that. I'll mock something up and we can talk about it.
No longer blocks: 653136
Created attachment 560891 [details]
Updated settings UI

Here's a mockup of the new styling on the settings pages. I'll post new assets and specs shortly.
Created attachment 560893 [details]
Graphic assets for control panels

Updated assets to go with the new light background design
Attachment #559177 - Attachment is obsolete: true
Created attachment 560908 [details]
Font / colour specs
Attachment #559171 - Attachment is obsolete: true
Created attachment 561031 [details]
Margins and spacing measurements
Created attachment 561251 [details]
Font sizes, relative to the rest of our UI
(Assignee)

Comment 15

6 years ago
Created attachment 561633 [details] [diff] [review]
VWIP

Very wet in paint patch.
Works in both portrait and landscape mode.

Need to add changes to downloads and addons.
Created attachment 561826 [details]
mockup for downloads
Created attachment 561827 [details]
mockup for addons
Created attachment 561828 [details]
specs for buttons
(Assignee)

Comment 19

6 years ago
Created attachment 561929 [details] [diff] [review]
WIP
Attachment #561929 - Flags: review?(wjohnston)
Attachment #561929 - Flags: review?(mark.finkle)
(Assignee)

Comment 20

6 years ago
(In reply to Sriram Ramasubramanian [:sriram] from comment #19)
> Created attachment 561929 [details] [diff] [review]
> WIP

This contains the patch for preferences and downloads pane.
The addons pane needs to be cleared a bit.
(Assignee)

Updated

6 years ago
Attachment #561633 - Attachment is obsolete: true
Comment on attachment 561929 [details] [diff] [review]
WIP

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

This looks good to me. I haven't tested it yet. Mostly just some comments below, but nothing I think we should hold up landing this for.

::: mobile/themes/core/honeycomb/browser.css
@@ +1450,5 @@
> +  padding: @padding_snormal@ 0 0 0;
> +}
> +
> +setting[type="menulist"] .setting-label > .preferences-description {
> +  display: none;

Are we sure we're never going to want to show this?

::: mobile/themes/core/honeycomb/defines.inc
@@ +56,5 @@
>  %define font_tiny 2.48mozmm
>  %define font_xtiny 2.27mozmm
>  %define font_xxtiny 1.93mozmm
>  
> +%define touch_row 10.478mozmm

lol. changed from 10.41 to 10.478?

::: mobile/themes/core/honeycomb/platform.css
@@ +817,5 @@
> +
> +#panel-controls > .panel-row-button {
> +  border-top: @border_width_tiny@ solid @color_background_settings@ !important;
> +  border-bottom: @border_width_large@ solid !important;
> +  -moz-border-bottom-colors: @color_panel_row_border@ @color_background_settings@ !important;

Can you file a bug for us to go through our theme code and evaluate all of these !important rules. I'd like to clean them up sometime.

::: mobile/themes/core/jar.mn
@@ +394,5 @@
>    skin/honeycomb/images/check-30.png                  (images/check-30.png)
>    skin/honeycomb/images/check-selected-hdpi.png       (honeycomb/images/check-selected-hdpi.png)
>    skin/honeycomb/images/check-unselected-hdpi.png     (honeycomb/images/check-unselected-hdpi.png)
> +  skin/honeycomb/images/check-selected-tap-hdpi.png   (honeycomb/images/check-selected-tap-hdpi.png)
> +  skin/honeycomb/images/check-unselected-tap-hdpi.png (honeycomb/images/check-unselected-tap-hdpi.png)

We should also check if we really need 6 different checkmark images in this theme anymore.
Attachment #561929 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 22

6 years ago
Created attachment 562141 [details] [diff] [review]
Control Panel patch

This patch includes all changes.

Todos:
1. The addons header needs to be revisited.
2. The Back button bar on top needs to be added.
Attachment #561929 - Attachment is obsolete: true
Attachment #561929 - Flags: review?(mark.finkle)
Attachment #562141 - Flags: review?(wjohnston)
Attachment #562141 - Flags: review?(mark.finkle)
(Assignee)

Comment 23

6 years ago
https://bugzilla.mozilla.org/show_bug.cgi?id=685334 - requires the patch in this to be applied for the above patch to work.
(Assignee)

Updated

6 years ago
Depends on: 685334
Comment on attachment 562141 [details] [diff] [review]
Control Panel patch

>+#addons-list richlistitem.section-header {
>+  margin: 0;
>+  padding: @padding_xxnormal@ @padding_large@;
>+}
>+
>+#addons-list richlistitem:not([class="section-header"])[selected="true"] {
>+  background-color: @color_background_settings@;
>+  border-top: @border_width_tiny@ solid @color_text_button@;
>+  border-bottom: @border_width_tiny@ solid @color_text_button@;
>+}
>+
>+#addons-list richlistitem:not([class="section-header"]):not([selected="true"]):hover:active {
>+  background-color: @color_background_highlight@;
>+}

I am wondering why we want add-on section-headers should be different than other section-headers? Can't we tweak the platform.css richlistitem.section-header rules?

>+richlistitem[typeName="local"],
>+richlistitem[typeName="search"],
>+richlistitem[typeName="searchplugin"],
>+richlistitem[typeName="banner"] {
>+  padding: @padding_xxxnormal@ 0 @padding_tiny@ @padding_xlarge@; 
>+  border-bottom: @border_width_tiny@ solid @color_button_border@;
>+}
>+
>+richlistitem[typeName="local"] image,
>+richlistitem[typeName="search"] image,
>+richlistitem[typeName="searchplugin"] image,
>+richlistitem[typeName="banner"] image {
>+  margin-right: @margin_xxxnormal@;

for RTL safety, use -moz-margin-end

>+
>+richlistitem[typeName="local"] label.normal,
>+richlistitem[typeName="local"] description.normal,
>+richlistitem[typeName="search"] label.normal,
>+richlistitem[typeName="search"] description.normal,
>+richlistitem[typeName="searchplugin"] label.normal,
>+richlistitem[typeName="searchplugin"] description.normal,
>+richlistitem[typeName="banner"] label.normal,
>+richlistitem[typeName="banner"] description.normal {
>+  color: @color_subtext_settings@;
>+}

You do the same 3 types of rules for downloads. Maybe we could add a class and get them all instead of listing so many and listing them in add-ons and downloads.

> .addon-image,
> .searchplugin-image {

>-  -moz-margin-start: @margin_normal@;
>   -moz-margin-end: @margin_normal@;
>+  margin-left: @margin_large@;

For RTL safety, use -moz-margin-start

> /* downloads panel UI   ---------------------------------------------------- */
>+#downloads-list richlistitem[selected="true"] {
>+  background-color: @color_background_settings@;
>+}
>+
>+#downloads-list richlistitem:hover:active {
>+  background-color: @color_background_highlight@;
>+}
>+
>+richlistitem[typeName="download"] {
>+  padding: @padding_xxxnormal@ 0 @padding_tiny@ @padding_xlarge@; 
>+  border-bottom: @border_width_tiny@ solid @color_button_border@;
>+}
>+
>+richlistitem[typeName="download"] image {
>+  margin-right: @margin_xxxnormal@;

For RTL safety, use -moz-margin-end

>+}
>+
>+richlistitem[typeName="download"] label.normal,
>+richlistitem[typeName="download"] description.normal {
>+  color: @color_subtext_settings@;
>+}

Move this to platform? I commented there too.

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

> .toolbarbutton-icon[label]:not([label=""]),
> .toolbarbutton-icon[type="menu"] {
>   -moz-margin-end: @margin_tiny@;
>+  margin-left: @margin_xxxnormal@;
>+  margin-right: @margin_xnormal@;

For RTL safetly, use -moz-margin-end and -moz-margin-start

> richlistitem label.normal,
> richlistitem description.normal {
>   color: @color_subtext_default@;

You could safely change this to use color_settings_default and not add the rules for add-ons and downloads


>+#panel-controls > .panel-row-button {

>+  border-left: 0 solid; 
>+  border-right: 0 solid; 

For RTL safety, -moz-border-start and -moz-border-end

r- for the RTL stuff and I want to see if we can move more of these changes to platform.css
Attachment #562141 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 25

6 years ago
Created attachment 562190 [details] [diff] [review]
Control Panel patch

Most possible settings have been moved in platform.css.
This didn't affect anything as much as I checked.
RTL support specific "-moz-*-start" and "-moz-*-end" have been added.

I would like to move the settings to platform.css
Attachment #562141 - Attachment is obsolete: true
Attachment #562141 - Flags: review?(wjohnston)
Attachment #562190 - Flags: review?(wjohnston)
Attachment #562190 - Flags: review?(mark.finkle)
Comment on attachment 562190 [details] [diff] [review]
Control Panel patch

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

> .options-box {
>-  -moz-margin-start: 28px;  /* sized based on the 32px addon image */
>+  -moz-margin-start: @margin_xlarge@;

This worries me a little since the image is still based on pixels

>-#appmenu-popup-appcommands richlistitem:hover:active {
>-  background-color: @color_background_highlight_overlay@;
>-}
>-

Leave this. We need it.

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

> button:focus > .button-box {
>-  border: @border_width_tiny@ solid transparent;
> }

If we don't need this, remove the entire rule

> toolbarbutton {
>   min-width: @touch_button_small@ !important; /* primary button size */
>-  min-height: @touch_button_small@ !important; /* primary button size */
>+  min-height: @touch_button_tiny@ !important; /* primary button size */
>   -moz-appearance: none !important;
>   padding: @padding_xsmall@;
>   -moz-margin-end: @margin_xxxnormal@;
> }

This is causing trouble

> .toolbarbutton-icon[label]:not([label=""]),
> .toolbarbutton-icon[type="menu"] {
>-  -moz-margin-end: @margin_tiny@;
>+  -moz-margin-start: @margin_xxxnormal@;
>+  -moz-margin-end: @margin_xnormal@;
> }

This could cause trouble

> radio {
>   -moz-appearance: none;
>   min-width: @touch_button_small@ !important; /* button size */
>-  min-height: @touch_button_small@ !important; /* button size */
>+  min-height: @touch_button_tiny@ !important; /* button size */

I'm less worried about this one

> richlistitem {
>   -moz-user-focus: ignore;
>   min-height: @touch_row@; /* row size */
>-  padding: @padding_small@;
>+  padding-top: @padding_xxxnormal@;
>+  padding-bottom: @padding_tiny@;
>+  -moz-padding-start: @padding_xlarge@; 
>+  -moz-padding-end: 0;

Do we really need this level of tweaks to the padding?

> .prompt-button {
>   min-width: 33%;
>+  min-height: @touch_button_small@;

Doers this need an "!important" ?


>+@media (@orientation@: portrait) and (min-width: @tablet_panel_minwidth@) {
>+  #panel-controls > .panel-row-button {
>+    min-width: @tablet_panel_controls@ !important;
>+  }
>+  
>+  #panel-items {
>+    max-width: @tablet_panel_item_width@ !important;
>+  }
>+}
>+
>+@media (@orientation@: landscape) and (min-width: @tablet_panel_minwidth@) {
>+  #panel-controls > .panel-row-button {
>+    min-width: @tablet_panel_controls_wide@ !important;
>+  }
>+  
>+  #panel-items {
>+    max-width: @tablet_panel_item_width_wide@ !important;
>+  }
>+}
>+

These rules should be in browser.css since they are specific to UI in browser.xul and are not general purpose rules.

>+#panel-container {
>+  background-color: @color_background_settings@ !important;
>+}
>+
> #panel-container-inner {
>   -moz-box-orient: vertical;
>+  background-color: transparent !important;
>+}
>+
>+#panel-container-inner > vbox {
>+  border: @border_width_large@ solid @color_background_active@; 
>+  box-shadow: 0 0 @shadow_width_tiny@ @shadow_width_medium@ @color_background_button_overlay@;  
> }
> 
> #panel-controls {
>   -moz-box-orient: horizontal;
>+  background-color: transparent !important;
>+  padding: @padding_xxxnormal@ 0 !important;
> }
> 
>+#panel-controls .toolbarbutton-text {
>+  font-size: @font_snormal@ !important;
>+}
>+
>+#panel-controls > .panel-row-button {
>+  border-top: @border_width_tiny@ solid @color_background_settings@ !important;
>+  border-bottom: @border_width_large@ solid !important;
>+  -moz-border-bottom-colors: @color_background_button@ @color_background_settings@ !important;
>+  -moz-border-start: 0 solid; 
>+  -moz-border-end: 0 solid; 
>+}
>+
>+#panel-controls > .panel-row-button[checked="true"] {
>+  background-color: @color_background_highlight@;
>+  color: @color_text_default@;
>+}
>+
>+.panel-list {
>+  background: transparent;
>+}
>+
>+#panel-items richlistitem:not([class="section-header"])[selected="true"] {
>+  background-color: @color_background_settings@;
>+}
>+
>+#panel-items richlistitem label.normal,
>+#panel-items richlistitem description.normal {
>+  color: @color_subtext_settings@;
>+}
>+
>+
> @media (min-width: @tablet_panel_minwidth@) {
>   #panel-container-inner {
>     -moz-box-orient: horizontal;
>     -moz-box-pack: center;
>   }
> 
>   #panel-items {
>-    max-width: @tablet_panel_minwidth@;
>     min-width: 0px !important;
>   }
>   
>   /* This will affect the prefs screen, but not the awesome screen */
>   #panel-controls {
>     -moz-box-orient: vertical !important;
>     -moz-box-align: start;
>   }
>   
>   #panel-controls > .panel-row-button {
>+    margin: 0;
>     -moz-box-orient: horizontal;
>     -moz-box-flex: 0;
>-    min-width: @tablet_panel_controls@ !important;
>   }
> 
>   #panel-controls .toolbarbutton-text {
>     display: -moz-box !important;
>     -moz-box-flex: 1;
>   }
>   
>   #panel-container {
>     -moz-box-pack: center;
>-    padding: @padding_xlarge@ 0px;
>+    padding: @padding_xxxlarge@ 0 @padding_xlarge@ 0;
>+  }
>+
>+  #panel-container-inner > vbox {
>+    padding: @padding_xlarge@ @padding_xxlarge@;
>   }
> }

These rules should be in browser.css since they are specific to UI in browser.xul and are not general purpose rules.
(I see some of this was already here. That makes me sad)
Attachment #562190 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 27

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #26)
> Comment on attachment 562190 [details] [diff] [review] [diff] [details] [review]
> Control Panel patch
> 
> >diff --git a/mobile/themes/core/honeycomb/browser.css b/mobile/themes/core/honeycomb/browser.css
> 
> > .options-box {
> >-  -moz-margin-start: 28px;  /* sized based on the 32px addon image */
> >+  -moz-margin-start: @margin_xlarge@;
> 
> This worries me a little since the image is still based on pixels
> 

The preferences in honeycomb has changed, and it doesn't align with the image, but the hbox containing the image. Hence this wouldn't affect the pixel based position we had earlier.

http://cl.ly/0i3p382S0r3L051n2j2L

> >-#appmenu-popup-appcommands richlistitem:hover:active {
> >-  background-color: @color_background_highlight_overlay@;
> >-}
> >-
> 
> Leave this. We need it.
> 

This is not needed anymore. This is a redundant rule. This is covered under richlistitem already.

> >diff --git a/mobile/themes/core/honeycomb/platform.css b/mobile/themes/core/honeycomb/platform.css
> 
> > button:focus > .button-box {
> >-  border: @border_width_tiny@ solid transparent;
> > }
> 
> If we don't need this, remove the entire rule

I guess, this was added because there was a border showing up due to global.css. I would test it and remove it if not needed.

> 
> > toolbarbutton {
> >   min-width: @touch_button_small@ !important; /* primary button size */
> >-  min-height: @touch_button_small@ !important; /* primary button size */
> >+  min-height: @touch_button_tiny@ !important; /* primary button size */
> >   -moz-appearance: none !important;
> >   padding: @padding_xsmall@;
> >   -moz-margin-end: @margin_xxxnormal@;
> > }
> 
> This is causing trouble

I've reverted this back to old one.
> 
> > .toolbarbutton-icon[label]:not([label=""]),
> > .toolbarbutton-icon[type="menu"] {
> >-  -moz-margin-end: @margin_tiny@;
> >+  -moz-margin-start: @margin_xxxnormal@;
> >+  -moz-margin-end: @margin_xnormal@;
> > }
> 
> This could cause trouble
> 
I haven't seen this breaking UI anywhere. I would change it, if we could find any.

> > radio {
> >   -moz-appearance: none;
> >   min-width: @touch_button_small@ !important; /* button size */
> >-  min-height: @touch_button_small@ !important; /* button size */
> >+  min-height: @touch_button_tiny@ !important; /* button size */
> 
> I'm less worried about this one
> 

I've reverted it to old one.

> > richlistitem {
> >   -moz-user-focus: ignore;
> >   min-height: @touch_row@; /* row size */
> >-  padding: @padding_small@;
> >+  padding-top: @padding_xxxnormal@;
> >+  padding-bottom: @padding_tiny@;
> >+  -moz-padding-start: @padding_xlarge@; 
> >+  -moz-padding-end: 0;
> 
> Do we really need this level of tweaks to the padding?
> 

Ibarlow's mockup demands it ;)

> > .prompt-button {
> >   min-width: 33%;
> >+  min-height: @touch_button_small@;
> 
> Doers this need an "!important" ?
> 

I guess, this was overriden by "button", hence had to add the !important.
I'll check it again.

> 
> >+@media (@orientation@: portrait) and (min-width: @tablet_panel_minwidth@) {
> >+  #panel-controls > .panel-row-button {
> >+    min-width: @tablet_panel_controls@ !important;
> >+  }
> >+  
> >+  #panel-items {
> >+    max-width: @tablet_panel_item_width@ !important;
> >+  }
> >+}
> >+
> >+@media (@orientation@: landscape) and (min-width: @tablet_panel_minwidth@) {
> >+  #panel-controls > .panel-row-button {
> >+    min-width: @tablet_panel_controls_wide@ !important;
> >+  }
> >+  
> >+  #panel-items {
> >+    max-width: @tablet_panel_item_width_wide@ !important;
> >+  }
> >+}
> >+
> 
> These rules should be in browser.css since they are specific to UI in
> browser.xul and are not general purpose rules.

Do we do it as a separate bug?
> 
> >+#panel-container {
> >+  background-color: @color_background_settings@ !important;
> >+}
> >+
> > #panel-container-inner {
> >   -moz-box-orient: vertical;
> >+  background-color: transparent !important;
> >+}
> >+
> >+#panel-container-inner > vbox {
> >+  border: @border_width_large@ solid @color_background_active@; 
> >+  box-shadow: 0 0 @shadow_width_tiny@ @shadow_width_medium@ @color_background_button_overlay@;  
> > }
> > 
> > #panel-controls {
> >   -moz-box-orient: horizontal;
> >+  background-color: transparent !important;
> >+  padding: @padding_xxxnormal@ 0 !important;
> > }
> > 
> >+#panel-controls .toolbarbutton-text {
> >+  font-size: @font_snormal@ !important;
> >+}
> >+
> >+#panel-controls > .panel-row-button {
> >+  border-top: @border_width_tiny@ solid @color_background_settings@ !important;
> >+  border-bottom: @border_width_large@ solid !important;
> >+  -moz-border-bottom-colors: @color_background_button@ @color_background_settings@ !important;
> >+  -moz-border-start: 0 solid; 
> >+  -moz-border-end: 0 solid; 
> >+}
> >+
> >+#panel-controls > .panel-row-button[checked="true"] {
> >+  background-color: @color_background_highlight@;
> >+  color: @color_text_default@;
> >+}
> >+
> >+.panel-list {
> >+  background: transparent;
> >+}
> >+
> >+#panel-items richlistitem:not([class="section-header"])[selected="true"] {
> >+  background-color: @color_background_settings@;
> >+}
> >+
> >+#panel-items richlistitem label.normal,
> >+#panel-items richlistitem description.normal {
> >+  color: @color_subtext_settings@;
> >+}
> >+
> >+
> > @media (min-width: @tablet_panel_minwidth@) {
> >   #panel-container-inner {
> >     -moz-box-orient: horizontal;
> >     -moz-box-pack: center;
> >   }
> > 
> >   #panel-items {
> >-    max-width: @tablet_panel_minwidth@;
> >     min-width: 0px !important;
> >   }
> >   
> >   /* This will affect the prefs screen, but not the awesome screen */
> >   #panel-controls {
> >     -moz-box-orient: vertical !important;
> >     -moz-box-align: start;
> >   }
> >   
> >   #panel-controls > .panel-row-button {
> >+    margin: 0;
> >     -moz-box-orient: horizontal;
> >     -moz-box-flex: 0;
> >-    min-width: @tablet_panel_controls@ !important;
> >   }
> > 
> >   #panel-controls .toolbarbutton-text {
> >     display: -moz-box !important;
> >     -moz-box-flex: 1;
> >   }
> >   
> >   #panel-container {
> >     -moz-box-pack: center;
> >-    padding: @padding_xlarge@ 0px;
> >+    padding: @padding_xxxlarge@ 0 @padding_xlarge@ 0;
> >+  }
> >+
> >+  #panel-container-inner > vbox {
> >+    padding: @padding_xlarge@ @padding_xxlarge@;
> >   }
> > }
> 
> These rules should be in browser.css since they are specific to UI in
> browser.xul and are not general purpose rules.
> (I see some of this was already here. That makes me sad)

Do we do it as a separate bug?
This is in other themes as well.
(Assignee)

Comment 28

6 years ago
Created attachment 562508 [details] [diff] [review]
Control Panel patch

This patch incorporates the required changes.
The toolbarbutton has its width preserved, fixing the back button issues in previous patch.
Attachment #562508 - Flags: review?(wjohnston)
Attachment #562508 - Flags: review?(mark.finkle)
Attachment #562190 - Attachment is obsolete: true
Attachment #562190 - Flags: review?(wjohnston)
(Assignee)

Comment 29

6 years ago
Created attachment 562579 [details] [diff] [review]
Control Panel patch

Changed the default richlistitem to have small padding. Reflect the changes in other places using "secondary-richlistitem" class.
Attachment #562508 - Attachment is obsolete: true
Attachment #562508 - Flags: review?(wjohnston)
Attachment #562508 - Flags: review?(mark.finkle)
Attachment #562579 - Flags: review?(mark.finkle)
Comment on attachment 562579 [details] [diff] [review]
Control Panel patch

>diff --git a/mobile/chrome/content/downloads.js b/mobile/chrome/content/downloads.js

>       item.setAttribute("lastSeconds", Infinity);
>+      item.setAttribute("class", "secondary-richlistitem");

Let's use "panel-listitem" (this is a list item in the panel area)

>diff --git a/mobile/chrome/content/extensions.js b/mobile/chrome/content/extensions.js
>     item.setAttribute("iconURL", aAddon.iconURL);
>+    item.setAttribute("class", "secondary-richlistitem");

Let's use "panel-listitem" (this is a list item in the panel area)

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

>+#tool-back2 image {
>+  -moz-margin-end: 0;
>+}

Do we really need this?

> /* browser tools panel UI   ------------------------------------------------ */
> .panel-header {
>   margin: 0;
>-  padding: @padding_small@ @padding_normal@ @padding_normal@ @padding_normal@;
>-  font-weight: bold;
>-  background-color: @color_background_header@;
>-  color: @color_text_header@ !important;
>-  background-repeat: repeat-x;
>-  background-size: auto 100%;
>+  padding: @padding_small@ @padding_normal@ @padding_xlarge@ @padding_normal@;
>+  border-bottom: @border_width_tiny@ solid @color_text_default@;
>+  background-color: transparent;
>+  color: @color_text_panel_header@ !important;
>+  font-size: @font_snormal@;
> }

Put the rule for .panel-listitem right here, not in platform.css

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

> .button-text,
> .toolbarbutton-text {
>   font-weight: normal;
>-  font-size: @font_snormal@ !important;
>+  font-size: @font_xtiny@ !important;

We need to make sure the prompt buttons are unaffected

> button {
>   -moz-appearance: none;
>   min-width: @touch_button_minwidth@ !important;
>-  min-height: @touch_button_small@ !important; /* button size */
>+  min-height: @touch_button_tiny@ !important; /* button size */

We need to make sure the prompt buttons are unaffected

>+  border-width: 0px;
>+  border-radius: 0px;

0 is unitless, pleased remove the "px"

>+.secondary-richlistitem {
>+  padding-top: @padding_xxxnormal@;
>+  padding-bottom: @padding_tiny@;
>+  -moz-padding-start: @padding_xlarge@; 
>+  -moz-padding-end: 0;
>+}

Renaming to .panel-listitem and moving to browser.css

>+#panel-items richlistitem:not([class="section-header"])[selected="true"] {
>+  background-color: @color_background_settings@;
>+}
>+
>+#panel-items richlistitem label.normal,
>+#panel-items richlistitem description.normal {
>+  color: @color_subtext_settings@;
>+}

Why can't these rules be used with .panel-listitem (secondary-richlistitem) ?

We try to avoid this type of all descendants selector


r-, but getting better
Attachment #562579 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 31

6 years ago
(In reply to Mark Finkle (:mfinkle) from comment #30)
> Comment on attachment 562579 [details] [diff] [review] [diff] [details] [review]
> Control Panel patch
> 
> >diff --git a/mobile/chrome/content/downloads.js b/mobile/chrome/content/downloads.js
> 
> >       item.setAttribute("lastSeconds", Infinity);
> >+      item.setAttribute("class", "secondary-richlistitem");
> 
> Let's use "panel-listitem" (this is a list item in the panel area)

I'll change this.

> 
> >diff --git a/mobile/themes/core/honeycomb/browser.css b/mobile/themes/core/honeycomb/browser.css
> 
> >+#tool-back2 image {
> >+  -moz-margin-end: 0;
> >+}
> 
> Do we really need this?
> 

There is a margin_tiny added to the image. This extends the image size of back button to 48x46.
But still, a '0' is not helping me shrink it to 46x46. There is something else affecting the back button. probably the -moz-calc.
This is needed to make sure the image is 46x46, though.

> > /* browser tools panel UI   ------------------------------------------------ */
> > .panel-header {
> >   margin: 0;
> >-  padding: @padding_small@ @padding_normal@ @padding_normal@ @padding_normal@;
> >-  font-weight: bold;
> >-  background-color: @color_background_header@;
> >-  color: @color_text_header@ !important;
> >-  background-repeat: repeat-x;
> >-  background-size: auto 100%;
> >+  padding: @padding_small@ @padding_normal@ @padding_xlarge@ @padding_normal@;
> >+  border-bottom: @border_width_tiny@ solid @color_text_default@;
> >+  background-color: transparent;
> >+  color: @color_text_panel_header@ !important;
> >+  font-size: @font_snormal@;
> > }
> 
> Put the rule for .panel-listitem right here, not in platform.css

I'll move it here.

> 
> >diff --git a/mobile/themes/core/honeycomb/platform.css b/mobile/themes/core/honeycomb/platform.css
> 
> > .button-text,
> > .toolbarbutton-text {
> >   font-weight: normal;
> >-  font-size: @font_snormal@ !important;
> >+  font-size: @font_xtiny@ !important;
> 
> We need to make sure the prompt buttons are unaffected
> 

I've made necessary changes to the prompt button.

> 
> >+#panel-items richlistitem:not([class="section-header"])[selected="true"] {
> >+  background-color: @color_background_settings@;
> >+}
> >+
> >+#panel-items richlistitem label.normal,
> >+#panel-items richlistitem description.normal {
> >+  color: @color_subtext_settings@;
> >+}
> 
> Why can't these rules be used with .panel-listitem (secondary-richlistitem) ?
> 
> We try to avoid this type of all descendants selector
> 

The richlistitem in preferences don't use ".secondary-richlistitem" (.panel-listitem). Do I add a class there? If I do that, these descendant selectors can be avoided.
(Assignee)

Comment 32

6 years ago
Created attachment 563125 [details] [diff] [review]
Control Panel patch

The secondary-richlistitem has been renamed now. And few more fixes have been made.
Attachment #562579 - Attachment is obsolete: true
Attachment #563125 - Flags: review?(mark.finkle)
(Assignee)

Comment 33

6 years ago
https://bugzilla.mozilla.org/attachment.cgi?id=563125&action=diff#a/mobile/themes/core/honeycomb/platform.css_sec6

- I would be happy to move these to browser.css. But it is present in Gingerbread also. Hence I would like to make that change as a separate bug instead - which takes care of all themes, to be consistent.
Comment on attachment 563125 [details] [diff] [review]
Control Panel patch

Good job. I like minimal changes.
Attachment #563125 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/ed87178d3ee2
Whiteboard: [inbound]

Comment 36

6 years ago
https://hg.mozilla.org/mozilla-central/rev/ed87178d3ee2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Keywords: uiwanted
Target Milestone: --- → Firefox 10
Depends on: 690849
Comment on attachment 563125 [details] [diff] [review]
Control Panel patch

Get the preferences UI looking better for tablets. It's been on nightly for a bit.
Attachment #563125 - Flags: approval-mozilla-aurora?

Comment 38

6 years ago
Comment on attachment 563125 [details] [diff] [review]
Control Panel patch

Naoki, we're taking this late in the game. Can you make sure that we get testing on the newly styled UI bits on all of the tablets we have availbale, as well as a quick check on phones to make sure we're not accidentally busting anything there. (abundance of caution)
Attachment #563125 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
If I'm not mistaken, this should only affect the Honeycomb tablets.

Verified on Toshiba Thrive Android OS 3.1; Samsung Tablet Android OS 3.1
Just to be on the safe side as per suggestion, I also tested on HTC Flyer Android 2.3 and tested on 2.2 Thunderbolt.

Marking qa+ for aurora verification once this lands in Aurora.
Status: RESOLVED → VERIFIED
Whiteboard: qa+
Asa, I sent out an email to the QA-mobile team to take a look at this bug on their own devices once this lands.  I will be testing on my own devices as well.
https://hg.mozilla.org/releases/mozilla-aurora/rev/ad21e8e83543
status-firefox9: --- → fixed
Target Milestone: Firefox 10 → Firefox 9
Depends on: 690870
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a2) Gecko/20111006 Firefox/9.0a2 Fennec/9.0a2
Samsung Galaxy Tab 10.01 (Android 2.3.3)
Keywords: verified-aurora
Whiteboard: qa+
(In reply to Aaron Train [:aaronmt] from comment #42)
> Mozilla/5.0 (Android; Linux armv7l; rv:9.0a2) Gecko/20111006 Firefox/9.0a2
> Fennec/9.0a2
> Samsung Galaxy Tab 10.01 (Android 2.3.3)

Whoops, typo, should read: Samsung Galaxy Tab 10.1 (Android 3.1)
Also verified:
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a2) Gecko/20111006 Firefox/9.0a2
Fennec/9.0a2

Device: HTC Flyer 
OS: Android 2.3

Device: Thunderbolt
OS: Android 2.2

Device: Droid 2 
OS: Android 2.2

Device: Toshiba Thrive
OS Android 3.1


Martijn also took a look.
You need to log in before you can comment on or make changes to this bug.