Closed Bug 323851 Opened 19 years ago Closed 18 years ago

xforms select widgets for xul

Categories

(Core Graveyard :: XForms, defect)

Product:
Core Graveyard
Component:
XForms
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: fixed1.8.0.4, fixed1.8.1)

Attachments

(2 files, 2 obsolete files)

testcase
7.59 KB, application/vnd.mozilla.xul+xml
Details
patch2 [aaron's]
38.37 KB, 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
Depends on: 323849
Blocks: 307627
Depends on: 323845
Blocks: 327236
Assignee: aaronr → surkov
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Blocks: 326556
Attached patch patch (obsolete) — — Splinter Review 

        Attachment #216609 -
        Flags: review?(aaronr)

    
  

        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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch [aaron's] (obsolete)
        — 
          — Splinter Review
      

    


  
(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)

    
  

        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+

    
    

    
  

      
      
      
      

        Attached file
          
        testcase
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch2 [aaron's]
        — 
          — Splinter Review
      

    


  
(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 on attachment 216893 [details] [diff] [review]
patch2 [aaron's]

looks good

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

    
    

    
  
Fixed on trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

    
  
Blocks: 332853

    
  
Keywords: fixed1.8.0.3, 
          
            fixed1.8.1

    
  
Whiteboard: xf-to-branch
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: