Status

Core Graveyard
XForms
--
enhancement
RESOLVED FIXED
11 years ago
10 months ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({fixed1.8.0.5, fixed1.8.1})

Trunk
fixed1.8.0.5, fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

11 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

case for xhtml and xul

Reproducible: Always
(Assignee)

Updated

11 years ago
Blocks: 327236

Updated

11 years ago
Assignee: aaronr → surkov

Updated

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

Comment 1

11 years ago
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 ...
(Assignee)

Comment 2

11 years ago
(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

11 years ago
(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

11 years ago
(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>?
(Assignee)

Comment 5

11 years ago
(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

11 years ago
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

11 years ago
Created attachment 218039 [details]
Testcase
(Assignee)

Comment 8

11 years ago
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.
(Assignee)

Comment 9

11 years ago
(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

11 years ago
(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

11 years ago
(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.

?
(Assignee)

Comment 12

11 years ago
(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.

Updated

11 years ago
Severity: normal → enhancement
(Assignee)

Comment 13

11 years ago
Created attachment 219570 [details] [diff] [review]
patch
Attachment #218038 - Attachment is obsolete: true
Attachment #219570 - Flags: review?(allan)

Comment 14

11 years ago
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+
(Assignee)

Comment 15

11 years ago
Created attachment 219587 [details] [diff] [review]
patch [style fix]
Attachment #219570 - Attachment is obsolete: true
Attachment #219587 - Flags: review?(aaronr)

Comment 16

11 years ago
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+
(Assignee)

Comment 17

11 years ago
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.
Attachment #219587 - Attachment is obsolete: true
(Assignee)

Comment 18

11 years ago
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.
Attachment #219819 - Attachment is obsolete: true
(Assignee)

Comment 19

11 years ago
Created attachment 219991 [details] [diff] [review]
patch3
Attachment #219970 - Attachment is obsolete: true

Comment 20

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

Updated

11 years ago
Keywords: fixed1.8.1

Updated

11 years ago
Keywords: fixed1.8.0.5

Updated

11 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.