307627 - [Meta] Native xforms XUL widgets

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050821 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050821 Firefox/1.6a1

There is xforms in xul example http://lab.cph.novell.com/~abeaufour/xfcalc.zip .
This example redefines some xforms controls: redefined controls use xul elements
instead of html elements. When I try use native xforms controls (with html
elements) then such elements is not work in xul. I think the bug is dupe and 
actually isn't related to xforms.

Allan, if you wrotes this example then please comment the bug. Why native xforms
controls doesn't work with xul?

Reproducible: Always

Comment 1

[alexander :surkov (:asurkov)]

Attachment: testcase

native xforms controls in xul document

Comment 2

[alexander :surkov (:asurkov)]

For thease elements box model is broken. I watched testcase in DOM Inspector.
All properties (coordinates x/y, width/height) of box model has value undefined.

Comment 3

[alexander :surkov (:asurkov)]

It's clear. Because xforms element hasn't namespace xul or html then mozilla
doesn't process it even it has binding with content. Actually problem is lying
in xbl.lux

Comment 4

[alexander :surkov (:asurkov)]

To comment #3
I filed bug 307646 .

Comment 5

[alexander :surkov (:asurkov)]

If the bug 307646 will be fixed then the bug will not be solved. You should
change styles for bindings of xforms controls for xul documents. In instance,
styles for xforms input controls can be defined as:

xf|input{
  -moz-appearance: textfield;
  cursor: text;
  margin: 2px 4px;
  border: 2px solid;
  -moz-border-top-colors: ThreeDShadow ThreeDDarkShadow;
  -moz-border-right-colors: ThreeDHighlight ThreeDLightShadow;
  -moz-border-bottom-colors: ThreeDHighlight ThreeDLightShadow;
  -moz-border-left-colors: ThreeDShadow ThreeDDarkShadow;
  padding: 1px 0px 1px 2px;
  background-color: -moz-Field;
  color: -moz-FieldText;
  display: -moz-box; /*nota bene*/
}
    
xf|input html|input{
  margin: 0px !important;
  border: none !important;
  padding: 0px !important;
  background-color: inherit;
  color: inherit;
  font: inherit;
  -moz-box-flex: 1; /*nota bene*/

 }

Comment 6

[alexander :surkov (:asurkov)]

If there is ability to use xforms in xul then xforms controls should looks and
behave self as xul controls. In instance, since xul 'textbox' element has
context menu then I belive xforms 'input' elemnt should have context menu also.
Threafore I guess in order to provide a work xforms controls in xul document we
should rewrite some xforms controls (not styles only as I propose before).

Comment 7

[Allan Beaufour]

> Allan, if you wrotes this example then please comment the bug. Why native xforms
> controls doesn't work with xul?

As you have found out there are more than one issue. But for "XForms in XUL out
of the box", we would need to:

1) create implementations for all XForms controls in XUL instead of using HTML
components.

2) figure out how to do bindings that the depend on the namespace of the
document. Is there a "root node" target in CSS?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)

> 2) figure out how to do bindings that the depend on the namespace of the
> document. Is there a "root node" target in CSS?

Yes, :root pseudo-class is working.

> 1) create implementations for all XForms controls in XUL instead of using HTML
> components.

I can try to create implementation at least partly. I should use XForms in XUL
and threafore I will should realize XForms controls in XUL in any case.

Comment 9

[Allan Beaufour]

(In reply to comment #8)
> (In reply to comment #7)
> 
> > 2) figure out how to do bindings that the depend on the namespace of the
> > document. Is there a "root node" target in CSS?
> 
> Yes, :root pseudo-class is working.

Ok. So we could change our current bindings from "xf|input" to "xhtml|:root
xf|input", and you could do "xul|:root xf|input"?

> > 1) create implementations for all XForms controls in XUL instead of using HTML
> > components.
> 
> I can try to create implementation at least partly. I should use XForms in XUL
> and threafore I will should realize XForms controls in XUL in any case.

Most of the elements will be simple, and you can inherit from the common
implementation, but implementing select and select1 will be tricky. But give it
a go.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)

> Most of the elements will be simple, and you can inherit from the common
> implementation, but implementing select and select1 will be tricky. But give it
> a go.

I propose to mark out content from interface for current bindings of xfroms
controls. It has one advantage. In this case we should be able to use one
interface for two contents: binding for xul and binding for xhtml.

In instance:

<binding id="input-iface">
<implementation>
  <!-- here behaviour of xforms:input -->
</implementation>
</binding>

<binding id="html-input" extends="#input-iface">
<content>
 <!-- here content for xhtml -->
</content>

<binding id="xul-input" extends="#input-iface">
<content>
 <!-- here content for xul -->
</content>

What do you think about it?

I guess this bug should be depended on bug 306764, since fixing of it involves a
changes of bindings for xforms controls.

Comment 11

[Allan Beaufour]

