Closed Bug 328148 Opened 18 years ago Closed 18 years ago

switch/case for xul

Categories

(Core Graveyard :: XForms, enhancement)

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: fixed1.8.0.5, fixed1.8.1)

Attachments

(2 files, 5 obsolete files)

Testcase
1000 bytes, application/vnd.mozilla.xul+xml
Details
patch3
6.35 KB, patch
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

case for xhtml and xul

Reproducible: Always
Blocks: 327236
Assignee: aaronr → surkov
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
You may want to give more details. Are you saying that firefox uses the
wrong case in places within case-sensitive mark-up?

Can you say what you observed and what you expected ...
(In reply to comment #1)
> You may want to give more details. Are you saying that firefox uses the
> wrong case in places within case-sensitive mark-up?
> 

I guess no.

> Can you say what you observed and what you expected ...
> 

Just I guess it's more right to have separate widgets of xforms:case for xul context. Though now we have xforms:case for xhmtl and it works for xul too.
(In reply to comment #1)
> You may want to give more details. Are you saying that firefox uses the
> wrong case in places within case-sensitive mark-up?

Component: XForms -- just ignore us, we know what we are doing :-)
(In reply to comment #2)
> Just I guess it's more right to have separate widgets of xforms:case for xul
> context. Though now we have xforms:case for xhmtl and it works for xul too.

Agrred. Should be fairly easy to do with a XUL <deck> + <box>?
(In reply to comment #4)
> (In reply to comment #2)
> > Just I guess it's more right to have separate widgets of xforms:case for xul
> > context. Though now we have xforms:case for xhmtl and it works for xul too.
> 
> Agrred. Should be fairly easy to do with a XUL <deck> + <box>?
> 

Yes, it should be if we'll add binding for xf:switch.
Attached patch wip patch (obsolete) — — Splinter Review 
Here's a work-in-progress patch, using deck. It also has xforms:group bound to xul:groupbox, as it seems natural.

It does not work though. The deck shows everything :(
Attached file Testcase — 

    
    

    
  
Comment on attachment 218038 [details] [diff] [review]
wip patch


>+    <implementation>
>+      <method name="getControlElement">
>+        <body>
>+          return {
>+            __proto__: this.ownerDocument.
>+              getAnonymousElementByAttribute(this, 'anonid', 'container'),
>+
>+            set enabled(val) {
>+              this.style.display = val ? "inherit" : "none";

Should be '=='

>+  <!-- SWITCH -->
>+  <binding id="xformswidget-switch"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
>+    <content>
>+      <xul:deck flex="1">
>+        <children includes="case"/>

xforms:case replace anonymous content. Is there a way to specify namespace in attribute @includes? Temporary solution is @inlcudes attribute removing.

>+
>+            set enabled(val) {
>+              dump("\n\nenabled(" + val + ")\n");
>+              this.parentNode.parentNode.selectedPanel = this;

It seems to me 'this.parentNode.parentNode' returns xforms:switch element, should be xul:deck. And 'this' returns xul:box, should be xf:case.

I don't like parentNode.parentNode using because I think it's not nice :))

> 
>-      <field name="_container">null</field>
>-      <property name="container" readonly="true">
>+      <!--
>+        Return object to operate with underliying controls. Each xforms widget
>+        defines interface what should implement the object.
>+      -->
>+      <property name="control" readonly="true">
>         <getter>
>-          if (!this._container) {
>-            this._container = document.
>-              getAnonymousElementByAttribute(this, "anonid", "container");
>-          }
>-          return this._container;
>+          if (!this._control)
>+            this._control = this.getControlElement();
>+          return this._control;
>         </getter>
>       </property>

If we will fix the bug 327236 at first then we will not be needed in 'control' property.

The rest looks good.

    
    

    
  
(In reply to comment #8)
> (From update of attachment 218038 [details] [diff] [review] [edit])
> 
> >+              this.style.display = val ? "inherit" : "none";
> 
> Should be '=='

No, sorry, it shouldn't be :)

    
    

    
  
(In reply to comment #6)
> It also has xforms:group bound to xul:groupbox, as it seems natural.

Changed my mind about this one... group should not have a visual reprensentation. It seems natural in my use case only ... :)

    
    

    
  
(In reply to comment #8)
> >+  <!-- SWITCH -->
> >+  <binding id="xformswidget-switch"
> >+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
> >+    <content>
> >+      <xul:deck flex="1">
> >+        <children includes="case"/>
> 
> xforms:case replace anonymous content. Is there a way to specify namespace in
> attribute @includes? Temporary solution is @inlcudes attribute removing.

Actually, it was part of my debugging. We can just skip the @includes.
 
> >+
> >+            set enabled(val) {
> >+              dump("\n\nenabled(" + val + ")\n");
> >+              this.parentNode.parentNode.selectedPanel = this;
> 
> It seems to me 'this.parentNode.parentNode' returns xforms:switch element,
> should be xul:deck. And 'this' returns xul:box, should be xf:case.

I think I'm hitting the right node. But the deck is showing everything all the time, so there's something more general that is not behaving well...

> I don't like parentNode.parentNode using because I think it's not nice :))

I agree, but it works and I did not have a better solution right at hand.

> If we will fix the bug 327236 at first then we will not be needed in 'control'
> property.

?

    
    

    
  
(In reply to comment #11)

> > If we will fix the bug 327236 at first then we will not be needed in 'control'
> > property.
> 
> ?
> 
Sorry, I have in view the bug 328149.


    
  
Severity: normal → enhancement

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #218038 -
        Attachment is obsolete: true

        Attachment #219570 -
        Flags: review?(allan)

    
    

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

mozilla/extensions/xforms/resources/content/xforms.css
>--- mozilla.orig/extensions/xforms/resources/content/xforms.css	2006-04-18 >@@ -322,15 >+/* switch/case widgets */
>+html|*:root case {
>+  -moz-binding: url('chrome://xforms/content/xforms-xhtml.xml#xformswidget-case');
> }
>-
>-case > html|div.-moz-xforms-case-container {
>+html|*:root case > html|div.-moz-xforms-case-container {
>   display: inherit;
> }
> 
>+xul|*:root switch {
>+  -moz-binding: url('chrome://xforms/content/xforms-xul.xml#xformswidget-switch');
>+}
>+xul|*:root case {
>+  -moz-binding: url('chrome://xforms/content/xforms-xul.xml#xformswidget-case');
>+}
>+

I think we should do the same styling as for XHTML cases, so:
xul|*:root case > box{
  -moz-box-orient: inherit;
}

Then the form author should be able to do:
xf|case {
  -moz-box-orient: vertical;
}
Right?

mozilla/extensions/xforms/resources/content/xforms-xul.xml
>--- mozilla.orig/extensions/xforms/resources/content/xforms-xul.xml	2006-04-04 >+  <!-- CASE -->
>+  <binding id="xformswidget-case"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-case-base">
>+    <content>
>+      <xul:box anonid="control" xbl:inherits="orient" flex="1">

Do not inherit the orient, if we can use CSS instead.

>+        <children/>
>+      </xul:box>
>+    </content>

With the styling fixed, r=me

        Attachment #219570 -
        Flags: review?(allan) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch [style fix] (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #219570 -
        Attachment is obsolete: true

        Attachment #219587 -
        Flags: review?(aaronr)

    
    

    
  
Comment on attachment 219587 [details] [diff] [review]
patch [style fix]


>+  <!-- CASE -->
>+  <binding id="xformswidget-case"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-case-base">
>+    <content>
>+      <xul:box anonid="control" flex="1">
>+        <children/>
>+      </xul:box>
>+    </content>
>+
>+    <implementation>
>+      <method name="getControlElement">
>+        <body>
>+        <![CDATA[
>+          var switchElm = this.parentNode;
>+          while (switchElm && switchElm.namespaceURI != this.XFORMS_NS &&
>+                 switchElm.nodeName != "switch") {
>+            switchElm = switchElm.parentNode;
>+          }
>+
>+          return {
>+            __proto__: this.ownerDocument.
>+              getAnonymousElementByAttribute(this, 'anonid', 'control'),
>+            caseElm: this,
>+            switchElm: switchElm,
>+
>+            set selected(val) {
>+              this.switchElm.selectedCase = this.caseElm;
>+            }
>+          };
>+        ]]>
>+        </body>
>+      </method>
>+    </implementation>
>+  </binding>
>+
> </bindings>


nit: I guess I don't see why you have caseElm if it only returns 'this'.  Can't you just say, "this.switchElm.selectedCase = this;"

r=me either way.

        Attachment #219587 -
        Flags: review?(aaronr) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        checkin patch (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to comment #16)

> nit: I guess I don't see why you have caseElm if it only returns 'this'.  Can't
> you just say, "this.switchElm.selectedCase = this;"
> 
> r=me either way.
> 

No because

> >+          return {
> >+            __proto__: this.ownerDocument.
> >+              getAnonymousElementByAttribute(this, 'anonid', 'control'),

                // 'this' means the binding
> >+            caseElm: this,
> >+            switchElm: switchElm,
> >+
> >+            set selected(val) {

                  // 'this' means the object
> >+              this.switchElm.selectedCase = this.caseElm;
> >+            }
> >+          };

Though I can see that object shouldn't be inherited from 'control' element. Also I removed a debug alerts from switch element.

        Attachment #219587 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        checkin patch2 (obsolete)
        — 
          — Splinter Review
      

    


  
Oops, we should have switch binding for xhtml too since switch can have single node binding attribute. If you don't have any comments then please check in the patch.

        Attachment #219819 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch3
        — 
          — Splinter Review
      

    


  

        Attachment #219970 -
        Attachment is obsolete: true

    
    

    
  
Fixed on trunk
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Summary: case for xhtml and xul → switch/case for xul
Whiteboard: xf-to-branch

    
  
Keywords: fixed1.8.1

    
  
Keywords: fixed1.8.0.5

    
  
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: