Closed Bug 731041 Opened 8 years ago Closed 8 years ago

Tidy up addon inline preferences code, add labels for checkboxes, remove ugly description hack

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch WIP (obsolete) — Splinter Review
Originally I was doing this just to get rid of the ugly description hack, but in the process I removed some code duplication, and tidied up a fair bit.

I also added capacity for settings with checkboxes to have a label on the checkbox. I don't know how mobile might handle that, but it's something I definitely want on desktop.
Attachment #601102 - Flags: feedback?(mbrubeck)
Attachment #601102 - Flags: feedback?(bmcbride)
FYI the patch doesn't show this existing comment on the function stripTextNodes

> // This function removes and returns the text content of aNode without
> // removing any child elements. Removing the text nodes ensures any XBL
> // bindings apply properly.

or the min-height property on <setting> elements in each theme.
Comment on attachment 601102 [details] [diff] [review]
WIP

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

Me like!

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2885,2 @@
>        var desc = stripTextNodes(setting).trim();
> +      if (!setting.hasAttribute("desc")) {

Seems the call to stripTextNodes() can be inside this if-statement now.

::: toolkit/mozapps/extensions/content/setting.xml
@@ +176,3 @@
>        </xul:vbox>
> +      <xul:hbox class="preferences-alignment">
> +        <xul:checkbox anonid="input" xbl:inherits="disabled,label=checkboxlabel" oncommand="inputChanged();"/>

Removing usage of onlabel/offlabel seems to affect mobile, which doesn't use "label".

(Based on a brief late-night glance at https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/bindings/checkbox.xml)

::: toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
@@ +755,5 @@
>    margin-top: 2em;
>  }
>  
> +.preferences-alignment {
> +  min-height: 33px;

Since this is dependent on the font size, em is more appropriate than px.
Attachment #601102 - Flags: feedback?(bmcbride) → feedback+
Comment on attachment 601102 [details] [diff] [review]
WIP

>   <binding id="setting-bool" extends="chrome://mozapps/content/extensions/setting.xml#setting-base">
>-    <content>
>-      <xul:vbox class="setting-label">
>+    <content align="start">
>+      <xul:vbox>

Fennec's themes style the "setting-label" element (giving it -moz-box-flex:1 to make the input portion right-aligned).  Can we keep the className on that element, please?

We'll need to make some other minor alignment changes in the Fennec themes too.  I can handle that, but can you please coordinate with me to make sure that Fennec is ready when this lands?

I don't think this causes any functional problems in Fennec, though I haven't tested too thoroughly yet.
Attachment #601102 - Flags: feedback?(mbrubeck) → feedback+
Comment on attachment 601102 [details] [diff] [review]
WIP

>   <binding id="setting-bool" extends="chrome://mozapps/content/extensions/setting.xml#setting-base">
>+    <content align="start">

We also need to override the alignment here, which we can't easily do if the attribute is set.  Can you remove attributes like "align" and "flex" and use styles instead where possible?
(In reply to Blair McBride (:Unfocused) from comment #2)
> Seems the call to stripTextNodes() can be inside this if-statement now.

It needs to be called in all cases, since the XBL won't apply if there's stray text nodes.

> ::: toolkit/themes/gnomestripe/mozapps/extensions/extensions.css
> @@ +755,5 @@
> >    margin-top: 2em;
> >  }
> >  
> > +.preferences-alignment {
> > +  min-height: 33px;
> 
> Since this is dependent on the font size, em is more appropriate than px.

It's based on the height of the largest control (<button>), which is >1em of text (not sure why, it's 17px when the font size is 14.66px) + 12px of border + 3em of margin in gnomestripe's case.

> Removing usage of onlabel/offlabel seems to affect mobile, which doesn't use
> "label".

(In reply to Matt Brubeck (:mbrubeck) from comment #3)
> Fennec's themes style the "setting-label" element (giving it -moz-box-flex:1
> to make the input portion right-aligned).  Can we keep the className on that
> element, please?
> 
> We'll need to make some other minor alignment changes in the Fennec themes
> too.  I can handle that, but can you please coordinate with me to make sure
> that Fennec is ready when this lands?
> 
> I don't think this causes any functional problems in Fennec, though I
> haven't tested too thoroughly yet.

Are you not overriding the content parts of the bindings in bug 696533?
(In reply to Geoff Lankow (:darktrojan) from comment #5)
> > I don't think this causes any functional problems in Fennec, though I
> > haven't tested too thoroughly yet.
> 
> Are you not overriding the content parts of the bindings in bug 696533?

We may override some of them in Native fennec, but we definitely do not override them in XUL Fennec:
http://mxr.mozilla.org/mozilla-central/source/mobile/xul/chrome/content/bindings.xml#1886

Since we are still shipping both products for now, we need to keep both of them working.
Oh, okay, I'll repair them then.
Attached patch WIP (mobile) (obsolete) — Splinter Review
I found a few things which just don't work right on mobile, so I've fixed them up:
* hid unsupported <setting>s
* removed the text node children to make the XBL bindings apply
* made textboxes flex properly
Attachment #601102 - Attachment is obsolete: true
Attachment #602115 - Flags: feedback?(mbrubeck)
Attachment #602115 - Flags: feedback?(bmcbride)
Attached patch WIP (toolkit)Splinter Review
Basically the same as the previous one with comments addressed. These two patches are building on tryserver for your testing pleasure(?) - I've been using the test addon in bug 653637.
https://tbpl.mozilla.org/?tree=Try&rev=982c6c04b6d3
Attachment #602120 - Flags: review?(mbrubeck)
Attachment #602120 - Flags: review?(bmcbride)
Comment on attachment 602115 [details] [diff] [review]
WIP (mobile)

Looks good.

I'm sure you'll be overjoyed to know that the styles in mobile/xul/themes/core/browser.css are duplicated in themes/core/gingerbread/browser.css and themes/core/honeycomb/browser.css, so you'll need to apply the same changes there.
Attachment #602115 - Flags: feedback?(mbrubeck) → feedback+
I did know that. Yet to figure out why, though.
Attached patch patch (mobile)Splinter Review
Attachment #602115 - Attachment is obsolete: true
Attachment #602305 - Flags: review?(mbrubeck)
Attachment #602305 - Flags: review?(bmcbride)
Attachment #602115 - Flags: feedback?(bmcbride)
Comment on attachment 602305 [details] [diff] [review]
patch (mobile)

>+++ b/mobile/android/chrome/content/aboutAddons.xhtml

This code was moved into a separate file aboutAddons.js in bug 730502.  Sorry for the conflict.  The code itself hasn't changed except for its indentation level, so it should be trivial to rebase.
Attachment #602305 - Flags: review?(mbrubeck) → review+
Comment on attachment 602120 [details] [diff] [review]
WIP (toolkit)

I haven't done a detailed review, just focused on anything that might affect Fennec.
Attachment #602120 - Flags: review?(mbrubeck) → review+
(In reply to Matt Brubeck (:mbrubeck) from comment #13)
> Comment on attachment 602305 [details] [diff] [review]
> patch (mobile)
> 
> >+++ b/mobile/android/chrome/content/aboutAddons.xhtml
> 
> This code was moved into a separate file aboutAddons.js in bug 730502. 
> Sorry for the conflict.  The code itself hasn't changed except for its
> indentation level, so it should be trivial to rebase.

Hitting a moving target? I'm good at that. Blair and Dave T give me plenty of practice.
Comment on attachment 602120 [details] [diff] [review]
WIP (toolkit)

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

(In reply to Geoff Lankow (:darktrojan) from comment #15)
> Hitting a moving target? I'm good at that. Blair and Dave T give me plenty
> of practice.

In all fairness, I do that to myself too.

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2888,1 @@
>        }

Nit: Remove unneeded { } brackets.
Attachment #602120 - Flags: review?(bmcbride) → review+
Comment on attachment 602305 [details] [diff] [review]
patch (mobile)

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

::: mobile/android/chrome/content/aboutAddons.xhtml
@@ +378,5 @@
> +              var setting = settings[i];
> +              var desc = stripTextNodes(setting).trim();
> +              if (!setting.hasAttribute("desc")) {
> +                setting.setAttribute("desc", desc);
> +              }

Nit: Remove unneeded { } brackets.

::: mobile/xul/chrome/content/bindings/extensions.xml
@@ +139,5 @@
> +              var setting = settings[i];
> +              var desc = stripTextNodes(setting).trim();
> +              if (!setting.hasAttribute("desc")) {
> +                setting.setAttribute("desc", desc);
> +              }

Nit: Remove unneeded { } brackets.
Attachment #602305 - Flags: review?(bmcbride) → review+
(In reply to Blair McBride (:Unfocused) from comment #16)
> ::: toolkit/mozapps/extensions/content/extensions.js
> @@ +2888,1 @@
> >        }
> 
> Nit: Remove unneeded { } brackets.

Er, wow, those context lines aren't the most useful. Same brackets as I mentioned in the mobile patch.
Depends on: 734053
You need to log in before you can comment on or make changes to this bug.