Last Comment Bug 328148 - switch/case for xul
: switch/case for xul
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: alexander :surkov
: Stephen Pride
Mentors:
Depends on:
Blocks: 327236
  Show dependency treegraph
 
Reported: 2006-02-21 21:24 PST by alexander :surkov
Modified: 2016-07-15 14:46 PDT (History)
1 user (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
wip patch (7.47 KB, patch)
2006-04-11 09:18 PDT, Allan Beaufour
no flags Details | Diff | Splinter Review
Testcase (1000 bytes, application/vnd.mozilla.xul+xml)
2006-04-11 09:21 PDT, Allan Beaufour
no flags Details
patch (6.33 KB, patch)
2006-04-23 21:49 PDT, alexander :surkov
allan: review+
Details | Diff | Splinter Review
patch [style fix] (6.39 KB, patch)
2006-04-24 03:38 PDT, alexander :surkov
aaronr: review+
Details | Diff | Splinter Review
checkin patch (6.08 KB, patch)
2006-04-25 17:20 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
checkin patch2 (6.33 KB, patch)
2006-04-26 20:05 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch3 (6.35 KB, patch)
2006-04-27 01:53 PDT, alexander :surkov
no flags Details | Diff | Splinter Review

Description alexander :surkov 2006-02-21 21:24:57 PST
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
Comment 1 Ben Fowler 2006-03-05 21:54:27 PST
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 ...
Comment 2 alexander :surkov 2006-03-05 22:03:24 PST
(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.
Comment 3 Allan Beaufour 2006-03-06 04:19:53 PST
(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 :-)
Comment 4 Allan Beaufour 2006-04-10 03:48:44 PDT
(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>?
Comment 5 alexander :surkov 2006-04-10 20:14:29 PDT
(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.
Comment 6 Allan Beaufour 2006-04-11 09:18:40 PDT
Created attachment 218038 [details] [diff] [review]
wip patch

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 :(
Comment 7 Allan Beaufour 2006-04-11 09:21:21 PDT
Created attachment 218039 [details]
Testcase
Comment 8 alexander :surkov 2006-04-11 23:27:14 PDT
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.
Comment 9 alexander :surkov 2006-04-11 23:28:31 PDT
(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 :)
Comment 10 Allan Beaufour 2006-04-12 01:40:30 PDT
(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 ... :)
Comment 11 Allan Beaufour 2006-04-12 01:44:24 PDT
(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.

?
Comment 12 alexander :surkov 2006-04-12 03:03:47 PDT
(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.
Comment 13 alexander :surkov 2006-04-23 21:49:41 PDT
Created attachment 219570 [details] [diff] [review]
patch
Comment 14 Allan Beaufour 2006-04-24 02:08:52 PDT
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
Comment 15 alexander :surkov 2006-04-24 03:38:37 PDT
Created attachment 219587 [details] [diff] [review]
patch [style fix]
Comment 16 aaronr 2006-04-25 10:49:55 PDT
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.
Comment 17 alexander :surkov 2006-04-25 17:20:59 PDT
Created attachment 219819 [details] [diff] [review]
checkin patch

(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.
Comment 18 alexander :surkov 2006-04-26 20:05:49 PDT
Created attachment 219970 [details] [diff] [review]
checkin patch2

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.
Comment 19 alexander :surkov 2006-04-27 01:53:36 PDT
Created attachment 219991 [details] [diff] [review]
patch3
Comment 20 Allan Beaufour 2006-04-27 02:05:22 PDT
Fixed on trunk

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