516122 - Support more <textbox> styles and a <select> style widget in settings

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Fennec currently support toggles, buttons and strings settings in the preference list. We should add:

* Password and numeric string styles
* Multiple choice selection widget like <select> or <menulist>

See bug 509082 for additional background

Comment 1

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

Attachment: WIP 1

Turns out, <setting type="string"> supports inputtype attr which is used to set the internal <textbox type=> attr. So we can use it to make passwrod settings. This patch adds support for numeric strings too, by inheriting several other <textbox> attrs.

Th patch changes the "setting-button" to "setting-control" preparing for allowing <menulist> controls.

The patch allows moves setting.xml and checkbox.xml into chrome/content/bindings  - a simple housekeeping change.

I need to add support for <menulist> explicitly in this patch, or in a new patch. Still thinking about it...

Comment 2

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

Attachment: patch

This patch adds support for menulist in chrome using the SelectHelper. The patch creates two adapters (one for select and one for menulist) so the SelectHelper code can support either widget.

Comment 3

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

Attachment: patch 2

This patch fixes some small padding issues, limits CSS to dropmarkers inside menulists and adds the CSS to wince

Comment 4

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

Attachment: patch 3 - unbitrotted

Unbitrotted the patch

Comment 5

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

Comment on attachment 401232 [details] [diff] [review]
patch 3 - unbitrotted

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml

>+  <binding id="menulist" display="xul:box" extends="chrome://global/content/bindings/menulist.xml#menulist">

>+    <handlers>
>+      <handler event="mousedown" phase="capturing">
>+        <![CDATA[
>+          event.stopPropagation();

This is to prevent the normal popup from appearing, I guess?

>+      <handler event="click" button="0">
>+        <![CDATA[
>+          if (this.itemCount == 0)

Does this need to check "disabled"? (how does that work for <select>?)

>diff --git a/chrome/content/preferences/setting.xml b/chrome/content/bindings/setting.xml

>-  <binding id="setting-button" extends="chrome://browser/content/preferences/setting.xml#setting-base">
>+  <binding id="setting-control" extends="chrome://browser/content/preferences/setting.xml#setting-base">

Seems like you forgot to change the "extends" here (and everywhere else in this file). I guess this didn't break anything because we didn't clobber after applying this patch. Could just use relative URIs here anyways to avoid the problem.

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>   show: function(aControl) {

>+    if (aControl instanceof HTMLSelectElement)
>+      this._control = new SelectWrapper(aControl);
>+    else
>+      this._control = new MenulistWrapper(aControl);

Worth a check for instanceof XULMenuListElement to make the failure case more obvious just in case something weird happens?

>-      } else if (child instanceof HTMLOptionElement) {
>+      } else {

>         let item = document.createElement("option");
>-        item.setAttribute("label", child.textContent);
>+        item.setAttribute("label", this._control.getText(child));

Isn't it possible for _control.children to include non-HTMLOptionElement and non-HTMLOptGroupElement children in the <select> case? Won't getting .text (via getText()) throw in that case due to this change?

I guess you should filter such elements out of .children if that's the case. The previous code would have shown the textContent, which is still wrong but not fatal.

r=me with those addressed.

Comment 6

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

(In reply to comment #5)
> (From update of attachment 401232 [details] [diff] [review])
> >+      <handler event="mousedown" phase="capturing">
> >+        <![CDATA[
> >+          event.stopPropagation();
> 
> This is to prevent the normal popup from appearing, I guess?

Yep, added a comment

> >+      <handler event="click" button="0">
> >+        <![CDATA[
> >+          if (this.itemCount == 0)
> 
> Does this need to check "disabled"? (how does that work for <select>?)

It does need to check for disabled. The <select> handler doesn't. Add the check.

> Seems like you forgot to change the "extends" here (and everywhere else in this
> file). I guess this didn't break anything because we didn't clobber after
> applying this patch. Could just use relative URIs here anyways to avoid the
> problem.

Fixed the file URIs

> >+    if (aControl instanceof HTMLSelectElement)
> >+      this._control = new SelectWrapper(aControl);
> >+    else
> >+      this._control = new MenulistWrapper(aControl);
> 
> Worth a check for instanceof XULMenuListElement to make the failure case more
> obvious just in case something weird happens?

Added a check for Ci.nsIDOMXULMenuListElement

> >-      } else if (child instanceof HTMLOptionElement) {
> >+      } else {
> 
> Isn't it possible for _control.children to include non-HTMLOptionElement and
> non-HTMLOptGroupElement children in the <select> case? Won't getting .text (via
> getText()) throw in that case due to this change?
> 
> I guess you should filter such elements out of .children if that's the case.
> The previous code would have shown the textContent, which is still wrong but
> not fatal.

I added a .isOption(aChild) helper that checks for the explicit type

Comment 7

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

pushed:
https://hg.mozilla.org/mobile-browser/rev/1bc8f12dc926

Comment 8

[David McNamara [:mackers]]

Attachment: Fennec Settings xul with menulist

The menulist in fennec settings is not working for me using nightly fennec-1.0b4pre.en-US.linux-i686 from 2009-09-25.

I'm getting errors in the binding and the onchange handler is never called.

I'm attaching my settings xul file and some screenshots.

Comment 9

[David McNamara [:mackers]]

Attachment: screenshot1

Comment 10

[David McNamara [:mackers]]

Attachment: screenshot2

Comment 11

[David McNamara [:mackers]]

Attachment: screenshot3

Comment 12

[Fabrice Desré]

David, seeing the last screenshot where the radio buttons of the console manager are not styled correctly, I think that your issue comes from an add-on that includes a stylesheet it shouldn't. Can you check by disabling them all but yours ?

Comment 13

[David McNamara [:mackers]]

I've installed the latest nightly and tried installing my extension (Add-on Collector) into a clean profile. I'm still seeing the same problem.

When trying to implement a select type control in my extension's fennec settings. I see the select control, but:

- I'm still seeing the "val.setAttribute is not a function" errors 
- The onchange handler is never called.
- The preference is never set.

Bug 509081 has the context.

Comment 14

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

(In reply to comment #13)

> When trying to implement a select type control in my extension's fennec
> settings. I see the select control, but:
> 
> - I'm still seeing the "val.setAttribute is not a function" errors 
> - The onchange handler is never called.
> - The preference is never set.
> 
> Bug 509081 has the context.

David - Do you have a code snippet of what you're doing? If I try this with a simple example, it works:

<setting type="control" title="Testing">
  <menulist onchange="Components.reportError('Hello');">
    <menupopup>
      <menulist label="AAA"/>
      <menulist label="BBB"/>
    </menupopup
  </menulist>
</setting>

I see "Hello" appear in the error console.

Comment 15

[David McNamara [:mackers]]

The code is in this attachment:

https://bugzilla.mozilla.org/attachment.cgi?id=402845

I'll try with the simplier example above.

Comment 16

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

> The code is in this attachment:
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=402845

David - I see a problem in the code:

<setting type="button" title="&login.label;">

We dropped support for type="button", it's type="control" now. Not sure if this has anything to do with the problem though.

Comment 17

[David McNamara [:mackers]]

I changed the type from button to control, but the problem remains.

In fact, the simple example you gave above gives me the same problems.

I'm testing on the latest desktop linux nightly on a clean profile.