472426 - listbox is rendered outside fennec window

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[timeless]

Attachment: picture

0. use noscript
1. load slashdot article
2. click the white on white link for classic view
3. click the select drop down

actual results:
see picture

(note that the picture has a picture of the previous incarnation in the background, there's only one yellow listbox at a time, unless there's a picture of the same in the background...)

Comment 3

[Hiroyuki Ikezoe (:hiro)]

This issue can be seen on Linux too.

Comment 4

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

Attachment: WIP 1: the basics

This patch gets basic combobox/dropdown widgets working in Fennec:
* Adds an XBL to override the <select> tag for combobox mode (size=1)
* Signals a chrome UI (XUL + JS) helper to display the combobox list
* Widget supports UP/DOWN keyboard nav
* Chrome supports clicking on an item to choose it or ESC to cancel

Need to add:
* Keyboard support in chrome
* Cancel button in chrome - to dismiss the list
* Auto sizing the XBL widget based on the widest <option>
* Full web-compat of the new XBL widget with a standard <select> element

Comment 5

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

Attachment: WIP 2: Close to finished

This patch:
* Supports <optgroup>
* Adds some DOM properties/methods for web compat
* Styles the chrome UI a little better

Still needs:
* Setting the width of the <select> to the widest <option>
* Weird bug with highlighting the preselected option when viewing in the chrome UI
* Web compat testing!

Comment 8

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

Attachment: patch

This patch has a functional chrome-based <select> handler. It works with <optgroup>, multiple select, listboxes and dropdowns.

The chrome UI displays the list of options. Click an option to select it. Multiple selection allows clicking on a selected option to unselect it.

Use the "Done" button to close the chrome UI.

Comment 9

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

Comment on attachment 383490 [details] [diff] [review]
patch

I hope you can remove the unnecessarily duplicated nsIDOMHTMLSelectElement methods as we discussed on IRC. That would make some of these comments irrelevant. I'd prefer if the binding really only wrapped the content in order to install an event listener for click/activation, and then passed the <select> object to the display code to manipulate directly. I think it would be workable, but it's a non-trivial amount of patch reworking, so I guess it depends on how insistent we are on getting this in tomorrow morning.

There are a couple of dump()s that should be removed before landing.

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

>+  <binding id="chrome-select" extends="xul:menu">

