Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla13

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

unspecified
mozilla13
Points:
---
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 601102 [details] [diff] [review]
WIP

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)
(Assignee)

Comment 1

6 years ago
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?
(Assignee)

Comment 5

6 years ago
(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.
(Assignee)

Comment 7

6 years ago
Oh, okay, I'll repair them then.
(Assignee)

Comment 8

6 years ago
Created attachment 602115 [details] [diff] [review]
WIP (mobile)

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)
(Assignee)

Comment 9

6 years ago
Created attachment 602120 [details] [diff] [review]
WIP (toolkit)

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+
(Assignee)

Comment 11

6 years ago
I did know that. Yet to figure out why, though.
(Assignee)

Comment 12

6 years ago
Created attachment 602305 [details] [diff] [review]
patch (mobile)
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+
(Assignee)

Comment 15

6 years ago
(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.
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6faa3058142b
https://hg.mozilla.org/integration/mozilla-inbound/rev/87d12a41e3e0
Flags: in-testsuite+
Flags: in-litmus-
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/6faa3058142b
https://hg.mozilla.org/mozilla-central/rev/87d12a41e3e0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 734053
You need to log in before you can comment on or make changes to this bug.