listbox is rendered outside fennec window

VERIFIED FIXED

Status

Firefox for Android Graveyard
General
P1
normal
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: timeless, Assigned: mfinkle)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 355705 [details]
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...)
Duplicate of this bug: 485888

Updated

9 years ago
tracking-fennec: --- → 1.0b2+

Updated

9 years ago
Assignee: nobody → gavin.sharp

Updated

9 years ago
Duplicate of this bug: 461125

Updated

9 years ago
Priority: -- → P1
Assignee: gavin.sharp → nobody
(Assignee)

Updated

9 years ago
Assignee: nobody → mark.finkle
This issue can be seen on Linux too.
OS: Windows XP → All
Hardware: x86 → All
Created attachment 382449 [details] [diff] [review]
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
Created attachment 382923 [details] [diff] [review]
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!
Attachment #382449 - Attachment is obsolete: true
Duplicate of this bug: 460970
(Assignee)

Updated

9 years ago
Duplicate of this bug: 493846
Created attachment 383490 [details] [diff] [review]
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.
Attachment #382923 - Attachment is obsolete: true
Attachment #383490 - Flags: review?(gavin.sharp)
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.
(Assignee)

Updated

9 years ago
Depends on: 500206
(Assignee)

Updated

9 years ago
Depends on: 500208
(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.
Created attachment 384886 [details] [diff] [review]
patch 2 - addresses comments

This patch addresses the review comments
Attachment #383490 - Attachment is obsolete: true
Attachment #384886 - Flags: review?(gavin.sharp)
Attachment #383490 - Flags: review?(gavin.sharp)
The patch has a bug. If script changes the selection, the control label is not updated. Filed bug 500208
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.
Attachment #384886 - Flags: review?(gavin.sharp) → review+
(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.
pushed: http://hg.mozilla.org/mobile-browser/rev/70e9519cada3
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Duplicate of this bug: 493845
Need a localization comment in browser.dtd
<!ENTITY selectListDisplay.format  "#1 Items">

What is #1? A number?
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
verified with nightly build of maemo 20090818
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.