391905 - XTF doesn't properly handle readonly and readwrite

Status: RESOLVED FIXED

(Core Graveyard :: XTF, defect)

Description

[aaronr]

Attachment: patch

If I use SetIntrinsicState to set the intrinsic state to readwrite, readonly is now always returned due to a core change in nsGenericElement.  Rather than just or'ing in the current XTF intrinsic state with the return from the parent IntrinsicState() to build the return for XTF's IntrinsicState(), there needs to be logic so that only one of readonly/readwrite is returned since they are mutually exclusive.

Comment 1

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

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


> 
>+  NS_ASSERTION(!((aNewState & NS_EVENT_STATE_MOZ_READONLY)
>+               && (aNewState & NS_EVENT_STATE_MOZ_READWRITE)),
>+               "Both READONLY and READWRITE are being set.  Yikes!!!");


I think this could be NS_WARNING.

>+  if (aNewState & NS_EVENT_STATE_MOZ_READONLY) {
>+    aNewState &= ~NS_EVENT_STATE_MOZ_READWRITE;
>+  }

Why this change?

Comment 2

[aaronr]

(In reply to comment #1)
> (From update of attachment 276346 [details] [diff] [review])
> 
> > 
> >+  NS_ASSERTION(!((aNewState & NS_EVENT_STATE_MOZ_READONLY)
> >+               && (aNewState & NS_EVENT_STATE_MOZ_READWRITE)),
> >+               "Both READONLY and READWRITE are being set.  Yikes!!!");
> 
> 
> I think this could be NS_WARNING.

ok

> 
> >+  if (aNewState & NS_EVENT_STATE_MOZ_READONLY) {
> >+    aNewState &= ~NS_EVENT_STATE_MOZ_READWRITE;
> >+  }
> 
> Why this change?
> 

To prevent someone from being able to set both flags at the same time.  Taking the most restrictive of the two as the one to remember.

Comment 3

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

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


>+  NS_ASSERTION(!((aNewState & NS_EVENT_STATE_MOZ_READONLY)
>+               && (aNewState & NS_EVENT_STATE_MOZ_READWRITE)),
>+               "Both READONLY and READWRITE are being set.  Yikes!!!");

Change to NS_WARNING

>+  if (aNewState & NS_EVENT_STATE_MOZ_READONLY) {
>+    aNewState &= ~NS_EVENT_STATE_MOZ_READWRITE;
>+  }

Don't add this. r=me

Comment 4

[aaronr]

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

Tough to think who is best to sr this.  Asking Jonas for sr since he works on XBL which is like a distant cousin :-)

Comment 5

[aaronr]

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

bah, added sr to wrong patch

Comment 6

[aaronr]

Attachment: patch2

patch addresses olli's concerns

Comment 7

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

Comment on attachment 276712 [details] [diff] [review]
patch2

Peterv could sr ;)

Comment 8

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

Peterv, could you please sr this?

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 276712 [details] [diff] [review]
patch2

>Index: content/xtf/src/nsXTFElementWrapper.cpp
>===================================================================

>+  if ((aNewState & NS_EVENT_STATE_MOZ_READONLY) &&
>+      (aNewState & NS_EVENT_STATE_MOZ_READWRITE)) {
>+    NS_WARNING("Both READONLY and READWRITE are being set.  Yikes!!!");
>+  }

NS_WARN_IF_FALSE

I think for now this will do, but note that this doesn't solve the problem for _ENABLED/_DISABLED, _REQUIRED/_OPTIONAL, ... (we'd probably need to allow setting and unsetting of values by having two arguments in SetIntrinsicState or something to fix that).

Comment 10

[aaronr]

(In reply to comment #9)
> (From update of attachment 276712 [details] [diff] [review])
> >Index: content/xtf/src/nsXTFElementWrapper.cpp
> >===================================================================
> 
> >+  if ((aNewState & NS_EVENT_STATE_MOZ_READONLY) &&
> >+      (aNewState & NS_EVENT_STATE_MOZ_READWRITE)) {
> >+    NS_WARNING("Both READONLY and READWRITE are being set.  Yikes!!!");
> >+  }
> 
> NS_WARN_IF_FALSE
> 
> I think for now this will do, but note that this doesn't solve the problem for
> _ENABLED/_DISABLED, _REQUIRED/_OPTIONAL, ... (we'd probably need to allow
> setting and unsetting of values by having two arguments in SetIntrinsicState or
> something to fix that).
> 

Thanks for the sr, peterv.  I guess we might as well fix this 'right' while we are in here.  We need to get this nailed down before FF3 ships.  Did you have a suggestion in mind?

Comment 11

[aaronr]

Attachment: patch

patch fixes the immediate sr comment.  We'll have to fix the more general comment under another bug.  This problem has already slowed down XForms development on the trunk so lets get it out of the way now.

This patch should be safe for the trunk.  It is low risk, just making sure that the desired intrinsic state overrides the inherited state as far as readonly-ness goes.

Comment 12

[aaronr]

checked into trunk