324120 - ban to change value of calculated instance node

Status: RESOLVED WONTFIX

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

When I change value of calculated instance node then nsIXFormsDelegate fires recalculate, revalidate, refresh events. Calculated node value shouldn't be changed and events shouldn't be fired.

See 
nsMDGEngine::setNodeValue() method (
http://lxr.mozilla.org/mozilla/source/extensions/xforms/nsXFormsMDGEngine.cpp#660)
and nsXFormsDelegate::setValue() method (http://lxr.mozilla.org/mozilla/source/extensions/xforms/nsXFormsDelegateStub.cpp#149)

Reproducible: Always

Comment 1

[alexander :surkov (:asurkov)]

Attachment: testcase

Comment 2

[aaronr]

I don't think that this is a valid bug since the spec doesn't say that a calculated node can't be used as the target of a set value.  And we are behaving the same as formsPlayer and XSmiles.

If a form author doesn't want his calculated controls to accept user input and thus generate recalc events, then the form author should probably choose to add the readonly MIP to the bind.  And even then there might be a recalc event generated if xf:setvalue is used on the bound node.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> I don't think that this is a valid bug since the spec doesn't say that a
> calculated node can't be used as the target of a set value.  And we are
> behaving the same as formsPlayer and XSmiles.

> If a form author doesn't want his calculated controls to accept user input and
> thus generate recalc events, then the form author should probably choose to add
> the readonly MIP to the bind.  And even then there might be a recalc event
> generated if xf:setvalue is used on the bound node.
> 

I think value changing of calculated node and following recalc events posting is useless action because it's nothing happen as a result. I can't see a utility of situation when user can change a value of calculated node. If you see the utility when it is useful then please mark bug as invalid. Probably the calculated node should be readonly always. What do you think?

Comment 4

[aaronr]

ok, so you are propsing a styling rule so that any element bound to a calculated node is readonly by default?  Actually, looking at: http://www.w3.org/TR/2005/PER-xforms-20051006/index-all.html#model-prop-readOnly, it seems like that is exactly how it SHOULD behave.  Sounds like this is a valid bug to me.  Good catch Alexander!  I guess no one is handling this right so now we'll be the first :)

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> ok, so you are propsing a styling rule so that any element bound to a
> calculated node is readonly by default?  Actually, looking at:
> http://www.w3.org/TR/2005/PER-xforms-20051006/index-all.html#model-prop-readOnly,
> it seems like that is exactly how it SHOULD behave. 
> 

I guess it shouldn't be a styling rule because taking into account the specs f.x. nsIXFormsAccessors::IsReadonly() should return 'true' if readonly property is not specified. It involves if bind element has calculate property then readonly property should be added to model dependences graph. Is it right?

Thus calculated instance node can be marked as readonly="false()" by following the specs. As before I cant see the utility for it. As I said the above probably the calculated node should be readonly always even if readonly="false()" property is specified. But I guess it is in contradiction with the specs.

> Sounds like this is a
> valid bug to me.  Good catch Alexander!  I guess no one is handling this right
> so now we'll be the first :)

It is cool when we the first! :)

Comment 6

[Allan Beaufour]

Attachment: testcase showing badness

(In reply to comment #5)
> (In reply to comment #4)
> > ok, so you are propsing a styling rule so that any element bound to a
> > calculated node is readonly by default?  Actually, looking at:
> > http://www.w3.org/TR/2005/PER-xforms-20051006/index-all.html#model-prop-readOnly,
> > it seems like that is exactly how it SHOULD behave. 
> > 
> 
> I guess it shouldn't be a styling rule because taking into account the specs
> f.x. nsIXFormsAccessors::IsReadonly() should return 'true' if readonly property
> is not specified. It involves if bind element has calculate property then
> readonly property should be added to model dependences graph. Is it right?

Unfortunately, as it is in the specification, yes. But I do not like it, because if it is a MIP it is also inherited. Badness.

This testcase (courtesy of David) shows the problem.

Comment 7

[Steve Speicher]

Attachment: implicitly sets readonly='true()' on bind if calculate is present

patch sets the readonly MIP to true() if calculate is present and readonly is not explicitly defined.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Created an attachment (id=211810) [edit]
> implicitly sets readonly='true()' on bind if calculate is present
> 
> patch sets the readonly MIP to true() if calculate is present and readonly is
> not explicitly defined.
> 

Probably it would be better if you put the code into MDG.

There is one more stuff. Allan said:

> Unfortunately, as it is in the specification, yes. But I do not like it,
> because if it is a MIP it is also inherited. Badness.

> This testcase (courtesy of David) shows the problem.

It's not clear with the bug (at least for me :)) ). It seems to me it's not all good in the specs :).

Comment 9

[Allan Beaufour]

Comment on attachment 211810 [details] [diff] [review]
implicitly sets readonly='true()' on bind if calculate is present

Uh, no.

1) As Doron also pointed out (regarding comment 6), I do not think the spec. is clear on this.

2) Setting the readonly as an XPath "true()" statement wastes a lot of cycles, when the MDG can be told directly