(In reply to comment #10)
> (In reply to comment #9)
> 
> > Most of the elements will be simple, and you can inherit from the common
> > implementation, but implementing select and select1 will be tricky. But give it
> > a go.
> 
> I propose to mark out content from interface for current bindings of xfroms
> controls. It has one advantage. In this case we should be able to use one
> interface for two contents: binding for xul and binding for xhtml.
> 
> In instance:
> 
> <binding id="input-iface">
> <implementation>
>   <!-- here behaviour of xforms:input -->
> </implementation>
> </binding>
> 
> <binding id="html-input" extends="#input-iface">
> <content>
>  <!-- here content for xhtml -->
> </content>
> 
> <binding id="xul-input" extends="#input-iface">
> <content>
>  <!-- here content for xul -->
> </content>
> 
> What do you think about it?

If it's possible in practice to do that seperation, then it's fine with me, but
I fear than you cannot do as clean a cut as that. Let's see.

> I guess this bug should be depended on bug 306764, since fixing of it
> involves a changes of bindings for xforms controls.

Possibly, yes.

Comment 12

[alexander :surkov (:asurkov)]

Attachment: xhtml test

Test case for xhtml controls (don't including select and select1 controls)

Comment 13

[alexander :surkov (:asurkov)]

Attachment: xul test

Test case for xul controls (don't including select and select1 controls)

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 195370 [details]
testcase

><window width="600" height="230" title="XForms Calculator" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" xmlns:ev="http://www.w3.org/2001/xml-events" xmlns:xforms="http://www.w3.org/2002/xforms"><xforms:model id="xf_model">
>    <xforms:instance>
>      <equation xmlns="">
>	<screen>0</screen>
>	<screenbuffer>0</screenbuffer>
>	<first>0</first>
>	<second>0</second>
>	<memory>0</memory>
>	<result/>
>      </equation>
>    </xforms:instance>
>  </xforms:model><vbox><hbox style="font-size: 30pt;"><spacer flex="1"/><xforms:output ref="/equation/screen"/><spacer flex="1"/></hbox><hbox><xforms:output ref="/equation/memory">
>        <xforms:label>M:</xforms:label>
>      </xforms:output></hbox><hbox><xforms:trigger>
>        <xforms:label>MC</xforms:label>
>        <xforms:action ev:event="DOMActivate">        
>        <xforms:setvalue ref="/equation/memory" value="0"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>7</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/screenbuffer" value="/equation/screenbuffer * 10 + 7"/>
>          <xforms:setvalue ref="/equation/screen" value="/equation/screenbuffer"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>8</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/screenbuffer" value="/equation/screenbuffer * 10 + 8"/>
>          <xforms:setvalue ref="/equation/screen" value="/equation/screenbuffer"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>9</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/screenbuffer" value="/equation/screenbuffer * 10 + 9"/>
>          <xforms:setvalue ref="/equation/screen" value="/equation/screenbuffer"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>/</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/first" value="/equation/screen"/>
>          <xforms:setvalue ref="/equation/screenbuffer" value="0"/>
>          <xforms:toggle ev:event="DOMActivate" case="divide"/>
>        </xforms:action>
>      </xforms:trigger></hbox><hbox><xforms:trigger>
>        <xforms:label>MR</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/screenbuffer" value="/equation/memory"/>
>          <xforms:setvalue ref="/equation/screen" value="/equation/memory"/>
>          <xforms:setvalue ref="/equation/screenbuffer" value="0"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>4</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/screenbuffer" value="/equation/screenbuffer * 10 + 4"/>
>          <xforms:setvalue ref="/equation/screen" value="/equation/screenbuffer"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>5</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/screenbuffer" value="/equation/screenbuffer * 10 + 5"/>
>          <xforms:setvalue ref="/equation/screen" value="/equation/screenbuffer"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>6</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/screenbuffer" value="/equation/screenbuffer * 10 + 6"/>
>          <xforms:setvalue ref="/equation/screen" value="/equation/screenbuffer"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>*</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/first" value="/equation/screen"/>
>          <xforms:setvalue ref="/equation/screenbuffer" value="0"/>
>          <xforms:toggle ev:event="DOMActivate" case="multiply"/>
>        </xforms:action>
>      </xforms:trigger></hbox><hbox><xforms:trigger>
>        <xforms:label>MS</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/memory" value="/equation/screen"/>
>          <xforms:setvalue ref="/equation/screenbuffer" value="0"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>1</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/screenbuffer" value="/equation/screenbuffer * 10 + 1"/>
>          <xforms:setvalue ref="/equation/screen" value="/equation/screenbuffer"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>2</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/screenbuffer" value="/equation/screenbuffer * 10 + 2"/>
>          <xforms:setvalue ref="/equation/screen" value="/equation/screenbuffer"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>3</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/screenbuffer" value="/equation/screenbuffer * 10 + 3"/>
>          <xforms:setvalue ref="/equation/screen" value="/equation/screenbuffer"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>-</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/first" value="/equation/screen"/>
>          <xforms:setvalue ref="/equation/screenbuffer" value="0"/>
>          <xforms:toggle ev:event="DOMActivate" case="subtract"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>1/x</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/screen" value="1 div /equation/screen"/>
>        </xforms:action>
>      </xforms:trigger></hbox><hbox><xforms:trigger>
>        <xforms:label>M+</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/memory" value="/equation/memory + /equation/screen"/>
>          <xforms:setvalue ref="/equation/screenbuffer" value="0"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>0</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/screenbuffer" value="/equation/screenbuffer * 10"/>
>          <xforms:setvalue ref="/equation/screen" value="/equation/screenbuffer"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>+/-</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/screen" value="/equation/screen * -1"/>
>        </xforms:action>
>      </xforms:trigger><xforms:trigger>
>        <xforms:label>+</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/first" value="/equation/screen"/>
>          <xforms:setvalue ref="/equation/screenbuffer" value="0"/>
>          <xforms:toggle ev:event="DOMActivate" case="add"/>
>        </xforms:action>
>      </xforms:trigger><xforms:switch>
>        <xforms:case id="add" selected="true">
>          <xforms:trigger>
>            <xforms:label>=</xforms:label>
>            <xforms:action ev:event="DOMActivate">
>              <xforms:setvalue ref="/equation/second" value="/equation/screenbuffer"/>
>              <xforms:setvalue ref="/equation/result" value="/equation/first + /equation/second"/>
>            <xforms:setvalue ref="/equation/screen" value="/equation/result"/>
>            <xforms:setvalue ref="/equation/screenbuffer" value="0"/>
>            </xforms:action>
>          </xforms:trigger>
>        </xforms:case>
>        <xforms:case id="subtract">
>          <xforms:trigger>
>            <xforms:label>=</xforms:label>
>            <xforms:action ev:event="DOMActivate">
>              <xforms:setvalue ref="/equation/second" value="/equation/screenbuffer"/>
>              <xforms:setvalue ref="/equation/result" value="/equation/first - /equation/second"/>
>            <xforms:setvalue ref="/equation/screen" value="/equation/result"/>
>            <xforms:setvalue ref="/equation/screenbuffer" value="0"/>
>            </xforms:action>
>          </xforms:trigger>
>        </xforms:case>
>        <xforms:case id="multiply">
>          <xforms:trigger>
>            <xforms:label>=</xforms:label>
>            <xforms:action ev:event="DOMActivate">
>              <xforms:setvalue ref="/equation/second" value="/equation/screenbuffer"/>
>              <xforms:setvalue ref="/equation/result" value="/equation/first * /equation/second"/>
>            <xforms:setvalue ref="/equation/screen" value="/equation/result"/>
>            <xforms:setvalue ref="/equation/screenbuffer" value="0"/>
>            </xforms:action>
>          </xforms:trigger>
>        </xforms:case>
>        <xforms:case id="divide">
>          <xforms:trigger>
>            <xforms:label>=</xforms:label>
>            <xforms:action ev:event="DOMActivate">
>              <xforms:setvalue ref="/equation/second" value="/equation/screenbuffer"/>
>              <xforms:setvalue ref="/equation/result" value="/equation/first div /equation/second"/>
>            <xforms:setvalue ref="/equation/screen" value="/equation/result"/>
>            <xforms:setvalue ref="/equation/screenbuffer" value="0"/>
>            </xforms:action>
>          </xforms:trigger>
>        </xforms:case>
>      </xforms:switch><xforms:trigger>
>        <xforms:label>Clear</xforms:label>
>        <xforms:action ev:event="DOMActivate">
>          <xforms:setvalue ref="/equation/first" value="0"/>
>          <xforms:setvalue ref="/equation/second" value="0"/>
>          <xforms:setvalue ref="/equation/result" value="0"/>
>          <xforms:setvalue ref="/equation/screen" value="0"/>
>          <xforms:setvalue ref="/equation/screenbuffer" value="0"/>
>          <xforms:toggle ev:event="DOMActivate" case="add"/>
>        </xforms:action>
>      </xforms:trigger></hbox></vbox></window>

Comment 15

[alexander :surkov (:asurkov)]

Threre is a question. Element <xf:input incremental="true"/> (xformswidget-input
binding) updates delegate value when 'click' event is occured. I'm not clear in
one's mind why can value be changed when 'click' event is occured? Element
<xf:input type="xsd:boolean"/> has the next check when 'click' event is occured:

if (this.getAttribute("incremental") != "false")

Should such check be presented because empty and "false" values are equivalent?

if (this.getAttribute("incremental") == "true")

Comment 16

[alexander :surkov (:asurkov)]

covered bugs:

1) xformswidget-trigger-minimal

native code:
onclick="if (!this._disabled) this.parentNode.dispatchDOMActivate();"

should be:
onclick="if (!this.parentNode._disabled) this.parentNode.dispatchDOMActivate();"

2) xformswidget-secret

native code:
this.inputField.readonly = this.delegate.isReadonly;

should be:
if(this.delegate.isReadonly) this.inputField.setAttribute('readonly', 'true');
else this.inputField.removeAttribute('readonly');

since 'readonly' property of xhtml:input doesn't work.

Comment 17

[alexander :surkov (:asurkov)]

Since html:input type="checkbox" doesn't support 'readonly' attribute then we
should use 'disabled'.

xformswidget-input-boolean code:

if (this.delegate.isReadonly) {
 this.inputField.setAttribute("readonly", "readonly");
} else {
 this.inputField.removeAttribute("readonly");
}

Comment 18

[Allan Beaufour]

(In reply to comment #15)
> Threre is a question. Element <xf:input incremental="true"/> (xformswidget-input
> binding) updates delegate value when 'click' event is occured. I'm not clear in
> one's mind why can value be changed when 'click' event is occured?

Hmm, I see no reason for it either. I guess clicking should not do anything for
an ordinary input. But it's not terribly important, as nothing will happen when
the value is not changed. If you feel for it, create a bug for it ;-)


>Element
> <xf:input type="xsd:boolean"/> has the next check when 'click' event is occured:
> 
> if (this.getAttribute("incremental") != "false")
> 
> Should such check be presented because empty and "false" values are equivalent?
> 
> if (this.getAttribute("incremental") == "true")

The difference between the checks is that the bool-input is incremental by
default, as it is more intuitive to the user.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)

Ok, I agree. But has xforms spec something about incremental behaviour of
bool-input or it is xforms realization feature?

> If you feel for it, create a bug for it ;-)

