xforms select widgets for xul

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
13 years ago
2 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({fixed1.8.0.4, fixed1.8.1})

Trunk
fixed1.8.0.4, fixed1.8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

7.59 KB, application/vnd.mozilla.xul+xml
Details
38.37 KB, patch
Doron Rosenberg (IBM)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1

the patch coming up.

Reproducible: Always
(Assignee)

Updated

13 years ago
Depends on: 323849
(Assignee)

Updated

13 years ago
Blocks: 307627
(Assignee)

Updated

13 years ago
Depends on: 323845
(Assignee)

Updated

13 years ago
Blocks: 327236

Updated

13 years ago
Assignee: aaronr → surkov

Updated

13 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Updated

13 years ago
Blocks: 326556
(Assignee)

Comment 1

12 years ago
Created attachment 216609 [details] [diff] [review]
patch
Attachment #216609 - Flags: review?(aaronr)
(Assignee)

Updated

12 years ago
Attachment #216609 - Flags: review?(doronr)

Comment 2

12 years ago
Comment on attachment 216609 [details] [diff] [review]
patch


I need to preface my review by saying that I haven't done anything with XUL in a long, long time so I'll rely on Doron for making sure the XUL stuff is right.

>diff -uNrap mozilla.orig/extensions/xforms/resources/content/select-xul.xml mozilla/extensions/xforms/resources/content/select-xul.xml
>--- mozilla.orig/extensions/xforms/resources/content/select-xul.xml	1970-01-01 08:00:00.000000000 +0800
>+++ mozilla/extensions/xforms/resources/content/select-xul.xml	2006-03-29 15:26:21.000000000 +0900
>@@ -0,0 +1,681 @@
>+<?xml version="1.0"?>
>+
>+<!-- ***** BEGIN LICENSE BLOCK *****
>+   - Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+   -
>+   - The contents of this file are subject to the Mozilla Public License Version
>+   - 1.1 (the "License"); you may not use this file except in compliance with
>+   - the License. You may obtain a copy of the License at
>+   - http://www.mozilla.org/MPL/
>+   -
>+   - Software distributed under the License is distributed on an "AS IS" basis,
>+   - WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+   - for the specific language governing rights and limitations under the
>+   - License.
>+   -
>+   - The Original Code is Mozilla XForms support.
>+   -
>+   - The Initial Developer of the Original Code is
>+   - Alexander Surkov.
>+   - Portions created by the Initial Developer are Copyright (C) 2005
>+   - the Initial Developer. All Rights Reserved.
>+   -
>+   - Contributor(s):
>+   -  Alexander Surkov <surkov@dc.baikal.ru>
>+   -

Since this is a new file, you should make the date 2006

>+
>+      <method name="appendGroup">
>+        <parameter name="aLabel"/>
>+        <parameter name="aGroup"/>
>+        <body>
>+          // XXX: Group supporting isn't implemented
>+          return null;
>+        </body>
>+      </method>
>+

make sure bugs get opened on all your XXX items

>+      <method name="addItemToSelection">
>+        <parameter name="aItem"/>
>+        <body>
>+          // there are cases when 'listitem' binding isn't created yet,
>+          // threafore we use setTimeout
>+
>+          window.setTimeout(
>+            function(list, item) {
>+              list.setAttribute("suppressonselect", "true");
>+              list.addItemToSelection(item);
>+              list.removeAttribute("suppressonselect");
>+            },
>+            0,
>+            this.control, aItem
>+          );
>+        </body>
>+      </method>
>+
>+      <method name="removeItemFromSelection">
>+        <parameter name="aItem"/>
>+        <body>
>+          this.control.setAttribute("suppressonselect", "true");
>+          this.control.removeItemFromSelection(aItem);
>+          this.control.removeAttribute("suppressonselect");
>+        </body>
>+      </method>
>+
>+      <method name="isItemSelected">
>+        <parameter name="aItem"/>
>+        <body>
>+          if (aItem.selected)
>+            return true;
>+          return false;
>+        </body>
>+      </method>
>+
>+      <constructor>
>+        // 'select' event is not always handled when handler is added by using
>+        // xbl:handel element.

nit: should be xbl:handler element, right?