3) Setting the readonly MIP actually inhibits the form author from setting the MIP through another bind, because it is illegal to set the same MIP twice.

So no go. I would wait for the WG to clear this out.

Comment 10

[Steve Speicher]

(In reply to comment #9)
> 1) As Doron also pointed out (regarding comment 6), I do not think the spec. is
> clear on this.
Spec says: 
6.1.2 The readonly Property
"Default Value: false(), unless a calculate property is specified, then true()."
So, what exactly is unclear.  The badness test case just shows a unique cause of mixed content.
Then later spec says:
"Inheritance Rules: If any ancestor node evaluates to true, this value is treated as true."
Since calculate causes readonly to be "true", then all children must be readonly. Which I see now my changes don't match this.

> 2) Setting the readonly as an XPath "true()" statement wastes a lot of cycles,
> when the MDG can be told directly
Ok, wasn't that brave yet.
 
> 3) Setting the readonly MIP actually inhibits the form author from setting the
> MIP through another bind, because it is illegal to set the same MIP twice.
Yes, thought about this scenario after I did the changes.
 
> So no go. I would wait for the WG to clear this out.
Ok, I haven't found any postings to the WG on this.  I would but I'm uncertain what the ambiguity is.

Perhaps one clarifying point is that settign MIP readonly on a calculate node, or child nodes, has no effect.

Comment 11

[Allan Beaufour]

(In reply to comment #10)
> (In reply to comment #9)
> > 1) As Doron also pointed out (regarding comment 6), I do not think the spec. is
> > clear on this.
> Spec says: 
> 6.1.2 The readonly Property
> "Default Value: false(), unless a calculate property is specified, then
> true()."
> So, what exactly is unclear.  The badness test case just shows a unique cause
> of mixed content.

Oh well, "unclear" was wrong. I think the spec. is wrong in stating the above. It has unintended implications.

> Then later spec says:
> "Inheritance Rules: If any ancestor node evaluates to true, this value is
> treated as true."
> Since calculate causes readonly to be "true", then all children must be
> readonly. Which I see now my changes don't match this.

Which is what I mean by "unintended implications".
 
> > 2) Setting the readonly as an XPath "true()" statement wastes a lot of cycles,
> > when the MDG can be told directly
> Ok, wasn't that brave yet.

Well, I ported/wrote it and I stay out of it too :)

> > 3) Setting the readonly MIP actually inhibits the form author from setting the
> > MIP through another bind, because it is illegal to set the same MIP twice.
> Yes, thought about this scenario after I did the changes.
> 
> > So no go. I would wait for the WG to clear this out.
> Ok, I haven't found any postings to the WG on this.  I would but I'm uncertain
> what the ambiguity is.

Sorry, that was my bad. But there'll be a mail to the WG this week about it.

> Perhaps one clarifying point is that settign MIP readonly on a calculate node,
> or child nodes, has no effect.

Well is it. It's only the default value that is set.. you can override it.

Comment 12

[Allan Beaufour]

David's mail to the WG:
http://lists.w3.org/Archives/Public/www-forms-editor/2006Feb/0007.html

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

RIP xforms