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

It depends on two patches to extensions/xforms.

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

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

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.

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

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

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?

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

: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.)

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

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

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.

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

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?

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 on attachment 174289 [details] [diff] [review] Patch v2 Never mind, I see now. But you still need a better name for the interface.

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.

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.

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

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

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

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.

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

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

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.

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

See also bug 84400 regarding :enabled and :disabled.

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

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.

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

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.

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 :( )

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?)

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

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?

(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 on attachment 181406 [details] [diff] [review] Approach 2 - core, patch 1 So, David... comments?

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.

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

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.

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

Why would we?

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-

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.

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.

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

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

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

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

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

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

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

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.

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.

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

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.

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

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

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

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.

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

(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 on attachment 187107 [details] [diff] [review] With Boris' comments fixed Development will continue on seperate bugs now: bug 84400, bug 302186, and bug 302188

> 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!

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?

Yes, that's what we should do.

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

I agree with dbaron

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

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

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

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 on attachment 199833 [details] [diff] [review] Patch for extensions/xforms Checked in to trunk

Comment on attachment 199833 [details] [diff] [review] Patch for extensions/xforms Requestions a for 1.8rc1, as this is xforms only.

Checked in on 1.8 branch. Created bug 313111 for the proper :read-only/:read-write support.