Closed
Bug 323851
Opened 19 years ago
Closed 19 years ago
xforms select widgets for xul
Categories
(Core Graveyard :: XForms, defect)
Core Graveyard
XForms
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: surkov, Assigned: surkov)
References
Details
(Keywords: fixed1.8.0.4, fixed1.8.1)
Attachments
(2 files, 2 obsolete files)
|
(deleted),
application/vnd.mozilla.xul+xml
|
Details | |
|
(deleted),
patch
|
doronr
:
review+
|
Details | Diff | Splinter Review |
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
|
||
Updated•19 years ago
|
Assignee: aaronr → surkov
|
|
||
Updated•19 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
|
Assignee | |
Comment 1•19 years ago
|
||
Attachment #216609 -
Flags: review?(aaronr)
|
|
Assignee | |
Updated•19 years ago
|
Attachment #216609 -
Flags: review?(doronr)
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•19 years ago
|
||
(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•19 years ago
|
Attachment #216702 -
Flags: review?(doronr)
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•19 years ago
|
||
|
|
Assignee | |
Comment 6•19 years ago
|
||
(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•19 years ago
|
||
Comment on attachment 216893 [details] [diff] [review]
patch2 [aaron's]
looks good
Attachment #216893 -
Flags: review?(doronr) → review+
|
|
||
Comment 8•19 years ago
|
||
Fixed on trunk
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
|
|
||
Updated•19 years ago
|
Keywords: fixed1.8.0.3,
fixed1.8.1
|
|
||
Updated•19 years ago
|
Whiteboard: xf-to-branch
|
||
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•