271720 - Use pseudoclasses instead of attributes

Status: RESOLVED FIXED

(Core Graveyard :: XForms, enhancement)

Description

[Allan Beaufour]

We need to support the pseudoclasses specified in the spec (see URL), that is
:disabled, :enabled, :read-only, etc.

Comment 1

[Allan Beaufour]

Attachment: Patch for content/ that implements :enabled and :disabled

It depends on two patches to extensions/xforms.

Comment 2

[Allan Beaufour]

Attachment: Patch to extensions/xforms

This patch patches the patch from bug 265467, os it utilitzes the two
pseudoclasses (only for xforms:input though).

Comment 3

[Allan Beaufour]

Attachment: Document that shows the pseudoclass usage

Comment 4

[Allan Beaufour]

I have only implemented enabled and disabled, so we can discuss how to implement
this. :enabled and :disabled are general pseudoclasses I think, but we also need
to have :read-only, :read-write, etc. that, as far as I know, are XForms specific.

1) How should the interface look like?
As I see it, nsXFormsControl should have a getNodeState() function, that could
return the current node state (read-only, read-write, etc.), or specific
functions  as in the patch: isRelative, isReadonly, isValid, etc. Through use of
nsXFormsNodeState, they could be handled the same way for all controls, either
through a inheritance or through nsXFormsUtils.

2) Should the interface live?
We could either implement it through nsIXFormsControl, og a more generic
pseudoclass interface, if other controls should have the same pseudoclasses?

(Somebody with more "Mozilla-community knowledge" should probably CC the
appropriate people...)

Comment 5

[Hixie (not reading bugmail)]

These pseudo-classes also apply to HTML controls.

Don't we already support :enabled/:disabled and some of the others? I'm sure
I've seen bugs on these pseudo-classes, maybe they haven't yet had patches
checked in.

Comment 6

[Boris Zbarsky [:bzbarsky]]

It's not clear to me how using a single flag manages to implement both enabled
and disabled.... (unless we assume they are mutually exclusive, which they are
not).  What those should be are two separate content states.  I have a partial
patch to that effect in my tree (see discussion in bug 84400 for why it's not
attached anywhere or checked in).

Comment 7

[Hixie (not reading bugmail)]

I would propose using this API to deal with this:
   http://www.whatwg.org/specs/web-controls/current-work/#the-elementui

Comment 8

[Allan Beaufour]

Heh, searched the tree but not Bugzilla, silly me... oh well, that clarifies a
lot :)

To bzbarsky: Absolutely right, we cannot use a single state, as elements could
be in a state where they are neither disabled or enabled.

To ian: We still need :in-range, :out-of-range, :read-only, and :read-write, and
as bzbarsky correctly noted, f.x. enabled and disabled are not mutually
exclusive. Should we not try to be as close to the CSS3-UI as possible?

Comment 9

[Allan Beaufour]

...maybe we should keep CSS3-UI discussion on bug 84400, and only have the
XForms related stuff on this bug?

Comment 10

[Hixie (not reading bugmail)]

:enabled and :disabled are mutually exclusive, but not all elements match both,
true. The spec should be updated (it is indeed out of date); my point was just
that we should use an API along those lines. (That spec doesn't cover :in-range
yet either because it was written before :in-range was added to CSS3 UI.)

Comment 11

[Anne (:annevk)]