Also please note comment #16 and comment #17. I guess I'll fix this bugs in
patch for this bug if you'll not be against it.

Comment 20

[alexander :surkov (:asurkov)]

Attachment: xhtml test

Comment 21

[alexander :surkov (:asurkov)]

Attachment: xul test

Comment 22

[alexander :surkov (:asurkov)]

Attachment: patch, common controls

Use 'xhtml test' and 'xul test' attachments to test the patch. Patch fixes
common controls (non select and select1 controls) only.

Comment 23

[alexander :surkov (:asurkov)]

Sorry but patch has Windows line-endings again :). After patch remark I'll fix this.

Comment 24

[aaronr]

(In reply to comment #19)
> (In reply to comment #18)
> 
> Ok, I agree. But has xforms spec something about incremental behaviour of
> bool-input or it is xforms realization feature?
> 

boolean inputs having incremental="true" behavior by default isn't mentioned in
the spec.  It is something that we observed in other processors and like Allan
said, it really does seem like what a user would expect.  So we do it, too.

Comment 25

[alexander :surkov (:asurkov)]

Before fixing select and select1 controls I guess some bugs should be fixed (in
instance, bug 303353) and I'll have some questions.

Now <xf:select appearance="minimal"/> looks like <xf:select
appearance="compact"/>. How compact select should looks? Should it looks like on
image presented in specs?

Comment 26

[Allan Beaufour]

(In reply to comment #19)
> (In reply to comment #18)
> 
> Ok, I agree. But has xforms spec something about incremental behaviour of
> bool-input or it is xforms realization feature?
> 
> > If you feel for it, create a bug for it ;-)
> 
> Also please note comment #16 and comment #17. I guess I'll fix this bugs in
> patch for this bug if you'll not be against it.

It's a bit bigger a hassle for you to create seperate bugs, but giant "fix it
all bugs" are a pain to review, and it's difficult to figure out what a patch
does. So I very much prefer seperate bugs.

Comment 27

[Allan Beaufour]

(In reply to comment #25)
> Before fixing select and select1 controls I guess some bugs should be fixed (in
> instance, bug 303353) and I'll have some questions.
> 
> Now <xf:select appearance="minimal"/> looks like <xf:select
> appearance="compact"/>. How compact select should looks? Should it looks like on
> image presented in specs?

Probably :) My advise is to ignore the appearance for now, and just make one
appearance work.

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #26)
> (In reply to comment #19)

> > Also please note comment #16 and comment #17. I guess I'll fix this bugs in
> > patch for this bug if you'll not be against it.
> 
> It's a bit bigger a hassle for you to create seperate bugs, but giant "fix it
> all bugs" are a pain to review, and it's difficult to figure out what a patch
> does. So I very much prefer seperate bugs.

It is a thing that I was afraid too badly. When I did a attemt to separate
interface from content then I was forced to do a big changes of code. And
treafore this bugs was't fixed as you understood but they gone because of code
changing. Since code was changed vastly then patch review will be difficult in
any case. 

I see three approaches for bug fixing:
1) to do as I mention (separation interface from content)
2) to write xul controls above html controls. I belive it's ugly solution.
3) to write xul controls don't linking them with xhtml controls. I guess this
approach entails complexity when controls will be modified.

If you see something else please say me. Please take a look at my patch, maybe
it brings to you some thoughts.

Comment 29

[Allan Beaufour]

(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #19)
> 
> > > Also please note comment #16 and comment #17. I guess I'll fix this bugs in
> > > patch for this bug if you'll not be against it.
> > 
> > It's a bit bigger a hassle for you to create seperate bugs, but giant "fix it
> > all bugs" are a pain to review, and it's difficult to figure out what a patch
> > does. So I very much prefer seperate bugs.
> 
> It is a thing that I was afraid too badly. When I did a attemt to separate
> interface from content then I was forced to do a big changes of code. And
> treafore this bugs was't fixed as you understood but they gone because of code
> changing. Since code was changed vastly then patch review will be difficult in
> any case. 

We're getting ready for another preview, so I think it would be unwise to do
such big changes to the UI before that, so doing small fixes to existing
controls should go to seperate bugs so we'll get it in to the preview.

> I see three approaches for bug fixing:
> 1) to do as I mention (separation interface from content)
> 2) to write xul controls above html controls. I belive it's ugly solution.
> 3) to write xul controls don't linking them with xhtml controls. I guess this
> approach entails complexity when controls will be modified.
> 
> If you see something else please say me. Please take a look at my patch, maybe
> it brings to you some thoughts.