>+
>+  <!-- CONTROL WIDGET FOR SELECT APPEARANCE='FULL' -->
>+  <binding id="controlwidget-select-full"
>+           extends="chrome://xforms/content/select.xml#controlwidget-base">

>+      <method name="isItemSelected">
>+        <parameter name="aItem"/>
>+        <body>
>+          if (aItem.firstChild.checked)
>+            return true;
>+          return false;
>+        </body>
>+      </method>
>+    </implementation>

can't you just return aItem.firstChild.checked?


I've gotten down to the select1 controls.  More review comments tomorrow.
(Assignee)

Comment 3

12 years ago
Created attachment 216702 [details] [diff] [review]
patch [aaron's]

(In reply to comment #2)
> Since this is a new file, you should make the date 2006

Fixed

> make sure bugs get opened on all your XXX items

I'll do it after bug fix.
> 
> 
> nit: should be xbl:handler element, right?

Fixed

> can't you just return aItem.firstChild.checked?

Right. Fixed.

> I've gotten down to the select1 controls.  More review comments tomorrow.
> 
I'm looking forward :)
Attachment #216609 - Attachment is obsolete: true
Attachment #216702 - Flags: review?(aaronr)
Attachment #216609 - Flags: review?(doronr)
Attachment #216609 - Flags: review?(aaronr)
(Assignee)

Updated

12 years ago
Attachment #216702 - Flags: review?(doronr)

Comment 4

12 years ago
Comment on attachment 216702 [details] [diff] [review]
patch [aaron's]


>+<!-- CONTROL WIDGETS FOR SELECT1 CONTROLS
>+  The section contains underlying widgets implementations needed for xforms
>+  select1 controls. All underlying widgets implement the interface what base
>+  widget for xforms select1 controls ask for. You can find interface description
>+  in 'select.xml' file.
>+-->
>+
>+  <!-- CONTROL WIDGET FOR SELECT1 APPEARANCE='MINIMAL' -->
>+  <binding id="controlwidget-select1-minimal"
>+           extends="chrome://xforms/content/select.xml#controlwidget-base">
>+
>+    <content>
>+      <xul:menulist xbl:inherits="style, accesskey, disabled=readonly"
>+                    anonid="control" flex="1">
>+        <xul:menupopup/>
>+      </xul:menulist>
>+    </content>
>+
>+    <implementation>
>+      <property name="readonly">
>+        <getter>
>+          return this.getAttribute("readonly") == "true" ? true : false;
>+        </getter>
>+        <setter>
>+          if (val) {
>+            this.setAttribute("readonly", "true");
>+          } else {
>+            this.removeAttribute("readonly");
>+          }
>+        </setter>
>+      </property>
>+
>+      <property name="selectedIndex"
>+                onget="return this.control.selectedIndex;"
>+                onset="this.control.selectedIndex = val;"/>

can't readonly be moved into select.xml#controlwidget-base?  Doesn't it do the exact same things in all of the implementations for XUL and XHTML?  Could probably do the same with removeAllItems, too, right?  4 out of 6 implementations are the same.

>+
>+      <method name="removeAllItems">
>+        <body>
>+          var popup = this.control.menupopup;
>+          while (popup.hasChildNodes()) {
>+            popup.removeChild(popup.lastChild);
>+          }
>+        </body>
>+      </method>
>+
>+      <method name="appendItem">
>+        <parameter name="aLabel"/>
>+        <parameter name="aValue"/>
>+        <parameter name="aGroup"/>
>+        <body>
>+          var item = this.ownerDocument.createElementNS(this.XUL_NS, "menuitem");
>+          item.setAttribute("value", aValue);
>+          if (aLabel) {
>+            // XXX: We should use node instead of its text content to add a
>+            // label. But we cannot put node into menuitem, threafore we now
>+            // use label text and set @label for menulist.
>+            item.setAttribute("label", aLabel.textContent);
>+          }
>+

nit: spelling s/threafore/therefore

>+          // XXX: Group supporting isn't implemented
>+          this.control.menupopup.appendChild(item);
>+          return item;
>+        </body>
>+      </method>
>+
>+      <method name="appendGroup">
>+        <parameter name="aLabel"/>
>+        <parameter name="aGroup"/>
>+        <body>
>+          // XXX: Group supporting isn't implemented
>+          return null;
>+        </body>
>+      </method>
>+
>+      <method name="addItemToSelection">
>+        <parameter name="aItem"/>
>+        <body>
>+          this.control.selectedItem = aItem;
>+        </body>
>+      </method>
>+
>+      <method name="removeItemFromSelection">
>+        <parameter name="aItem"/>
>+        <body>
>+          this.control.selectedItem = null;
>+        </body>
>+      </method>

Should you sanity check this?  What if aItem isn't the currently selected item?  You'll be deselecting the wrong item, right?

>+
>+      <method name="isItemSelected">
>+        <parameter name="aItem"/>
>+        <body>
>+          if (aItem.getAttribute("selected") == "true")
>+            return true;
>+          return false;
>+        </body>
>+      </method>
>+    </implementation>

could just use, "return aItem.getAttribute("selected") == "true");" right?

>+
>+    <handlers>
>+      <handler event="focus" phase="capturing">
>+        this.dispatchDOMUIEvent("DOMFocusIn");
>+      </handler>
>+
>+      <handler event="blur" phase="capturing">
>+        this.updateInstanceData(false);
>+        this.dispatchDOMUIEvent("DOMFocusOut");
>+      </handler>
>+
>+      <handler event="command">
>+        this.updateInstanceData(true);
>+      </handler>
>+
>+      <!--
>+        XXX: Since xforms:label can include arbitrary elements then 'focus',
>+          'blur' and 'command' events should be listen from 'xul:menulist'
>+          element only. The described problem is not actual now. Note we will
>+          get it when we will use node instead of text content for a label.
>+      -->
>+    </handlers>
>+  </binding>
>+
>+
>+  <!-- CONTROL WIDGET FOR SELECT1 APPEARANCE='COMPACT' -->
>+  <binding id="controlwidget-select1-compact"
>+           extends="#controlwidget-select-compact">
>+    <content>
>+      <xul:listbox xbl:inherits="style, accesskey, disabled=readonly"
>+                   rows="5" anonid="control"/>
>+    </content>
>+  </binding>
>+
>+
>+  <!-- CONTROL WIDGET FOR SELECT1 APPEARANCE='FULL' -->
>+  <binding id="controlwidget-select1-full"
>+           extends="chrome://xforms/content/select.xml#controlwidget-base">

>+      <method name="isItemSelected">
>+        <parameter name="aItem"/>
>+        <body>
>+          if (aItem.firstChild.selected)
>+            return true;
>+          return false;
>+        </body>
>+      </method>
>+    </implementation>
>+

just return(aItem.firstChild.selected);


Nothing too major other than moving some functions to the base interfaces if possible.  With those addressed, r=me

It'd also be nice if you have some simple XUL select* testcases to attach them to the bug.  Be nice to have them in case sometime in the future we need to test for regressions.
Attachment #216702 - Flags: review?(aaronr) → review+
(Assignee)

Comment 5

12 years ago
Created attachment 216892 [details]
testcase
(Assignee)

Comment 6

12 years ago
Created attachment 216893 [details] [diff] [review]
patch2 [aaron's]

(In reply to comment #4)

> can't readonly be moved into select.xml#controlwidget-base?  Doesn't it do the
> exact same things in all of the implementations for XUL and XHTML?

I'm agree. Fixed.

>  Could
> probably do the same with removeAllItems, too, right?  4 out of 6
> implementations are the same.
>

I don't agree. controlwidget-base shouldn't do supposes about internal structure of inherited widgets. Especially removeAllItems() realization is different for widgets.

> nit: spelling s/threafore/therefore

Fixed.

> Should you sanity check this?  What if aItem isn't the currently selected item?
>  You'll be deselecting the wrong item, right?

Right. Fixed.

> could just use, "return aItem.getAttribute("selected") == "true");" right?

Fixed.

Thanks for thorough review.
Attachment #216702 - Attachment is obsolete: true
Attachment #216893 - Flags: review?(doronr)
Attachment #216702 - Flags: review?(doronr)

Comment 7

12 years ago
Comment on attachment 216893 [details] [diff] [review]
patch2 [aaron's]

looks good
Attachment #216893 - Flags: review?(doronr) → review+

Comment 8

12 years ago
Fixed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Updated

12 years ago
Blocks: 332853

Updated

12 years ago
Keywords: fixed1.8.0.3, fixed1.8.1

Updated

12 years ago
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.