(In reply to comment #9)
> ...maybe we should keep CSS3-UI discussion on bug 84400, and only have the
> XForms related stuff on this bug?

I guess there should be separate bugs for other pseudo-classes (and
pseudo-elements) from CSS3 UI. That bug is just for :disabled and :enabled.

Comment 12

[Allan Beaufour]

(In reply to comment #11)
> (In reply to comment #9)
> > ...maybe we should keep CSS3-UI discussion on bug 84400, and only have the
> > XForms related stuff on this bug?
> 
> I guess there should be separate bugs for other pseudo-classes (and
> pseudo-elements) from CSS3 UI. That bug is just for :disabled and :enabled.

Fair enough, how about creating a CSS3 UI tracking bug?

Comment 13

[Anne (:annevk)]

I'm ok with it though I believe most people don't like tracking bugs. You could
mark them as blocking bug 65133 though. Since they are selectors, after all.

Comment 14

[Allan Beaufour]

Attachment: Patch v2

Here's another go at it. I'm not too fond of it though. nsIDOMCSS3UIPseudoClass
is a "nice looking" interface, but I would rather return two bit-vector
PRInt8s. One for the "presence" (applicability?) of the state and the other for
the state itself. That would only need one function call, and most of the
implementation could be done by bit operators.

Arguments for/against, comments to the code, etc? I'm all ears.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 174289 [details] [diff] [review]
Patch v2

This whole tri-state boolean business is all wrong.  :not() matches whenever
its argument doesn't.

The interface name nsIDOMCSS3UIPseudoClass is wrong.  An object implementing it
is not a pseudo-class.	The interface name should be something that it is.

Comment 16

[Allan Beaufour]

Thanks for looking at it right away.

(In reply to comment #15)
> (From update of attachment 174289 [details] [diff] [review] [edit])
> This whole tri-state boolean business is all wrong.  :not() matches whenever
> its argument doesn't.

I'm not sure I'm following you. I'm possibly returning the wrong value in
result. I thought I should return localTrue or localFalse, but I should compare
with them instead?
  result = (aStateMask & NS_EVENT_STATE_READONLY) ||
           (localTrue == (data.mIsReadonly == checkVal));
?

As far as I can see, we need the "tri-state boolean business".

> The interface name nsIDOMCSS3UIPseudoClass is wrong. An object implementing it
> is not a pseudo-class. The interface name should be something that it is.

nsIDOMCSS3UIElement? Should then include the pseudo-elements too I guess?

How about the bit-vector vs. "niceness" issue for the interface?

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sorry, I misread your selector matching code, so never mind the :not() bit.  But
I still don't see the need for the tri-state boolean.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 174289 [details] [diff] [review]
Patch v2

Never mind, I see now.	But you still need a better name for the interface.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

However, for dynamic change handling, I think you're probably better off using
the event state manager and having a bit for each pseudo-class (rather than
pair).  It looks like this patch is very incomplete, though, so it's hard to tell.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, maybenever mind about using the event state manager -- I don't think
you gain much -- except in the case of things that should be supported by a
whole bunch of other types of elements, like :enabled and :disabled, :default,
or :read-only and :read-write.  But I'm not sure if the ESM is the right place
for them, either.
 
But I still don't see the other side of the code for anything other than
NS_EVENT_STATE_RELEVANT.  (And we shouldn't start parsing them until we really
support them.)

Speaking of which, the state names could also probably use some improvement. 
They should probably all (except :default) be NS_EVENT_STATE_PAIR_* since it's a
pair of states represented by a single bit, and you should probably use
NS_EVENT_STATE_PAIR_ENABLED rather than RELEVANT since there's nothing called
:relevant (but there could be in the future).  But the ones that apply elsewhere
probably can't be implemented as a single bit in the long term.

Comment 21

[Allan Beaufour]

(In reply to comment #20)
> Actually, maybenever mind about using the event state manager -- I don't think
> you gain much -- except in the case of things that should be supported by a
> whole bunch of other types of elements, like :enabled and :disabled, :default,
> or :read-only and :read-write.  But I'm not sure if the ESM is the right place
> for them, either.

I'm not the guy to ask. I was hoping you were :)

> But I still don't see the other side of the code for anything other than
> NS_EVENT_STATE_RELEVANT.  (And we shouldn't start parsing them until we really
> support them.)

nsXFormsControlStub::ToggleProperty() is setting attributes for the moment, it
should set the pseudoclasses
http://lxr.mozilla.org/seamonkey/source/extensions/xforms/nsXFormsControlStub.cpp#236

I did not want to code it before the interface was agreed upon.

> Speaking of which, the state names could also probably use some improvement. 
> They should probably all (except :default) be NS_EVENT_STATE_PAIR_* since it's a
> pair of states represented by a single bit, and you should probably use
> NS_EVENT_STATE_PAIR_ENABLED rather than RELEVANT since there's nothing called
> :relevant (but there could be in the future).

Consider it done. I've also renamed defaultState to defaultPseudo.

> But the ones that apply elsewhere probably can't be implemented as a single
> bit in the long term.

Sorry?

Comment 22

[Allan Beaufour]

Attachment: Patch v3

Still tri-state, but the comments I've understood are implemented.

Comment 23

[Allan Beaufour]

Attachment: Full patch for extensions/xforms

Comment 24

[Allan Beaufour]

Attachment: Testdocument

This document shows the usage of the different classes (except
:in-range/:out-of-range).

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You haven't addressed the key comment, which I probably should have made clearer
in comment 20 (although I think I may have in email), which is that this is
fundamentally the wrong design and doesn't extend outside of XForms.  Because of
that, I'm against taking the patch, since once we start parsing a selector we
should be doing it right -- that's what allows authors to write stylesheets that
work cross-browser by depending on the CSS forward-compatible parsing rules.

Doing it right is harder, and I haven't had the time to think through how that
should work yet.

Comment 26

[Allan Beaufour]

(In reply to comment #25)

Hmmm, you are not really helping me here David.

> You haven't addressed the key comment, which I probably should have made clearer
> in comment 20 (although I think I may have in email), which is that this is
> fundamentally the wrong design and doesn't extend outside of XForms.

It's still not clear to me what you see as the problem. Any element can
implement the interface, and elements not implementing it are not influenced.

My best guess from your previous comments is in comment 20:
> But the ones that apply elsewhere probably can't be implemented as a single
> bit in the long term.

Could you elaborate on that?

I'm not using a single bit, I'm using the tri-state. It is my understanding that
any element can either be f.x. :enabled (x)or :disabled, so three states should
be enough.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Two problems:
 (1) we shouldn't parse the selector until we've really implemented it, not just
for XForms
 (2) I think the interface is the wrong API for at least some of the pseudoclasses

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, in more detail:
 * the following need to be implemented for HTML form controls:
    :default
 * the following need to be implemented for all elements (with special meaning
   for HTML form controls):
     :read-only, :read-write
   and it should work correctly when the document is loaded in the editor (or
   when contentEditable is used, if we implement it)

This should be done without increasing the size of any HTML element objects.

Comment 29

[Allan Beaufour]

Attachment: Patch for non-xforms elements too

(In reply to comment #28)
> OK, in more detail:
>  * the following need to be implemented for HTML form controls:
>     :default
>  * the following need to be implemented for all elements (with special
meaning
>    for HTML form controls):
>      :read-only, :read-write

This patch should do that, except for the :default submit button (see below).

>    and it should work correctly when the document is loaded in the editor (or

>    when contentEditable is used, if we implement it)

As we talked about on irc, I'll ignore this for now.

> This should be done without increasing the size of any HTML element objects.

The patch only touches the parser, but to support :default for the default
submit button I suggest that we extract the while loop in:
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLInputElement.cpp#930

into fx. GetDefaultSubmitControl(), expose it, and use it in the CSS parser.
Or?

(Naturally the BEAU_DBG* needs to go away before check in)

Comment 30

[Allan Beaufour]

Attachment: Patch for xforms to handle the modified interface

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See also bug 84400 regarding :enabled and :disabled.

Comment 32

[Allan Beaufour]

(In reply to comment #31)
> See also bug 84400 regarding :enabled and :disabled.

My eyes are wide open :), but there's not really a conclusion is there?

Comment 33

[Hixie (not reading bugmail)]

I took a look at the patch and have some comments:

 * This is lacking clear documentation.

 * It isn't clear to me how this applies to all elements.

 * It's unclear to me how this interacts at a JS level. How am I, from script,
   supposed to mark my <span> or <input> as being :required?

 * It isn't clear how bad states are handled. What if one of the values is set 
   to an invalid value, like 64? (As opposed to -1, 0, 1).

 * I don't understand how this interface interacts with the theming interface 
   (which it must). For example, if I set an element to checked, how does the
   theming interface ('appearance' property implementation) know to change to
   the relevant checked image?

 * It doesn't seem very efficient to be using 8 bits (or however much a short 
   is) just to store what fits in 2 (there are at most four states for these 
   pseudo-classes at the moment). This is especially important if these pseudos
   are relevant for all elements, which they should be.

 * This doesn't handle all the dynamic UI pseudos (in particular, some from
   CSS2.1 and Selectors are missing).

We should be handling the following states, IMHO. Note that some of them have
states that don't match any pseudo-classes yet. We should handle those because
it is possible (maybe even likely) that CSS will support those in the future.

These states apply to the element in all views at the same time:
   :default, not default, not applicable
   :valid, :invalid, not applicable
   :in-range, :out-of-range, not applicable
   :required, :optional, not applicable
   :read-only, :write-only, :read-write, not applicable
   :enabled, :disabled, not applicable
   :checked, unchecked, :indeterminate, not applicable
   selected, unselected, not applicable

These states apply to the element differently in each view:
   :hover, not being hovered
   :active, non-active, not applicable
   open, closed, not applicable

I'm not saying we need to implement all these right away. I'm just saying that
we need to design this so that adding support for them would be mostly trivial.

Comment 34

[Hixie (not reading bugmail)]

er, s/:write-only/write-only/.

Comment 35

[Hixie (not reading bugmail)]

I was asked to clarify my previous comment.

What I think should happen is every element should expose a scriptable interface
that says whether the element matches certain states. The states should be
mutable, so scripts and other parts of Mozilla should be able to say "you are
now the default", "you are now enabled", "the required state no longer applies
to you", etc. The pseudo-class code and the theming code would then use this
interface to establish element states for selectors and appearance painting.

   interface UIStates {
     // for each group of attributes, only one returns true at any one time;
     // setting any one of a group to true resets the others to false, setting
     // one to false when it is true sets its opposite to true
     // :default
     attribute bool isDefaultApplicable;
     attribute bool isNotDefault;
     attribute bool isDefault;
     // :valid, :invalid
     attribute bool isValidApplicable;
     attribute bool isInvalid;
     attribute bool isValid;
     // ...   
   }

These attributes would be exposing some internal values (probably all kept as a
bit mask somewhere -- since this would only be used on very few elements, its
data would probably be hidden away in much the same way that DOMSlots is, maybe
even in DOMSlots). All the states given above and plenty of room for expansion
can be put into a 32 bit structure (2 bits per state).

Then, the XForms, HTML forms, etc, code would simply use this interface to say
"ok I'm now enabled/valid/out-of-range", and this would trigger the recascade,
repainting, etc. Controls defined in XBL would be able to use this interface,
since it is scriptable, to set what states their element should be in as well.

Comment 36

[Allan Beaufour]

Attachment: Patch w/Hixies (approach)

Here's a go at it.

Notes:
* I've ignored the view-dependendent ones for now (:hover, etc.).

* I also handle :default for HTML now (moved functionality from
nsHTMLInputElement to nsIFormControl). But it does not correctly handle
dynamics. I haven't looked at it, but something like inputs and buttons need to
check if they are :default when added/removed, and inform new/old :default.

* There's no support for :enabled/:disabled for HTML elements either. I could
do a simple HasAttr("disabled") but that does not handle if parents set it. Add
a function to nsIFormControl?

What do you say Ian, is this what you meant?

And what of the above needs to get in on this bug?

(I have a patch for extensions/xforms too, but I cannot get to it from my
current location :( )

Comment 37

[Hixie (not reading bugmail)]

I can't really tell if this addresses dbaron's concerns or not. Does this data
survive a stylesheet change? Can it be modified from script? (Does modifying it
update the styles on the fly and so forth?)

Comment 38

[Allan Beaufour]

(In reply to comment #37)
> I can't really tell if this addresses dbaron's concerns or not. Does this data
> survive a stylesheet change? Can it be modified from script? (Does modifying it
> update the styles on the fly and so forth?)

As we talked about on IRC, I've implemented something that is as non-intrusive
as possible for existing elements. So  (XForms, WebForms, etc) elements that
need to support the pseudoclasses will just need to implement the
nsICSSUIElement interface.

As I understand it, your idea is that the interface is supported (and
implemented) by _all (XML) elements in Mozilla_ (I did not understand "every
element" in comment 35 as literally), probably through nsGenericElement.cpp.
That (IMHO) is a greater task.

I'll post the latest patch for my approach, and look at the all-inclusive concept.

But is this the concept that you would like David?

Comment 39

[Allan Beaufour]

Attachment: Approach 1 - core

Comment 40

[Allan Beaufour]

Attachment: Approach 1 - extensions/xforms

Comment 41

[Allan Beaufour]

Attachment: Approach 2 - core, patch 1

Ok, here I go again. Now with something that, hopefully, is closer to what
Hixie actually meant :)

Implementation notes/questions:

* I've temporarily put the interface in nsIContent. This is a problem, as it
should be scriptable. But where should the interface be exposed?

* There's still special handling of HTML button, input, option, and textarea in
nsCSSStyleSheet. I would rather leave existing elements be for now. There
should then probably be a flag that determines whether the nsCSSUIElement
should be created or not -- or the specific controls should override the
GenericElement-implementation.

* Shouldn't it be possible to make the states readonly? It does not make much
sense for the user to change the states on XForms controls.

* Shouldn't it be possible for the content to be advised about state changes,
or? F.x. if a user does |element.pseudoStates.isReadOnly| = 1 through script.

* I think nsICSSUIElement needs a name-change. nsICSSPseudoClassState? Or?

Comment 42

[Allan Beaufour]

Attachment: Approach 2 - extensions/xforms, patch 1

Comment 43

[Allan Beaufour]

(In reply to comment #42)
> Created an attachment (id=181407) [edit]
> Approach 2 - extensions/xforms, patch 1

There's a problem with this and the above testcase. My guess is that it's
because the visual content of the f.x. xforms:input control is a html:input and
that overrules the styling set on the xforms control.

Comment 44

[Allan Beaufour]

Comment on attachment 181406 [details] [diff] [review]
Approach 2 - core, patch 1

So, David... comments?

Comment 45

[Boris Zbarsky [:bzbarsky]]

So I'm trying to understand the setup here...

We have a scriptable interface, but the only getter for it is noscript... why is
it scriptable?

And I assume this is basically leaving mozilla-extension pseudo-classes out to
be handled by some other mechanism?  I'm sort of working on :-moz-broken and
:-moz-blocked pseudo-classes for images, and I'm wondering a little as to how
they'd fit into this scheme.

Comment 46

[Allan Beaufour]

(In reply to comment #45)
> So I'm trying to understand the setup here...
> 
> We have a scriptable interface, but the only getter for it is noscript... why is
> it scriptable?

It's scriptable because Hixie wanted it. But, getting to it is not possible
through script now. See comment 41. I need somewhere to expose it, and hoped for
some feedback, which I have yet to receive.

> And I assume this is basically leaving mozilla-extension pseudo-classes out to
> be handled by some other mechanism?  I'm sort of working on :-moz-broken and
> :-moz-blocked pseudo-classes for images, and I'm wondering a little as to how
> they'd fit into this scheme.

I've tried to minimize what I'm handling, to ease the process .... it's not
working, maybe I should just move to "solve the universe and everything" :)

Mozilla-extension pseudo-classes could be included too I guess. Dunno, I do not
know too much about them.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  Some comments on the patch, then.  Not sure whether these affect the
basic design...

HasPseudoStates should just return a PRBool.  No need for an out param.

> aTextControlsFound++;

really doesn't look like what you want to be doing.

> +    document->ContentStatesChanged(mContent, nsnull, NS_EVENT_STATE_UNSPECIFIED);

That's wrong.  It won't handle dynamic state changes correctly.  Not that the
code in nsCSSStyleSheet is checking the event state mask, but that's wrong too,
for the same reasons.  Let me know if this comment doesn't make sense and I'll
try to write up an explanation; it's likely to be a little long....

This is why the ESM got mentioned earlier, and why I raised the question I did
about how what I'm doing would mesh with this code.

I'm not sure why the new pseudos are getting added to IsEventPseudo().  The
things in IsEventPseudo are the ones we apply various quirks hacks to.  For
XForms this is probably a non-issue, but I think that :default for HTML
<options> should work even in quirks mode.

Comment 48

[Hixie (not reading bugmail)]

yeah, you'd want isBroken and isBlocked flags for that. That would fit here very
well, IMHO.

Comment 49

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why would we?

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 181406 [details] [diff] [review]
Approach 2 - core, patch 1

This really shouldn't be hard to do without increasing the size of our core
element classes.  I'm not sure what the use case for scriptability is or why
Ian is pushing for it, but what he has done is talk you into ignoring one of my
original review comments.  review-

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, maybe the new entry in nsDOMSlots isn't needed as often as I thought at
first glance, but perhaps you should comment (in the code) on what conditions
cause it to be created.

I'm still not sure what the use case for scriptability is or how much complexity
that's adding to the patch or whether this patch even meets that use case.

Comment 52

[Hixie (not reading bugmail)]

The use case for the scriptability is being able to make accessible custom
widgets. That hasn't changed since the first time it was proposed all those many
years ago. Are you saying we shouldn't do this? You don't seen to have commented
on it at all, so it is unclear what your position is here.

The elements would almost all be at their default values, it's only the few
elements that actually end up needing to match these pseudos that would need to
get the memory allocated here.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Actually, the patch increases by 4 bytes all elements with DOMSlots.  That means
anything that has:

  Had the childNodes getter called on it.
  Has inline style.
  Has has node.attributes accessed.
  Isn't XUL and is in an XBL binding.
  Is a XUL element with controllers.

Granted, the first three of those cases already use a fair amount of memory on
their own, which is why adding things to the DOMSlots is generally ok...  If we
want to avoid doing that, we could possibly use the property storage mechanism
and use yet another bit from bits-or-slots to make it fast...

Comment 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So does what this patch does actually meet that use case?  I suspect it doesn't.

Comment 55

[Hixie (not reading bugmail)]

I have no idea, I haven't looked at the most recent patch (and my knowledge of
internal Gecko things is definitely not enough for me to be able to tell whether
something like this would work or not).

Comment 56

[Allan Beaufour]

(In reply to comment #47)
> HasPseudoStates should just return a PRBool.  No need for an out param.

The reason for that is that it should be scriptable at some time.
 
> > aTextControlsFound++;
> 
> really doesn't look like what you want to be doing.

Well, I just expose existing code.

> > +    document->ContentStatesChanged(mContent, nsnull,
NS_EVENT_STATE_UNSPECIFIED);
> 
> That's wrong.  It won't handle dynamic state changes correctly.  Not that the
> code in nsCSSStyleSheet is checking the event state mask, but that's wrong too,
> for the same reasons.  Let me know if this comment doesn't make sense and I'll
> try to write up an explanation; it's likely to be a little long....

Got the explanation. I trashed the specific NS_EVENT_STATE-variables, which was
a mistake. I'll include those again.

Comment 57

[Allan Beaufour]

(In reply to comment #54)
> So does what this patch does actually meet that use case?  I suspect it doesn't.

Well, it's not scriptable yet, so no :) But as I noted in comment 41, where
should I expose it?

Comment 58

[Boris Zbarsky [:bzbarsky]]

> The reason for that is that it should be scriptable at some time.

Scriptable and moved off nsIContent?  Or scriptable via some other interface and
calling the nsIContent method internally?  In the former case, leaving it this
way might be ok (less changes to callers), but in the latter you really do want
it to just return PRBool.

> Well, I just expose existing code.

Existing code counts the text controls by incrementing an integer when a text
control is found.  Your code increments a pointer when a text control is found
and always reports 0 text controls.  Not quite the same.  Fwiw,
regression-testing on the testcases that form code was written to fix is a good
idea once this patch gets close enough to landing.

Comment 59

[Allan Beaufour]

(In reply to comment #58)
> > Well, I just expose existing code.
> 
> Existing code counts the text controls by incrementing an integer when a text
> control is found.  Your code increments a pointer when a text control is found
> and always reports 0 text controls.

Doh! Stupid me.

Comment 60

[Allan Beaufour]

Attachment: Patch that uses IntrinsicState()

Here's a patch that implements :enabled/:disabled, :read-only/:read-write, and
:default for the standard HTML form controls using IntrinsicState().

:enabled/:disabled just checks the attribute disabled, should I check focusable
instead? (bug 84400)

The only tricky part is handling :default for submit buttons. I've added
mDefaultSubmitElement in nsHTMLFormElement, which is being maintained on add,
remove and type changes for buttons and inputs. I've added two functions to
nsIForm to get the default submit element and reset it (ie. traverse the form
elements and find the default).

nsHTMLInputElement::MaybeSubmitForm() is changed too, so that it uses
mDefaultSubmitElement.

Comment 61

[Allan Beaufour]

Attachment: Testcase for :default

Comment 62

[Allan Beaufour]

Attachment: Testcase for :read-only/:read-write

Comment 63

[Boris Zbarsky [:bzbarsky]]

It'll take me a few days to get to this, Allan...  Also, if I don't manage to do
it by the 19th, I won't be able to until July.

Comment 64

[Boris Zbarsky [:bzbarsky]]

ccing content owners in case they have thoughts about some of the review
comments coming up.

Comment 65

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 185943 [details] [diff] [review]
Patch that uses IntrinsicState()

>Index: layout/style/nsCSSStyleSheet.cpp
>@@ -3120,16 +3120,49 @@ static PRBool SelectorMatches(RuleProces

>+    else if (nsCSSPseudoClasses::defaultPseudo == pseudoClass->mAtom) {

It might be a good idea to order the checks here such that the pseudos we
expect to see most often are first.

> PRBool IsStateSelector(nsCSSSelector& aSelector)

Same in this method.  I'm pretty sure :hover should come before :optional, say.

>Index: content/html/content/src/nsGenericHTMLElement.h

>+  virtual PRInt32 IntrinsicState() const;

There's state common to all HTML but not other document languages?

>+   * Called when an attribute has just been changed

This needs a lot more documentation (eg, what does a null aValue mean)?

>Index: content/html/content/src/nsGenericHTMLElement.cpp

>+nsGenericHTMLElement::IntrinsicState() const

>+  // Every HTML element is read only per default

Isn't this generally true for non-HTML?  And can't having an XBL binding on
HTML affect this?  Etc, etc.  That is, shouldn't this just live in
nsGenericElement or even in the default state returned by nsIContent?

>+nsGenericHTMLFormElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                                    PRBool aNotify)

>+  // SetAttr() removes and adds the control when @type is set, so we only need

What's @type?

>+  if (mForm && aName == nsHTMLAtoms::type) {

Shouldn't this check that aNameSpaceID == None?

>+      mForm->ResetDefaultSubmitElement();

This looks wrong; it'll notify even when aNotify is false.

>+nsGenericHTMLFormElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                                       const nsAString* aValue, PRBool aNotify)
>+{
>+  if (aName == nsHTMLAtoms::disabled) {

Check namespace, please.

>Index: content/html/content/src/nsHTMLFormElement.cpp

>+  /** The default submit element -- WEAK */
>+  nsIFormControl* mDefaultSubmitElement;

How do we clear this pointer if mDefaultSubmitElement dies?  That is, who's
responsible for maintaining this pointer in a sane way?  More to the point, is
there a reason not to make this a strong pointer?  Do form controls hold strong
pointers to the form?

>@@ -1195,16 +1201,39 @@ nsHTMLFormElement::AddElement(nsIFormCon

>+    } else if (CompareFormControlPosition(aChild, mDefaultSubmitElement) < 0) {
>+      // New element is positioned before the current default

Do we really want to be doing this during parsing?  What's preventing that from
happening here?

> nsHTMLFormElement::RemoveElement(nsIFormControl* aChild) 

>+  if (aChild == mDefaultSubmitElement) {
>+    // We are removing the default submit, find the new default
>+    rv = ResetDefaultSubmitElement();

Again, won't this always notify, even if the remove had aNotify false?	That
looks wrong.

>Index: content/html/content/src/nsHTMLInputElement.cpp

>+  if (aName == nsHTMLAtoms::readonly) {

Namespace id!

Also, readonly only applies to text inputs, no?  We don't want <input
type="checkbox" readonly="readonly"> to match :readonly, I don't think.  Which
means we need to check the type here and possible notify a content state change
when the type attr changes...

> nsHTMLInputElement::IntrinsicState() const
>+    // default checked element

<input type="text" checked="checked"> might not want to match :default...  Need
a type check here.

Also, for multiple radios all in the same group that have a checked attribute,
it's not clear what should happen (should they all match :default, or only the
ones that actually end up checked by default?).

>+    // Nasty hack because we need to call an interface method, and one that
>+    // toggles some of our hidden internal state at that!

Does GetDefaultChecked really toggle internal state?  I don't see where... 
|mutable| wouldn't help here, therefore.

>+  // :read-only/:read-write
>+  PRBool roState;
>+  // Same nasty hack as above
>+  NS_CONST_CAST(nsHTMLInputElement*, this)->GetReadOnly(&roState);

Again, this doesn't apply to all inputs, so this code is wrong.

>Index: content/html/content/src/nsHTMLOptionElement.cpp

>+  // Same nasty hack as above
>+  NS_CONST_CAST(nsHTMLOptionElement*, this)->GetDefaultSelected(&selected);

GetDefaultSelected doesn't change any internal state....

>Index: content/html/content/src/nsHTMLTextAreaElement.cpp
>+  virtual nsresult SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                           nsIAtom* aPrefix, const nsAString& aValue,
>+                           PRBool aNotify)
>+  {
>+    nsresult rv = nsGenericHTMLFormElement::SetAttr(aNameSpaceID, aName,
>+                                                    aPrefix, aValue, aNotify);
>+
>+    AfterSetAttr(aNameSpaceID, aName, &aValue, aNotify);

This will call AfterSetAttr twice (once in nsGenericHTMLFormElement::SetAttr
and once here), no?  Other subclasses of nsHTMLFormElement have the same
problem, I think.  We should only be calling that once.  Given the changes
you're making, I suspect that once should be in the superclass and the subclass
AfterSetAttr methods need to call the superclass's method.

>+  virtual nsresult UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aAttribute,

Same issue here.

>+  /**
>+   * Called when an attribute has just been changed
>+   */
>+  void AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                    const nsAString* aValue, PRBool aNotify);

That's virtual, right (given that the superclass's is)?

>+nsHTMLTextAreaElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,

>+  if (aName == nsHTMLAtoms::readonly) {
>+    nsIDocument* document = GetCurrentDoc();
>+    if (document) {
>+      mozAutoDocUpdate(document, UPDATE_CONTENT_STATE, PR_TRUE);
>+      document->ContentStatesChanged(this, nsnull, NS_EVENT_STATE_READONLY |
>+                                                   NS_EVENT_STATE_READWRITE);

This always notifies.  That's not desirable.

>+nsHTMLTextAreaElement::IntrinsicState() const
>+{
>+  // :checked

Huh?

>+  // Nasty hack because we need to call an interface method, and one that
>+  // toggles some of our hidden internal state at that!  Would that we could
>+  // use |mutable|.

GetReadOnly doesn't change internal state.

Perhaps we should have non-COM versions of all these methods for internal
use...

>Index: content/html/content/public/nsIForm.h

>+   * Get the default submit element

Can return null, right?

>+  NS_IMETHOD GetDefaultSubmitElement(nsIFormControl** aElement) const = 0;

Why not just have this return the nsIFormControl*?

For that matter, would using nsIContent* here make more sense?	You always seem
to QI the result to nsIContent...

Given the size of the patch and the complexity of getting some of the form
controls stuff right, it might be worthwhile to break this up into several
patches, but it's your call.

Comment 66

[Allan Beaufour]

Attachment: With Boris' comments fixed

(In reply to comment #65)
> (From update of attachment 185943 [details] [diff] [review] [edit])
> >Index: layout/style/nsCSSStyleSheet.cpp
> >@@ -3120,16 +3120,49 @@ static PRBool SelectorMatches(RuleProces
> 
> >+	else if (nsCSSPseudoClasses::defaultPseudo == pseudoClass->mAtom) {
> 
> It might be a good idea to order the checks here such that the pseudos we
> expect to see most often are first.

Good point, but I might need some help in determining the optimal order.
 
> > PRBool IsStateSelector(nsCSSSelector& aSelector)
> 
> Same in this method.	I'm pretty sure :hover should come before :optional,
say.

Yes. I've put the new ones in the bottom for a start.
 
> >Index: content/html/content/src/nsGenericHTMLElement.h
> 
> >+  virtual PRInt32 IntrinsicState() const;
> 
> There's state common to all HTML but not other document languages?

Argh, yeah, I did not re-read all the comments on the bug, thought I could
remember what David had written... that was obviously wrong (comment 28).

> >+   * Called when an attribute has just been changed
>
> This needs a lot more documentation (eg, what does a null aValue mean)?

Lousy excuse: I just copied it from nsHTMLInputElement.cpp. I'll add dox.
 
> >Index: content/html/content/src/nsGenericHTMLElement.cpp
> 
> >+nsGenericHTMLElement::IntrinsicState() const
> 
> >+  // Every HTML element is read only per default
> 
> Isn't this generally true for non-HTML?  And can't having an XBL binding on
> HTML affect this?  Etc, etc.	That is, shouldn't this just live in
> nsGenericElement or even in the default state returned by nsIContent?

I guess the default state could be READONLY instead of 0. But XUL should
probably implement IntrinsicState() too then, but that should really be a
separate bug then (at least because I'm no XUL-guru).
 
> >+nsGenericHTMLFormElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
> >+					PRBool aNotify)
> 
> >+  // SetAttr() removes and adds the control when @type is set, so we only
need
> 
> What's @type?

Attribute |type|, XPath, sorry.
 
> >+  if (mForm && aName == nsHTMLAtoms::type) {
> 
> Shouldn't this check that aNameSpaceID == None?

Yes, all the functions should check the namespace. I'll fix that.
 
> >Index: content/html/content/src/nsHTMLFormElement.cpp
> 
> >+  /** The default submit element -- WEAK */
> >+  nsIFormControl* mDefaultSubmitElement;
> 
> How do we clear this pointer if mDefaultSubmitElement dies?  That is, who's
> responsible for maintaining this pointer in a sane way?

The form control is responsible for adding and removing itself from the form,
and I check for the removal in RemoveElement(), so it should always be valid.
That's why I chose the weak pointer.

>  More to the point, is there a reason not to make this a strong pointer?

Hmmm, probably not, but I'm not exactly "Mr. content/" :-)

>  Do form controls hold strong pointers to the form?

No form controls have a weak pointer to the form.

> >@@ -1195,16 +1201,39 @@ nsHTMLFormElement::AddElement(nsIFormCon
> 
> >+	} else if (CompareFormControlPosition(aChild, mDefaultSubmitElement) <
0) {
> >+	  // New element is positioned before the current default
> 
> Do we really want to be doing this during parsing?  What's preventing that
from
> happening here?

I thought about the same, but obviously forgot to handle it :(

I guess parsing inserts elements in document order, so I just (re)use the check
for whether it's being inserted in the end of the form.
 
> >Index: content/html/content/src/nsHTMLInputElement.cpp
> 
> >+  if (aName == nsHTMLAtoms::readonly) {
> 
> Namespace id!

It's checked at the entry to the function, but I missed checking the namespace
other places ... I'll add it there.

> Also, readonly only applies to text inputs, no?  We don't want <input
> type="checkbox" readonly="readonly"> to match :readonly, I don't think. 
Which
> means we need to check the type here and possible notify a content state
change
> when the type attr changes...

I agree. I'll add the type checks. The type change should work without changes,
as form elements are removed and re-added to the form when they change type.

> Also, for multiple radios all in the same group that have a checked
attribute,
> it's not clear what should happen (should they all match :default, or only
the
> ones that actually end up checked by default?).

Heh, that's a good one. I would say it only match the one that actually ends up
getting checked.

> >Index: content/html/content/public/nsIForm.h
> 
> >+   * Get the default submit element
> 
> Can return null, right?

Yes, add a note about that you mean?
 
> >+  NS_IMETHOD GetDefaultSubmitElement(nsIFormControl** aElement) const = 0;
> 
> Why not just have this return the nsIFormControl*?

Well, I could, but all other functions in the interface obey the XPCOM
interface rules.
 
> For that matter, would using nsIContent* here make more sense?	You
always seem
> to QI the result to nsIContent...

The only place I QI to nsIContent is in nsHTMLInputElement::MaybeSubmitForm(),
all other places use nsIFormControl.

> Given the size of the patch and the complexity of getting some of the form
> controls stuff right, it might be worthwhile to break this up into several
> patches, but it's your call.

Let's take one more round... then I may split it.

Here's a summary of the changes in this patch:
* added namespace checks when I look at attributes
* ResetDefaultSubmitElement() now takes an aNotify parameter.
* added control type checks in nsHTMLInputElement
* implemented BeforeSetAttr() and AfterSetAttr() as virtuals in
nsGenericHTMLFormElement (funtionality taken from nsHTMLInputElement).
* made all elements (nsIContent) match :read-only per default.

Open issues:
* multiple checked="checked" on radio controls all match :default, I guess only
the one "true" default should match?
* SetForm() sends notifications all the time now... when should it actually
send them?
* optimal ordering of if-statements in nsCSSStyleSheet.cpp
* make all elements (nsIContent) match :read-only per default.

Comment 67

[Doron Rosenberg (IBM)]

Allan - is this patch the latest work that needs review, or do you have anything
newer in your trees?

Comment 68

[Allan Beaufour]

(In reply to comment #67)
> Allan - is this patch the latest work that needs review, or do you have anything
> newer in your trees?

Attachment 187107 [details] [diff] is the newest.

Comment 69

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 187107 [details] [diff] [review]
With Boris' comments fixed

>Index: content/base/public/nsIContent.h
>   virtual PRInt32 IntrinsicState() const
>   {
>-    return 0;
>+    // All elements are read only per default
>+    return NS_EVENT_STATE_READONLY;

So... I've been thinking about this more, and we have a slight problem here. 
For example, in an editable document, this default should be READWRITE, I would
think.	And I would think that we'd want XUL text inputs to match
readwrite/readonly accordingly; I don't see how this patch manages that.

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>@@ -3152,17 +3152,18 @@ nsGenericHTMLFormElement::SetForm(nsIDOM
>+    // XXX (when) should SetForm() send notifications?
>+    mForm->RemoveElement(this, PR_TRUE);

Please sort this out (look at the callers of SetForm and figure out what
they're doing, etc).  Notifying unnecessarily would be a major perf issue (eg
would force flushing out the content sink for no reason from the callsite at
<http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLConten
tSink.cpp#842>; that same callsite could also leave to broken behavior on the
part of the content sink).  It's entirely possible that SetForm() needs an
aNotify argument of its own.

> nsGenericHTMLFormElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* 

Fwiw, a -w diff in addition to a normal one would have helped a lot here... :(

>+nsGenericHTMLFormElement:: BeforeSetAttr(PRInt32 aNameSpaceID, 

What's with that extra space?

>+{
>+}

And why does this method exist?  If it's only used by subclasses, then perhaps
only they should declare it?

>+nsGenericHTMLFormElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                                       const nsAString* aValue, 
>+      mozAutoDocUpdate(document, UPDATE_CONTENT_STATE, aNotify);
>+      document->ContentStatesChanged(this, nsnull, NS_EVENT_STATE_DISABLED |

Why is this notifying when aNotify is false?

>Index: content/html/content/src/nsHTMLFormElement.cpp
>+  NS_IMETHOD GetDefaultSubmitElement(nsIFormControl** aElement) const;

Again, I'd like this to be an NS_IMETHOD_(nsIFormControl*).  There's no reason
for the refcounting here.  I just checked with jst, and he agrees on this.

@@ -1195,50 +1203,137 @@ nsHTMLFormElement::AddElement(nsIFormCon

>+        mozAutoDocUpdate(document, UPDATE_CONTENT_STATE, PR_TRUE);

Ouch.  This will notify in the middle of sink notifications.  It'll also notify
in the middle of attr changes that have aNotify set to false.  Please fix this
to only notify when it should, ok?

Note that notifying in the middle of sink notifications may well be ok here --
we at least want to notify that oldElement is not the default anymore, since it
might have a frame.  But I really don't think we want to be doing this if
aNotify is false on an attr change...

>+        NS_ASSERTION(oldElement && newElement,
>+                     "Form control not implementing nsIContent?!\n");

No need for the \n.

>+nsHTMLFormElement::ResetDefaultSubmitElement(PRBool aNotify)

>+      if (type == NS_FORM_INPUT_SUBMIT ||
>+          type == NS_FORM_BUTTON_SUBMIT ||
>+          type == NS_FORM_INPUT_IMAGE) {

Maybe we should have an nsIFormControl function that does this?  followup bug,
perhaps?

>+  // Inform about change

Even if aNotify is false?  That can't be right...

>+  if (oldDefault || mDefaultSubmitElement) {
>+    nsIDocument* document = GetCurrentDoc();
>+    if (document) {

I really wish we could avoid notifying here for cases when the default control
is being removed from the document altogether (since in that case the fact that
its state changed really doesn't matter)...  But I bet we get into here before
oldDefault's document is nulled out in that case, right?

>+      nsCOMPtr<nsIContent> newElement(do_QueryInterface(mDefaultSubmitElement));
>+      NS_ASSERTION(oldElement && newElement,
>+                   "Form control not implementing nsIContent?!\n");

Why can't newElement be null here?  I don't think you can make this
assertion....

>+nsHTMLFormElement::GetDefaultSubmitElement(nsIFormControl** aElement) const
>+{
>+  NS_ENSURE_ARG(aElement);

For future reference, don't null-check out params, please.

>Index: content/html/content/src/nsHTMLInputElement.cpp
> nsHTMLInputElement::BeforeSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>                                   const nsAString* aValue,
>                                   PRBool aNotify)
> {
>+  nsGenericHTMLFormElement::BeforeSetAttr(aNameSpaceID, aName, 


> nsHTMLInputElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>                                  const nsAString* aValue,
>                                  PRBool aNotify)
> {
>+  nsGenericHTMLFormElement::AfterSetAttr(aNameSpaceID, aName, aValue, aNotify);

Should we perhaps call the superclass AfterSetAttr after our own?  Or does it
not matter?  Perhaps the function decl in the superclass should document what
assumptions are made about call order, if any?

>@@ -536,16 +510,29 @@ nsHTMLInputElement::AfterSetAttr(PRInt32

>+      document->ContentStatesChanged(this, nsnull, NS_EVENT_STATE_READONLY |
>+                                                   NS_EVENT_STATE_READWRITE);

Even if aNotify is false?

>@@ -2462,20 +2446,69 @@ nsHTMLInputElement::DoneCreatingElement(
> nsHTMLInputElement::IntrinsicState() const
>+  if (mForm &&
>+      (mType == NS_FORM_INPUT_SUBMIT ||
>+       mType == NS_FORM_INPUT_IMAGE)) {

This should be in an else, no?	If we already hit the first if and it tested
true, this one is guaranteed to test false.

>+    if (defControl && defControl == this) {

No need for defControl null-check here.

I'm not seeing any provisions for toggling the :default state for
radio/checkbox when the relevant attr changes...

>Index: content/html/content/src/nsHTMLOptionElement.cpp

I don't see any provisions for toggling the :default state when the "selected"
attr changes.

This is where those testcases I talked about would come in useful...

>Index: content/html/content/src/nsHTMLTextAreaElement.cpp

>+nsHTMLTextAreaElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,

>+      document->ContentStatesChanged(this, nsnull, NS_EVENT_STATE_READONLY |
>+                                                   NS_EVENT_STATE_READWRITE);

Event if aNotify is false?

I really think this would work best split up into several patches, with some QA
involvement to test each patch thoroughly if we're going to take this for 1.8
at this late stage...

Comment 70

[Anne (:annevk)]

Web Forms 2 defines that :default should only match the submit button that will
be used when the user presses enter. Web Forms 2 does not define it for other
elements and does not intend to. I think we should follow Web Forms 2 here.

Comment 71

[Allan Beaufour]

(In reply to comment #70)
> Web Forms 2 defines that :default should only match the submit button that will
> be used when the user presses enter. Web Forms 2 does not define it for other
> elements and does not intend to. I think we should follow Web Forms 2 here.

Well, it certainly makes my life a lot easier.

Comment 72

[Allan Beaufour]

(In reply to comment #69)
> I really think this would work best split up into several patches, with some QA
> involvement to test each patch thoroughly if we're going to take this for 1.8
> at this late stage...

I'll split the patch.

Comment 73

[Allan Beaufour]

Comment on attachment 187107 [details] [diff] [review]
With Boris' comments fixed

Development will continue on seperate bugs now: bug 84400, bug 302186, and bug
302188

Comment 74

[Boris Zbarsky [:bzbarsky]]

> I think we should follow Web Forms 2 here.

CSS3 UI pretty clearly defines default to match the default option in a
<select>, and default radio in a radio group.  Per the Web Forms 2 description
of <input type="checkbox">, it also defines it to match a <input type="checkbox"
checked="checked">.

So luckily, we can follow both Web Forms 2 and CSS3 UI here, since they don't
actually conflict!

Comment 75

[Doron Rosenberg (IBM)]

So regarding valid/invalid and in-range/out-of-range

Those don't really work with regular html forms, since there is no way for an
html form currently in Mozilla to specify validity.  Web Forms does, but we
don't support that right now.

So should we just have valid/in-range always be true and invalid/out-of-range
false for HTML elements?  And leave an XXX: for whoever implements Web Forms in
the future?

Comment 76

[Boris Zbarsky [:bzbarsky]]

Yes, that's what we should do.

Comment 77

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #75)
> So should we just have valid/in-range always be true and invalid/out-of-range
> false for HTML elements?  And leave an XXX: for whoever implements Web Forms in
> the future?

I think neither should match HTML form elements unless we implement some notion
of validity or being within range.

Comment 78

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree with dbaron

Comment 79

[Anne (:annevk)]

We do support the MAXLENGTH attribute, but it might not be worth it to support
:valid, :invalid just for that.

Comment 80

[aaronr]

Allan, should this bug depend on
https://bugzilla.mozilla.org/show_bug.cgi?id=302186 ?

Comment 81

[Allan Beaufour]

(In reply to comment #80)
> Allan, should this bug depend on
> https://bugzilla.mozilla.org/show_bug.cgi?id=302186 ?

No, so please stop adding it as a dependency... XForms does not use :default for
anything.

Comment 82

[Allan Beaufour]

Attachment: Patch for extensions/xforms

Comment 83

[aaronr]

looks like this patch may have missed the attribute stuff in
nsXFormsControlStubBase::ResetBoundNode.  Javier is also going to use the
"disabled" attribute with his upload control implementation.  So if that patch
goes in before this patch does, please keep that in mind.

Comment 84

[Allan Beaufour]

Comment on attachment 199833 [details] [diff] [review]
Patch for extensions/xforms

Checked in to trunk

Comment 85

[Allan Beaufour]

Comment on attachment 199833 [details] [diff] [review]
Patch for extensions/xforms

Requestions a for 1.8rc1, as this is xforms only.

Comment 86

[Allan Beaufour]

Checked in on 1.8 branch.

Created bug 313111 for the proper :read-only/:read-write support.