I've quickly looked through your patch, and I like approach 1. If it works for
XUL and HTML, it could work for other languages too which is really good. I'm
not sure that I like your getControlElement though. Couldn't the functionality
you expose there be normal XBL methods?

Olli, what's your take on all this?

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)

> I've quickly looked through your patch, and I like approach 1. If it works for
> XUL and HTML, it could work for other languages too which is really good. I'm
> not sure that I like your getControlElement though. Couldn't the functionality
> you expose there be normal XBL methods?
> 

XForms controls interface ask content to provide the next interface:

readonly
disabled
value
control - dom node, f.x. for focus setting

If you don't like getControlElement() method then we could replace it by properties:

controlReadonly
controlDisabled
controlValue
control

At first I wrote the patch using such approach. But getControlElement() pleased
to me a little more. If you like to see xbl properties instead of control object
then I can rewrite patch. Do you like such properties names?

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #29)

> 
> We're getting ready for another preview, so I think it would be unwise to do
> such big changes to the UI before that, so doing small fixes to existing
> controls should go to seperate bugs so we'll get it in to the preview.
> 

I can post covered bugs and I can to post patches for them. But I belive it is
wasted labour since architecture of controls is changed and this bugs are
disappeared after patch applying. In other words covered bugs can't be exists in
mentioned architecture in form in what they was covered.

Comment 32

[Allan Beaufour]