>+      <constructor>
>+        <![CDATA[
>+          // XXX I wish this worked

remove it, or elaborate in the comment and/or file a followup and cite bug #?

>+      <property name="selectedIndex">

Won't this getter mess up key navigation as currently implemented in the multiple selection case (will just return first selected element)?

Seems like the users of this getter be checking for .multiple to make sure its value makes sense, at the very least?

>+            let options = this.options;
>+            for (let i = 0; i < options.length; i++) {
>+              if (options[i].hasAttribute("selected") && i != val)
>+                options[i].removeAttribute("selected");

Probably actually faster to not check hasAttribute first - removeAttribute already short-circuits appropriately.

>+            if (val != -1)
>+              this._label.value = options[val].textContent;

Should probably bounds check val?

>+      <property name="multiple">

>+            return (this.hasAttribute("multiple") && this.getAttribute("multiple") != "false");

Attributes like these depend only on presence rather than value, so I think you should remove the value check and just return hasAttribute().

Compare:
data:text/html,<select multiple="false"><option selected="false">test</option><option>foo</option></select>

>+      <method name="add">

>+            this.insertElement(element, before);

This method doesn't exist?

>+      <method name="item">

>+            return this.options[index];

|| null, right? item() shouldn't return undefined.

>+      <method name="_fireChange">

>+            if (this.multiple) {
>+              let label = this._displayFormat.replace(/#1/, this.selectedOptions.length);
>+              this._label.value = label;
>+            }

This makes the multi-selects looks kind of odd on the page (potentially breaking page styling more than we already are). Perhaps we should only specify <content> for single-select elements?

>+            let changeEvent = document.createEvent("Events");
>+            changeEvent.initEvent("change", true, false);
>+            this.dispatchEvent(changeEvent);

Does this match what happens in the underlying implementation? Can we achieve the same effect by just not overridding the selectedIndex setter and calling it (or selecting the relevant children)?

>+    <handlers>
>+      <handler event="keypress" keycode="VK_UP">

>+      <handler event="keypress" keycode="VK_DOWN">

I couldn't get these to work. Up/down seemed triggered spatial navigation in the background browser instead.

>+      <handler event="click" button="0">

>+          var showEvent = document.createEvent("Events");
>+          showEvent.initEvent("DOMSelectShowing", true, false);
>+          this.dispatchEvent(showEvent);

Given bug 286014, I think we should pick a name that doesn't contain DOM.

>+  <binding id="chrome-select-option">

>+      <property name="selected">

>+            return (this.hasAttribute("selected") && this.getAttribute("selected") != "false");

Similarly, I think this should just be hasAttribute.

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

>+var SelectHelper = {

>+  show: function(aControl) {
>+    if (!aControl)
>+      return;

when would this be hit?

>+    for (let i=0; i<children.length; i++) {
>+      let child = children[i];
>+      if (child.localName.toLowerCase() == "optgroup") {

instanceof HTMLOptGroupElement ?

>+        let group = document.createElement("option");
>+        group.setAttribute("label", child.label)
>+        this._list.appendChild(group);
>+        group.className = "optgroup";
>+
>+        for (let ii=0; ii<child.children.length; ii++) {

local variable for child.children.length and child.children[ii]?

>+          let item = document.createElement("option");
>+          item.setAttribute("label", child.children[ii].value)

I take it this should be "text" and not "value" to fix that bug we talked about (same below)?

>+      else {

Could use a check for instanceof HTMLOptionElement, perhaps?

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

>+    <vbox id="select-container" class="panel-dark" hidden="true"  style="-moz-stack-sizing: ignore;" top="0" left="0">

-moz-stack-sizing: ignore no longer needed (bug 476521)

>diff --git a/chrome/content/content.css b/chrome/content/content.css

>-@namespace html url(http://www.w3.org/1999/xhtml);
>+/* Style the scrollbars */
>+scrollbar {
>+  -moz-appearance: none !important;
>+  /*background-color: transparent !important;*/

get rid of this comment while you're at it?

>+scrollbarbutton {
>+-moz-appearance: none !important;

fix indent?

It's a bit odd for a bunch of presentational styles (border style/color, margin, background/foreground colors) to be put in the content stylesheet, but I guess we do want a consistent and restricted look here.

Comment 10

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

(In reply to comment #9)
> (From update of attachment 383490 [details] [diff] [review])
> I hope you can remove the unnecessarily duplicated nsIDOMHTMLSelectElement
> methods as we discussed on IRC. That would make some of these comments
> irrelevant. I'd prefer if the binding really only wrapped the content in order
> to install an event listener for click/activation, and then passed the <select>
> object to the display code to manipulate directly. I think it would be
> workable, but it's a non-trivial amount of patch reworking, so I guess it
> depends on how insistent we are on getting this in tomorrow morning.

I have removed all duplicate methods.

> There are a couple of dump()s that should be removed before landing.

Removed

> >diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml
> 
> >+  <binding id="chrome-select" extends="xul:menu">
> 
> >+      <constructor>
> >+        <![CDATA[
> >+          // XXX I wish this worked
> 
> remove it, or elaborate in the comment and/or file a followup and cite bug #?

Filed bug 500206

> >+      <method name="_fireChange">
> 
> >+            if (this.multiple) {
> >+              let label = this._displayFormat.replace(/#1/, this.selectedOptions.length);
> >+              this._label.value = label;
> >+            }
> 
> This makes the multi-selects looks kind of odd on the page (potentially
> breaking page styling more than we already are). Perhaps we should only specify
> <content> for single-select elements?

Leaving for now. Need Madhava's input.

> >+            let changeEvent = document.createEvent("Events");
> >+            changeEvent.initEvent("change", true, false);
> >+            this.dispatchEvent(changeEvent);
> 
> Does this match what happens in the underlying implementation? Can we achieve
> the same effect by just not overridding the selectedIndex setter and calling it
> (or selecting the relevant children)?

We still need to fire this event. Setting selectedIndex doesn't seem to fire it. I tested against FF and it seems to match. More testing needed though.

> >+    <handlers>
> >+      <handler event="keypress" keycode="VK_UP">
> 
> >+      <handler event="keypress" keycode="VK_DOWN">
> 
> I couldn't get these to work. Up/down seemed triggered spatial navigation in
> the background browser instead.

Yeah, but we might be turning spatial nav off by default.

> >+      <handler event="click" button="0">
> 
> >+          var showEvent = document.createEvent("Events");
> >+          showEvent.initEvent("DOMSelectShowing", true, false);
> >+          this.dispatchEvent(showEvent);
> 
> Given bug 286014, I think we should pick a name that doesn't contain DOM.

Changed to "UIShowSelect"

> 
> >+  <binding id="chrome-select-option">
> 
> >+      <property name="selected">
> 
> >+            return (this.hasAttribute("selected") && this.getAttribute("selected") != "false");
> 
> Similarly, I think this should just be hasAttribute.

Changed

> >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
> 
> >+var SelectHelper = {
> 
> >+  show: function(aControl) {
> >+    if (!aControl)
> >+      return;
> 
> when would this be hit?

Nowhere yet. It's just a defensive check.

> >+    for (let i=0; i<children.length; i++) {
> >+      let child = children[i];
> >+      if (child.localName.toLowerCase() == "optgroup") {
> 
> instanceof HTMLOptGroupElement ?

Done.

> >+        let group = document.createElement("option");
> >+        group.setAttribute("label", child.label)
> >+        this._list.appendChild(group);
> >+        group.className = "optgroup";
> >+
> >+        for (let ii=0; ii<child.children.length; ii++) {
> 
> local variable for child.children.length and child.children[ii]?

Done.

> >+          let item = document.createElement("option");
> >+          item.setAttribute("label", child.children[ii].value)
> 
> I take it this should be "text" and not "value" to fix that bug we talked about
> (same below)?

Yes, I used .textContent

> >+      else {
> 
> Could use a check for instanceof HTMLOptionElement, perhaps?

Done

> >diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul
> 
> >+    <vbox id="select-container" class="panel-dark" hidden="true"  style="-moz-stack-sizing: ignore;" top="0" left="0">
> 
> -moz-stack-sizing: ignore no longer needed (bug 476521)

Removed.

> >-@namespace html url(http://www.w3.org/1999/xhtml);
> >+/* Style the scrollbars */
> >+scrollbar {
> >+  -moz-appearance: none !important;
> >+  /*background-color: transparent !important;*/
> 
> get rid of this comment while you're at it?

Done.

> >+scrollbarbutton {
> >+-moz-appearance: none !important;
> 
> fix indent?

Done.

Comment 11

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

Attachment: patch 2 - addresses comments

This patch addresses the review comments

Comment 12

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

The patch has a bug. If script changes the selection, the control label is not updated. Filed bug 500208

Comment 13

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

Comment on attachment 384886 [details] [diff] [review]
patch 2 - addresses comments


(In reply to comment #10)
> > I take it this should be "text" and not "value" to fix that bug we talked about
> > (same below)?
> 
> Yes, I used .textContent

Think we actually want .text, which looks like the equivalent but with compressed whitespace:
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLOptionElement.cpp#389

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

>+  <binding id="chrome-select" extends="xul:menu">

>+        <xul:label anonid="select-label" flex="1" value=" " xbl:inherits="value=label"/>

Is the value=" " needed for layout purposes? Does "" work just as well?

>+      <field name="_displayFormat" readonly="true">"&selectListDisplay.format;"</field>

nit: fields don't have a readonly property.

>+      <method name="_updateLabel">

>+            let label = "";

>+              if (index != -1)
>+                label = this.options[index].textContent;
>+              else
>+                label = " ";

Could adjust the default value and get rid of this else.

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

>+var SelectHelper = {

>+  show: function(aControl) {

>+          item.option = optionIndex++;

maybe name the property optionIndex too? A bit confusing as just "option", make it look like a reference to an option element.

Comment 14

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

(In reply to comment #13)
> (From update of attachment 384886 [details] [diff] [review])
> 
> (In reply to comment #10)
> > > I take it this should be "text" and not "value" to fix that bug we talked about
> > > (same below)?
> > 
> > Yes, I used .textContent
> 
> Think we actually want .text, which looks like the equivalent but with
> compressed whitespace:

Right, changed to .text

> >+        <xul:label anonid="select-label" flex="1" value=" " xbl:inherits="value=label"/>
> 
> Is the value=" " needed for layout purposes? Does "" work just as well?

Changed to ""

> >+      <field name="_displayFormat" readonly="true">"&selectListDisplay.format;"</field>
> 
> nit: fields don't have a readonly property.

Removed

> >+              if (index != -1)
> >+                label = this.options[index].textContent;
> >+              else
> >+                label = " ";
> 
> Could adjust the default value and get rid of this else.

Yeah. Removed else

> >+          item.option = optionIndex++;
> 
> maybe name the property optionIndex too? A bit confusing as just "option", make
> it look like a reference to an option element.

Agreed. Done.

Comment 15

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

pushed: http://hg.mozilla.org/mobile-browser/rev/70e9519cada3

Comment 17

[Francesco Lodolo [:flod]]

Need a localization comment in browser.dtd
<!ENTITY selectListDisplay.format  "#1 Items">

What is #1? A number?

Comment 18

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

Francesco - Yes, the "#1" would be replcaced with the number of selected items. Such as "5 Items", which now becomes apparent that plural forms would need to be supported.

But! I'm fairly sure the string will be removed. We are changing the display of <select> elements in content to look more normal instead of making all <select> elements look like drop downs

See bug 500208

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

verified with nightly build of maemo 20090818