(In reply to comment #30)
> If you like to see xbl properties instead of control object
> then I can rewrite patch. Do you like such properties names?

I personally think it's a lot easier and cleaner to look at, but I'm no big JS
guy. What do the rest of you say?

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 197676 [details] [diff] [review]
patch, common controls

I think I like this. The change to the current architecture isn't so big.
But before accepting the change, I'd like to see some SVG widgets too and in
such way that one can dynamically switch from XHTML widgets to SVG widgets. I
wouldn't like to loose that feature.

And of course, converting <xf:input> is much easier than for example
<xf:select1>...

The getControlElement is a good idea. I prefer that to the XBL methods.

Comment 34

[alexander :surkov (:asurkov)]

(In reply to comment #32)
> (In reply to comment #30)
> > If you like to see xbl properties instead of control object
> > then I can rewrite patch. Do you like such properties names?
> 
> I personally think it's a lot easier and cleaner to look at, but I'm no big JS
> guy. What do the rest of you say?

I don't think it's more easier. Cleaner? I realy don't know. As one reason I can
mention the next. Method getControlElement() looks like property 'delegate'. We
have 'delegate' property to work with model and we have 'control' property to
work with control. I guess it's nice solution. What do you think? Although AFAIK
no one binding of mozilla tookit is realized by using js object. Maybe it is not
standard approach for xbls.

Comment 35

[alexander :surkov (:asurkov)]

(In reply to comment #33)
> (From update of attachment 197676 [details] [diff] [review] [edit])
> I think I like this. The change to the current architecture isn't so big.
> But before accepting the change, I'd like to see some SVG widgets too and in
> such way that one can dynamically switch from XHTML widgets to SVG widgets. I
> wouldn't like to loose that feature.
> 

I don't know svg a good. Please what kind of widget you like to see. I'll try to
realize it. How should looks the dynamicaly switch from xhtml widget to svg widget?

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #35)
> 
> I don't know svg a good. Please what kind of widget you like to see. I'll try to
> realize it. How should looks the dynamicaly switch from xhtml widget to svg
widget?

This is one example
http://www.cs.helsinki.fi/u/pettay/moztests/xforms/dynamic_ui/calc_svg.xhtml

Comment 37

[alexander :surkov (:asurkov)]

I loved svg :), should we include svg controls for xforms into a package as xul
controls?

Comment 38

[alexander :surkov (:asurkov)]

Attachment: svg controls

Comment 39

[alexander :surkov (:asurkov)]

Attachment: svg test

Comment 40

[alexander :surkov (:asurkov)]

(In reply to comment #33)

> But before accepting the change, I'd like to see some SVG widgets too and in
> such way that one can dynamically switch from XHTML widgets to SVG widgets. I
> wouldn't like to loose that feature.

I'll write some svg controls (label, output and trigger). XForms and SVG can
work together. Though I couldn't write input, secret, textarea and select
controls and now I can't imagine how can I do this. Attached test 'svg test'
doesn't show dynamicaly switches but I guess it will work fine.

Comment 41

[alexander :surkov (:asurkov)]

Attachment: xhtml select test

Comment 42

[alexander :surkov (:asurkov)]

Attachment: xul select test

Comment 43

[alexander :surkov (:asurkov)]

Attachment: patch, common and select controls

Comment 44

[alexander :surkov (:asurkov)]

Attachment: xhtml test [select controls]

Comment 45

[alexander :surkov (:asurkov)]

Attachment: xul test [select controls]

Comment 46

[alexander :surkov (:asurkov)]

Attachment: patch

Patch contains all XForms controls for XHTML and XUL.
Select and select1 controls have common base. Also patch realizes <select1
appearance='compact'/> and <select1 appearance='full'/> controls.

For testing use 'xhtml test' and 'xul test' attachments for common controls and
'xhtml test [select controls]' and 'xul test [select controls]' attachments for
select and select1 controls.

Comment 47

[Allan Beaufour]

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

Finally got around to this. Sorry for the delay. It's cool work.

Problem is that it does not apply cleanly on trunk (select.xml), and you need
to put your new chrome files in jar.mn.

Could you please supply a new patch?

Comment 48

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 49

[Doron Rosenberg (IBM)]

Why do we need seperate .xml files for a xul and xhtml version?

The select.xml for example should have all the code for xhtml, and have a xul
binding in the same file overwrite what it needs.  Less code duplication.  Plus,
not sure we need to change the way select.xml works that much for XUL to
override stuff.

Using a timeout is unexceptable as well.

Comment 50

[alexander :surkov (:asurkov)]

(In reply to comment #49)
> Why do we need seperate .xml files for a xul and xhtml version?
> 

There are one reason. If base, xul and xhtml controls will be described in one
file then file will be big. If some xml language (like svg) will provide own
select control then select.xml file will be even greater. If you like to see
xhmtl controls and xul controls in one file then I can combine into one file
common controls (xforms.xml) and select controls (select.xml) and I can file a
new patch.

> The select.xml for example should have all the code for xhtml, and have a xul
> binding in the same file overwrite what it needs.  Less code duplication.  

Please say what code duplication do you see?


> Plus,
> not sure we need to change the way select.xml works that much for XUL to
> override stuff.
> 

Not only for xul. Changes allow to write select1 controls for xhtml too. Also
changes allow to write small controls (please compare with
select[appearance="full"]).

> Using a timeout is unexceptable as well.

I belive timeout is a bad solution, but i can't see a way to do it otherwise.
When I try to append xul:listitem to selection then listitem binding isn't
created sometimes and threafore I can't select it. 

Comment 51

[alexander :surkov (:asurkov)]

Attachment: patch

fixed style issues
fixed select compact and selec1 compact in "browser mode"

Comment 52

[Allan Beaufour]

(In reply to comment #51)
> Created an attachment (id=199884) [edit]
> patch
> 
> fixed style issues
> fixed select compact and selec1 compact in "browser mode"

I've been fighting with the build bug today, so I haven't had much time too look
at this. The only thing I've gotten to is that it looks like your xul:selects
behaves as incremental="true" all the time... that's wrong.

Comment 53

[alexander :surkov (:asurkov)]

Attachment: xhtml test [select controls]

Comment 54

[alexander :surkov (:asurkov)]

Attachment: xul test [select controls]

Comment 55

[alexander :surkov (:asurkov)]

Attachment: patch

fixed some problems related to selects incremental="false"

Comment 56

[alexander :surkov (:asurkov)]

(In reply to comment #55)
> 
> fixed some problems related to selects incremental="false"

There is a problem. Element <select incremental='false' appearance='full'/>
updates instance data on all internal blur events. Actually it should update
instance data when focus leaves control.

Comment 57

[Allan Beaufour]

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

I'm definately not the right to review this... I suck at JS, and am not into
how select and select1 is actually implemented. Olli or Doron, could you review
this?

My overall comments are that I like the split between core, xhtml and xul, and
having native support for XUL would be a good asset. So I'm very much for this
patch.

There may be problems with the XUL controls, but I'm on for taking them in and
fixing them as we go. We just need to make sure that XHTML works.

> +++ mozilla/extensions/xforms/jar.mn	2005-10-21 17:32:36.000000000 +0900
> @@ -8,8 +8,11 @@
>  * content/xforms/xforms-prefs-ui.xul           (resources/content/xforms-prefs-ui.xul)
>  * content/xforms/xforms-prefs.js               (resources/content/xforms-prefs.js)
>    content/xforms/xforms.xml                    (resources/content/xforms.xml)
> -  content/xforms/select1.xml                   (resources/content/select1.xml)
> +  content/xforms/xforms-xhtml.xml                    (resources/content/xforms-xhtml.xml)
> +  content/xforms/xforms-xul.xml                    (resources/content/xforms-xul.xml)
>    content/xforms/select.xml                    (resources/content/select.xml)
> +  content/xforms/select-xhtml.xml                    (resources/content/select-xhtml.xml)
> +  content/xforms/select-xul.xml                    (resources/content/select-xul.xml)
>    content/xforms/bindingex.css                 (resources/content/bindingex.css)
>    content/xforms/bindingex.xul                 (resources/content/bindingex.xul)
>  * locale/en-US/xforms/contents.rdf             (resources/locale/en-US/contents.rdf)

Fix the indentation

> +++ mozilla/extensions/xforms/resources/content/select-xhtml.xml	2005-10-21 17:32:36.000000000 +0900
> +   - Contributor(s):
> +   -  Doron Rosenberg <[e-mail]>
> +   -  Alexander Surkov <[e-mail]>

You're not being fair to Olli here... and possibly in other files too.

> +  <binding id="xformswidget-select-minimal"
> +           extends="chrome://xforms/content/select-xhtml.xml#xformswidget-select"/>
> +
> +  <binding id="xformswidget-select-compact"
> +           extends="chrome://xforms/content/select-xhtml.xml#xformswidget-select"/>
> +
> +  <binding id="xformswidget-select-full"
> +           extends="chrome://xforms/content/select-xhtml.xml#xformswidget-select"/>

Why are these necessary? Can't you just bind all select controls to
xformswidget-select?

> +  <!-- WIDGET FOR SELECT APPEARANCE='COMPACT' -->
> +  <binding id="widget-select-compact"
> +           extends="chrome://xforms/content/select.xml#widget-base">
> +
> +    <content>
> +      <html:select xbl:inherits="style, disabled=readonly"
> +                     multiple="true"
> +                     size="5"
> +                     anonid="control"/>
> +    </content>
> +
> +    <implementation>
> +      <property name="readonly"
> +                     onget="return this.getAttribute('readonly' == 'true') ? true : false;"
> +                     onset="if(val) this.setAttribute('readonly', 'true'); else this.removeAttribute('readonly');"/>

What's this? There should be no readonly attribute on xforms:select.


> +++ mozilla/extensions/xforms/resources/content/xforms.css	2005-10-21 17:32:35.000000000 +0900
> @@ -45,10 +46,7 @@
>  contextcontainer,
>  group,
>  switch,
> -case,
> -select1 item,
> -select1 itemset,
> -select1 choices {
> +case{

case {

> -input {
> -  -moz-binding: url('chrome://xforms/content/xforms.xml#xformswidget-input');
> +xul|*:root output {
> +  -moz-binding: url('chrome://xforms/content/xforms-xul.xml#xformswidget-output');
> +  display: -moz-box;
>  }

What does "display: -moz-box" do?

> -/* Most of the select1 specific CSS is copied from forms.css */
> -
> -select1 item {
> -  -moz-binding: url('chrome://xforms/content/select1.xml#xformswidget-select1-item');
> -  white-space : nowrap;
> +xul|*:root trigger, xul|*:root submit {
> +  -moz-binding: url('chrome://xforms/content/xforms-xul.xml#xformswidget-trigger');
>  }

> -html|input.-moz-xforms-select1-dropdown {
> -  width: 12px;
> -  height: 1.3em;
> -  white-space: nowrap;
> -  position: static !important;
> -  background-image: url("data:image/gif;base64,R0lGODlhBwAEAIAAAAAAAP%2F%2F%2FyH5BAEAAAEALAAAAAAHAAQAAAIIhA%2BBGWoNWSgAOw%3D%3D") !important;
> -  background-repeat: no-repeat !important;
> -  background-position: center !important;
> -  -moz-appearance: menulist-button;
> -  -moz-user-select: none !important;
> -  -moz-user-focus: ignore !important;
> -  -moz-binding: none;
> -  vertical-align: text-top;
> -  margin: 0px !important;
> -  margin-top: -1px !important;

Why isn't all this magic needed anymore?

Comment 58

[alexander :surkov (:asurkov)]

(In reply to comment #57)

> Fix the indentation
> 

What exactly should I do?


> Why are these necessary? Can't you just bind all select controls to
> xformswidget-select?

When appearance attribute is changed then we should call nsIXFormsDelegate.widgetAttached() method. When we have separate binding for each appearance attibute value then it was happen automatically. I tried to realize this by using DOMAttrModified event but I was failed because binding for internal control is not changed yet.


> What's this? There should be no readonly attribute on xforms:select.
> 

There is no 'readonly' attribute on xforms:select. There is 'readonly' attribute on internal element of xfroms:select.


> What does "display: -moz-box" do?
> 

Custom namespace element looks like xul:box element. There are some layout problems for custom ns elements in xul context.


> 
> Why isn't all this magic needed anymore?
> 

Since select1.xml isn't used then it's styles are useless.

Comment 59

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 60

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

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

I didn't test this, but if I read the patch correctly, it breaks <select1 selection="open"/>.
Right?

selection="open" was the main reason to implement <select1> as a "dhtml widget".

Comment 62

[alexander :surkov (:asurkov)]

(In reply to comment #61)
> (From update of attachment 200599 [details] [diff] [review] [edit])
> I didn't test this, but if I read the patch correctly, it breaks <select1
> selection="open"/>.
> Right?
> 
> selection="open" was the main reason to implement <select1> as a "dhtml
> widget".
> 

Yes, you're right, I forgot about it. As I understand you want selection='open' realization for <select1 appearance='minimal'/>. Am I right?

There is a quick way to support this: xhtml minimal select1 leaving as is. Maybe it isn't so nice solution but it isn't bad. Because now there is no open selection realization for select, select1 controls (except minimal select1) and at first we should define how should them behave. How do you think?

Comment 63

[alexander :surkov (:asurkov)]

Attachment: patch (with select1)

Patch doesn't remove select1 appearance='minimal' for xhtml context. Now selection='open' works for it. I think it's better to file new bug about supporting open selection for other controls because I belive small patches is better than big.

Comment 64

[Sébastien Auger]

hmm .. any idea what can we do with this ? this patch looks good but as said we'll need to fill another bug and come up with a small patch later. I don't think it will make 1.5 except if they decide to check this in before after RC1 release for RC2. But definately, it's something i would like to have before 1.5 . What you guys plan for this one ?

Comment 65

[Sébastien Auger]

(In reply to comment #64)
> hmm .. any idea what can we do with this ? this patch looks good but as said
> we'll need to fill another bug and come up with a small patch later. I don't
> think it will make 1.5 except if they decide to check this in before after RC1
> release for RC2. But definately, it's something i would like to have before 1.5
> . What you guys plan for this one ?
> 

OMG, i'm still a bit asleep i thought it was a branch bug. Really sorry for these two unneeded auto-CC's :S

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

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

I'll review the patch with select1

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 200714 [details] [diff] [review]
patch (with select1)

please use spaces, not tabs.

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 200714 [details] [diff] [review]
patch (with select1)

(In reply to comment #67)
> (From update of attachment 200714 [details] [diff] [review] [edit])
> please use spaces, not tabs.
> 

And could you fix indentations and other syntax style problems.
It is somewhat difficult to read the current patch.
http://www.mozilla.org/hacking/mozilla-style-guide.html is mainly
for C++, but should give a hint ;)

Comment 69

[alexander :surkov (:asurkov)]

Attachment: patch

fixed code style issues

Comment 70

[alexander :surkov (:asurkov)]

Attachment: patch

updated to current build

Comment 71

[Olli Pettay [:smaug][[e-mail]]]

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

This may sound nitpicking to you, but still comments about coding style;) Right style makes code so much more readable.

The syntax for if-else should be

if (expression)
  statement;
else
  statement;


or

if (expression) {
  1_or_more_statements;
} else {
  1_or_more_statements;
}

Indentation is 2 spaces.

If attributes can't be put to the same line, they should be handled like this (if possible):
<element attr="val"
         attr2="val2"/>

I'll try to really review the code tomorrow, but marking r- because of styling issues (at least because of those).

Comment 72

[alexander :surkov (:asurkov)]

Attachment: patch

fixed style issues

Comment 73

[Olli Pettay [:smaug][[e-mail]]]

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

The patch doesn't use the new version of Custom Control API  (Bug 306764)
And btw, I found still at least one tab in the patch.

>+
>+  <binding id="xformswidget-select-minimal"
>+           extends="chrome://xforms/content/select-xhtml.xml#xformswidget-select"/>
>+
>+  <binding id="xformswidget-select-compact"
>+           extends="chrome://xforms/content/select-xhtml.xml#xformswidget-select"/>
>+
>+  <binding id="xformswidget-select-full"
>+           extends="chrome://xforms/content/select-xhtml.xml#xformswidget-select"/>
>+

The reason for these bindings is not quite clear to me.


>+
>+    <handlers>
>+      <handler event="focus" phase="capturing">
>+        this.dispatchDOMUIEvent("DOMFocusIn");
>+      </handler>
>+
>+      <handler event="blur" phase="capturing">
>+        this.updateModelData();
>+	this.dispatchDOMUIEvent("DOMFocusOut");
>+      </handler>
>+
>+      <handler event="change">
>+        this.updateModelDataIncremental();
>+      </handler>
>+    </handlers>


If possible, I'd attach event listeners to the element which is the originalTarget
of the event. So usually it is some element in the anonymous content.

>+  <!-- WIDGET FOR SELECT1 APPEARANCE='MINIMAL' -->
>+  <binding id="widget-select1-minimal"
>+           extends="chrome://xforms/content/select-xhtml.xml#widget-select-compact">
>+
>+    <content>
>+      <html:select xbl:inherits="style, disabled=readonly" anonid="control"/>
>+    </content>

Are you still using html:select for some version of <xforms:select1> ?
Any reason for that. I think in XHTML we should use the one 
we already have and XUL should have its own widget which supports the
features <xforms:select1 selection="open" appearance="minimal"/>


>+  
>+  <!-- WIDGET SELECT1 APPEARANCE='FULL' -->
>+  <binding id="widget-select1-full"
>+           extends="chrome://xforms/content/select-xhtml.xml#widget-select-full">
>+
>+    <content>
>+      <html:form anonid="control"/>
>+    </content>

Why do you need <html:form> here?

More comments later. Still haven't tested this, because it doesn't work with the current
trunk.

Comment 74

[alexander :surkov (:asurkov)]

(In reply to comment #73)
> (From update of attachment 201603 [details] [diff] [review] [edit])
> The patch doesn't use the new version of Custom Control API  (Bug 306764)

I updated patch to current build but I can't to check it because I can't find latest xforms package for windows (I checked 5-6 November, ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/).

> And btw, I found still at least one tab in the patch.
> 

I found one tab in the patch. Fixed.

> >+
> >+  <binding id="xformswidget-select-minimal"
> >+           extends="chrome://xforms/content/select-xhtml.xml#xformswidget-select"/>
> >+
> >+  <binding id="xformswidget-select-compact"
> >+           extends="chrome://xforms/content/select-xhtml.xml#xformswidget-select"/>
> >+
> >+  <binding id="xformswidget-select-full"
> >+           extends="chrome://xforms/content/select-xhtml.xml#xformswidget-select"/>
> >+
> 
> The reason for these bindings is not quite clear to me.
> 

I added comment about it into the patch. 

The reason why I do it is the following. When appearance attribute is changed then we should call nsIXFormsDelegate.widgetAttached() method. If we will have separate binding for each appearance attibute value then it will be happen automatically. I tried to realize this by using DOMAttrModified event but I was failed because binding for internal control is not changed when DOMAttrModified is handled.

> 
> >+
> >+    <handlers>
> >+      <handler event="focus" phase="capturing">
> >+        this.dispatchDOMUIEvent("DOMFocusIn");
> >+      </handler>
> >+
> >+      <handler event="blur" phase="capturing">
> >+        this.updateModelData();
> >+	this.dispatchDOMUIEvent("DOMFocusOut");
> >+      </handler>
> >+
> >+      <handler event="change">
> >+        this.updateModelDataIncremental();
> >+      </handler>
> >+    </handlers>
> 
> 
> If possible, I'd attach event listeners to the element which is the
> originalTarget
> of the event. So usually it is some element in the anonymous content.
> 

It is possible for full select and select1 full. I added the check for it. It's some tricly for other selects and I didn't do it.

> >+  <!-- WIDGET FOR SELECT1 APPEARANCE='MINIMAL -->

> Are you still using html:select for some version of <xforms:select1> ?
> Any reason for that. I think in XHTML we should use the one 
> we already have and XUL should have its own widget which supports the
> features <xforms:select1 selection="open" appearance="minimal"/>
> 
> 

No, I don't use. I forgot to remove this code.

> >+  
> >+  <!-- WIDGET SELECT1 APPEARANCE='FULL' -->
> >+  <binding id="widget-select1-full"
> >+           extends="chrome://xforms/content/select-xhtml.xml#widget-select-full">
> >+
> >+    <content>
> >+      <html:form anonid="control"/>
> >+    </content>
> 
> Why do you need <html:form> here?

Radio buttons with unique name works in coordination when they are placed in one form.

> 
> More comments later. Still haven't tested this, because it doesn't work with
> the current
> trunk.
> 

I'll update the patch but as I said I can't be sure on it. When I'll get latest patch then I'll check it.

Comment 75

[alexander :surkov (:asurkov)]

Attachment: patch

I updated patch to current build but I couldn't check it because I didn't find
latest xforms package for windows (I checked 5-6 November,
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/).

Comment 76

[Olli Pettay [:smaug][[e-mail]]]

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

I get the following error when using <xf:select> (which has both <xf:item>s and <xf:choices>s) inside <xf:repeat>
( testcase http://www.cs.helsinki.fi/u/pettay/moztests/xforms/nested_select.xhtml )
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this.buildChoices is not a function" {file: "chrome://xforms/content/select.xml" line: 326}]' when calling method: [nsIXForm sUIWidget::refresh]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_ DETAILS)"  location: "<unknown>"  data: yes]

I get also:
JavaScript error: chrome://xforms/content/xforms.xml, line 230: syntax error

Check spelling in the comments.

I didn't test the XUL widgets at all yet. Perhaps you have
testcases, which you could put to some website?

>+<!-- SELECT, SELECT1 CONTROLS -->
>+  <binding id="xformswidget-select"
>+           extends="chrome://xforms/content/select.xml#xformswidget-select-base">
>+
>+    <content>
>+      <html:table xbl:inherits="style">
>+        <html:tr>
>+          <html:td valign="top"><children includes="label"/></html:td>
>+          <html:td><html:selectwidget anonid="control" class="xf-value"/></html:td>
>+        </html:tr>
>+      </html:table>
>+      <children/>
>+    </content>

Is the html:table used now for all <select>s? Earlier it was just for
<xforms:select appearance="full"/>, and even that was a bad choice, IMO.


>+      <method name="getSelectedValues">
>+        <body><![CDATA[
>+          var values=[];
>+
>+          var items =
>+            this.control.getElementsByTagNameNS(this.XHTML_NS, "input");
>+          for(var i = 0; i < items.length; i++) {
>+            if (items[i].checked) 
>+              values.push(items[i].value);
>+          }
>+          return values;
>+        ]]></body>
>+      </method>

newline after <body> and before </body>
and use the same style for CDATA sections also elsewhere.

and more comments later ;)

Comment 77

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 78

[alexander :surkov (:asurkov)]

(In reply to comment #76)
> (From update of attachment 202068 [details] [diff] [review] [edit])
> I get the following error when using <xf:select> (which has both <xf:item>s and
> <xf:choices>s) inside <xf:repeat>
> ( testcase
> http://www.cs.helsinki.fi/u/pettay/moztests/xforms/nested_select.xhtml )
> * Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "'[JavaScript Error: "this.buildChoices is not a function" {file:
> "chrome://xforms/content/select.xml" line: 326}]' when calling method:
> [nsIXForm sUIWidget::refresh]"  nsresult: "0x80570021
> (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_ DETAILS)"  location: "<unknown>"  data:
> yes]
> 
> I get also:
> JavaScript error: chrome://xforms/content/xforms.xml, line 230: syntax error
> 

Fixed

> Check spelling in the comments.
> 

Sorry, I forgot to fix this. I'll fix it in the next patch.


> I didn't test the XUL widgets at all yet. Perhaps you have
> testcases, which you could put to some website?
> 

Simple xul tests are attached to the bug: "xul test" and "xul test [select controls]" attacments. If you need something more, please tell me, I can expand tests.

> >+<!-- SELECT, SELECT1 CONTROLS -->
> >+  <binding id="xformswidget-select"
> >+           extends="chrome://xforms/content/select.xml#xformswidget-select-base">
> >+
> >+    <content>
> >+      <html:table xbl:inherits="style">
> >+        <html:tr>
> >+          <html:td valign="top"><children includes="label"/></html:td>
> >+          <html:td><html:selectwidget anonid="control" class="xf-value"/></html:td>
> >+        </html:tr>
> >+      </html:table>
> >+      <children/>
> >+    </content>
> 
> Is the html:table used now for all <select>s? Earlier it was just for
> <xforms:select appearance="full"/>, and even that was a bad choice, IMO.
> 

Yes, for all. If I understand right then we should have the next layout for all select controls:

|-------|---------|
| label | selitem |
|       | selitem |
|       | selitem |
|-------|---------|

Table does it a simple and quick. If you prefer another way then please let me know. I havn't any special preferences.

> 
> >+      <method name="getSelectedValues">
> >+        <body><![CDATA[
> >+          var values=[];
> >+
> >+          var items =
> >+            this.control.getElementsByTagNameNS(this.XHTML_NS, "input");
> >+          for(var i = 0; i < items.length; i++) {
> >+            if (items[i].checked) 
> >+              values.push(items[i].value);
> >+          }
> >+          return values;
> >+        ]]></body>
> >+      </method>
> 
> newline after <body> and before </body>
> and use the same style for CDATA sections also elsewhere.
> 

Fixed

> and more comments later ;)
> 

Comment 79

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #78)
> (In reply to comment #76)
> > (From update of attachment 202068 [details] [diff] [review] [edit] [edit])
> > Is the html:table used now for all <select>s? Earlier it was just for
> > <xforms:select appearance="full"/>, and even that was a bad choice, IMO.
> > 
> 
> Yes, for all. If I understand right then we should have the next layout for all
> select controls:
> 
> |-------|---------|
> | label | selitem |
> |       | selitem |
> |       | selitem |
> |-------|---------|
> 
> Table does it a simple and quick. If you prefer another way then please let me
> know. I havn't any special preferences.
> 

With tables we can't handle for example the following layout:
http://www.cs.helsinki.fi/u/pettay/moztests/xforms/select_inline.xhtml

Comment 80

[alexander :surkov (:asurkov)]

(In reply to comment #79)

> With tables we can't handle for example the following layout:
> http://www.cs.helsinki.fi/u/pettay/moztests/xforms/select_inline.xhtml
> 

I think it can be fixed by setting "display: inline" style. The above example shows <label/> element at bottom of <select/> control. W3c draws <label/> at the top of <select/> (http://www.w3.org/TR/2005/PER-xforms-20051006/slice8.html#ui-selectMany). Where should <label/> be placed?

Comment 81

[alexander :surkov (:asurkov)]

(In reply to comment #80)
> (In reply to comment #79)
> 
> > With tables we can't handle for example the following layout:
> > http://www.cs.helsinki.fi/u/pettay/moztests/xforms/select_inline.xhtml
> > 
> 
> I think it can be fixed by setting "display: inline" style. The above example
> shows <label/> element at bottom of <select/> control. W3c draws <label/> at
> the top of <select/>
> (http://www.w3.org/TR/2005/PER-xforms-20051006/slice8.html#ui-selectMany).
> Where should <label/> be placed?
> 

No, I cant achive desired behaviour with "display: inline" style. As before I think <label/> should be placed at the top of <select/> control. If you agree with it then maybe you know how can it be achived without <table/> using.

Comment 82

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #81)
> 
> No, I cant achive desired behaviour with "display: inline" style. As before I
> think <label/> should be placed at the top of <select/> control. If you agree
> with it then maybe you know how can it be achived without <table/> using.
> 

There are two cases in the example, the <label> at the top and bottom.
With the current <xf:select> implementation we can support them both.

Comment 83

[alexander :surkov (:asurkov)]

Attachment: patch

Now select controls use layouts as before.

Comment 84

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 85

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #83)
> Now select controls use layouts as before.
> 
No it does not. Re-test with
http://www.cs.helsinki.fi/u/pettay/moztests/xforms/select_inline.xhtml
I added two test cases and more comments to the original tests.
If you can produce same kind of layout with the patch, could you tell
how ;)

Comment 86

[alexander :surkov (:asurkov)]

Attachment: patch

Test http://www.cs.helsinki.fi/u/pettay/moztests/xforms/select_inline.xhtml works as it should, I hope :).

Comment 87

[alexander :surkov (:asurkov)]

Attachment: patch

updated to current build

Comment 88

[alexander :surkov (:asurkov)]

Attachment: patch

updated

Comment 89

[aaronr]

(In reply to comment #88)
> Created an attachment (id=204753) [edit]
> patch
> 
> updated
> 

Can you verify that this is updated to the latest level of code?  This patch seems to remove all of my copy handling code in select.xml

Comment 90

[alexander :surkov (:asurkov)]

(In reply to comment #89)
> 
> Can you verify that this is updated to the latest level of code?  This patch
> seems to remove all of my copy handling code in select.xml
> 

You're right. I'll fix it and verify patch update soon.

Comment 91

[alexander :surkov (:asurkov)]

Attachment: patch

updated

Comment 92

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 93

[alexander :surkov (:asurkov)]

I'll split the bug into several small bugs. I belive the patches for them will be easer to review.

Comment 94

[Olli Pettay [:smaug][[e-mail]]]

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

So I guess I can clear this review flag.

Comment 95

[Allan Beaufour]

(In reply to comment #93)
> I'll split the bug into several small bugs. I belive the patches for them will
> be easer to review.

Mighty good idea :) And keep up the good work.

Comment 96

[Allan Beaufour]

We should probably file this as a dupe of the new bugs, or?

Comment 97

[alexander :surkov (:asurkov)]

(In reply to comment #96)
> We should probably file this as a dupe of the new bugs, or?
> 

Probably we should set fixed when new bugs will be fixed.

Comment 98

[Allan Beaufour]

(In reply to comment #97)
> (In reply to comment #96)
> > We should probably file this as a dupe of the new bugs, or?
> > 
> 
> Probably we should set fixed when new bugs will be fixed.

I've "[Meta]"'ed it.

Comment 99

[alexander :surkov (:asurkov)]

Mark as fixed since main xul controls were implemented and bugs blocking this one were fixed. Please use the bug 327236 to track the work under rest of xul controls implementing.