Closed Bug 379178 Opened 14 years ago Closed 14 years ago

Common base class for style-able elements (HTML, SVG, MathML)

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: domob, Assigned: domob)

References

Details

Attachments

(2 files, 6 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.3) Gecko/20070425 BonEcho/2.0.0.3
Build Identifier: 

Currently, support for the style/id/class attributes and their CSS-meaning is implemented both in nsGenericHTMLElement and nsSVGElement, with significant code-duplication.  To implement style-ablility for MathML, too, we would require to duplicate this code just another time.

I'd rather see this code collected and shared by a common base for such CSS-style-able elements, called for instance nsStyleAbleElement, with overrides for GetClasses(), the InlineStyleRule-methods and ParseAttribute to parse style/class-attribute.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
I'd like to get this discussed and decided upon before I continue working on MathML, bug #276267.  If you agree we should do this, I'll file the nearby straight-forward patch.
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
I think this is a good idea.  Not sure what a good name is for the class, though.
Assignee: nobody → domob
Blocks: 276267
Status: UNCONFIRMED → NEW
Component: Layout → DOM
Ever confirmed: true
[mid-air collision, bz is way too fast! here was my comment]

You don't need further comments. Those who have a stake in it and care about it are those who expressed interest and support in the other bug. Up to you to drive it from here. I should mention however to remember that the rule of thumb on this large code base is that it doesn't mean that any patch will go in. The early thumb-up is usually on the principle/idea. Implementation details may compel people to revisit their stance. So don't take things personnally if this was to happen here, although I don't anticipate that to happen on this particular bug, it is well defined.

I wonder about the name... nsStyleAbleElement is my least preferred choice. Other choices might be nsStyleableElement or actually nsStyledElement -- there used to be nsIStyledContent in the tree for something similar to this, see: 
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsXMLElement.h&branch=&root=/cvsroot&subdir=mozilla/content/xml/content/src&command=DIFF_FRAMESET&rev1=1.32&rev2=1.33
Severity: minor → normal
Component: DOM → General
nsStyledElement is not bad, yeah.
Ok, thanks to you both.  nsStyledElement sounds good to me, too.  I'm going to file a patch here ASAP, which will most probably be today or tomorrow; and of course I'd be happy then for your comments.

Just one thing left:  I'm not sure whether to sub-class nsGenericElement or nsXMLElement -- while the latter seems to be somewhat too specific (and it would introduce nsXMLElement as parent of nsGenericHTMLElement), this wouldn't include XLink support.  But I suppose this is indeed not what we want, at least for this bug; right?  (I don't really know anything about how XLink is supported currently and how it should be supported)
So I'd sub-class nsGenericElement, but awaiting your comments (however, this can be changed without much effort still when a patch is there).
Hmm.  Does MathML want the XLink stuff?

If not, I'd subclass nsGenericElement (esp. because nsSVGElement is not a subclass of nsXMLElement either).  The link stuff is the only thing nsXMLElement gives you.

I'm not sure you want to make nsGenericHTMLElement inherit from nsXMLElement without a pretty careful look at possible perf impact, etc.  So I'm rather hoping MathML doesn't need the XLink stuff, in fact.

Yes, but those xlink attributes are being used on elements in the OpenMath namespace, not the MathML one... or am I missing something?
I just a look at the MathML DTD to double-check, and saw that it is allowed on MathML elements themselves:

# Attributes shared by all elements  ..........................
MATHML.Common.attrib =
  attribute xlink:href { text }?,
  attribute xlink:type { text }?,
  attribute class { text }?,
  attribute style { text }?,
  attribute id { xsd:ID }?,
  attribute xref { xsd:IDREF }?,
  attribute other { text }?
Yes, and currently MathML elements are nsXMLElement-instances in the content-tree; however, because of neither nsGenericHTMLElement nor nsSVGElement are nsXMLElement-subclasses, I'd use nsGenericElement as base, too.

And of course, nsStyledElement has nothing to do with XLink -- how to implement it in MathML should go to bug #276267 rather than this one, I believe.
Status: NEW → ASSIGNED
Here's a simple test-case to verify that styling with style/id/class works for HTML, SVG and MathML; for the last one, it is ok to fail here, as this is bug #276267.
For what it's worth, if you want to make the test in reftest format (see http://developer.mozilla.org/en/docs/Creating_reftest-based_unit_tests) it could just be checked in with the patch.  The reference would probably style based on tag name or something...
Attached patch First patch, breaks SVG (obsolete) — Splinter Review
This is my first proposed patch, or at least, what I had thought of to do; however, while it seems to work for HTML, it breaks SVG.              I found that there are GetStyle-methods both for nsGenericHTMLElement and nsSVGStylableElement, which do not come from the same interface or source to my understanding. This might be the reason for the failure of this patch in SVG, might it?

This GetStyle-method is implemented similiarly in HTML and SVG, and for SVG there's already a comment in mentioning that code could be shared between SVG and HTML. I had simply moved it to nsStyledElement -- if it wasn't declared non-virtual and weren't coming from different interfaces (nsIDOMNSHTMLElement and nsIDOMSVGStylableElement, IIRC).

Hope anyone could look through my patch and tell whether I got the main-part right; although HTML still seems to work, I'm nearly sure I must have done something wrong ;)

Reftest -- sounds really neat and I could of course provide such a reftest, but where to put it?
There doesn't seem to be any GetStyle stuff in that patch...

The reftest could just go into layout/reftests/bugs for this bug.
Yes, that's it (at least I believe so) -- my point was that those methods were non-virtual, coming from different interfaces/superclasses; but that shouldn't matter that much, I think, I'll try.

That reftest-stuff sounds good -- I'll include it there in my next patch, hopefully getting SVG to work ;)
Oh, you're talking about the screwup that is SVGStylable not inheriting from ElementCSSInlineStyle ?

Yeah, that sucks, along with so many other things about the SVG "DOM"...  You'll probably need to have those methods in the classes they're in now and have them call the common superclass impl.  :(
Yes, exactly that was it.  And indeed, I'll try to get it work simply calling the common method, as you suggested.  It seems to me, too, that SVG could really need some clean-up and work to get some duplicated code out of it; however, as far as my patch here does not break it (but I want at least introduce nsStyldElement as base even for SVG), this can be done afterwards whenever someone finds time, right?
Actually, the interface here is an issue with the SVG spec reinventing the wheel, not with the Mozilla SVG code...
This patch works for me both for HTML and SVG, moved quite a significant amount of code out of nsGenericHTMLElement and simplified nsSVGElement/nsSVGStylableElement, too.  Please have a look at it and tell me whether I did miss something to move and the like.

A reftest for HTML and SVG (the two which work) is also included, hope I did this right.
Attachment #263288 - Attachment is obsolete: true
Attachment #263346 - Flags: review?(bzbarsky)
Comment on attachment 263346 [details] [diff] [review]
Patch which works for HTML and SVG

>Index: content/base/src/nsStyledElement.cpp

I'm not sure about the copyright notice given that a lot of that code is basically copied, but it probably doesn't matter much.

>+// nsISupports methods

Why is this class even declaring nsISupports stuff?  I'd take out the NS_DECL_ISUPPORTS_INHERITED and these implementations of AddRef/Release/QueryInterface.

>+nsStyledElement::ParseAttribute(PRInt32 aNamespaceID, nsIAtom* aAttribute,

>+    if (aAttribute == nsGkAtoms::style) {
>+      ParseStyleAttribute(this, PR_TRUE, aValue, aResult);

Actually, the second arg to ParseStyleAttribute can be removed.  See below.

>+    if (aAttribute == nsGkAtoms::_class) {
>+      aResult.ParseAtomArray(aValue);

I _think_ this is changing behavior for SVG... If nothing else, this is affecting all SVG elements, not just nsSVGStylableElement.  And even then, doesn't SVG need the whole nsSVGClassValue mess going on?

jwatt, could you look over the SVG parts of this?

>+NS_IMETHODIMP
>+nsStyledElement::WalkContentStyleRules(nsRuleWalker* 

The mapped attribute code is very much HTML-specific thus far.  I don't think it makes sense to hoist this function up here.

>+nsStyledElement::GetInlineStyleRule()

This lost a bunch of the logic from nsGenericHTMLElement::GetInlineStyleRule.  You shouldn't lose that...

(need test for this too).

>+nsStyledElement::GetStyle(nsIDOMCSSStyleDeclaration** aStyle)
>+  // Why bother with QI?
>+  NS_IF_ADDREF(*aStyle = slots->mStyle);

That could probably be an NS_ADDREF().

>+nsStyledElement::ReparseStyleAttribute()

jwatt, is this correct for SVG?

>+nsStyledElement::ParseStyleAttribute(nsIContent* aContent,
>+                                     PRBool aCaseSensitive,

I know you just moved it, but this second arg is unused.  Just get rid of it?

Note that nsGenericHTMLElement::ParseStyleAttribute changed a bit a few days ago, so you'll have to resync this.. Luckily, if you don't it just won't compile.  ;)

>Index: content/base/src/nsStyledElement.h
>+/*
>+ * nsStyledElement is the base for elements supporting styling via the

If you make this comment start with a "/**" and put it before the include guard, it'll show up usefully in LXR directory listings.

>+class nsRuleWalker;

Shouldn't need this if you move WalkContentStyleRules back to HTML.

Is there a reason not to inline the constructor here?

>+  // nsISupports
>+  NS_DECL_ISUPPORTS_INHERITED

Remove this, please.

>+  // Forward to parent
>+  NS_FORWARD_NSIDOMNODE(nsStyledElementBase::)
>+  NS_FORWARD_NSIDOMELEMENT(nsStyledElementBase::)

And these.  There's no reason for them.

>+  NS_IMETHOD WalkContentStyleRules(nsRuleWalker* aRuleWalker);

Move that back into HTML.

>+  NS_IMETHOD GetStyle(nsIDOMCSSStyleDeclaration** aStyle);

This isn't an NS_IMETHOD.  Just a method that returns nsresult.  And it should probably be protected...

>+  NS_IMETHOD Clone(nsINodeInfo*, nsINode**) const;

Er... you don't implement that, right?  Nor should you, implement it or declare it.

>+#endif // __NS_STYLEDELEMENT_H__

Uber-nit: make the tail just "_", not "__".  The latter is technically reserved in C++.

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>-nsresult
>-nsGenericHTMLElement::GetStyle(nsIDOMCSSStyleDeclaration** aStyle)

It looks like you're not only moving this, but changing the |nsresult| to a |NS_IMETHODIMP|.  Please don't do either one -- just change the body of the method in-place.

>Index: content/svg/content/src/nsSVGElement.h
>-  NS_IMETHOD WalkContentStyleRules(nsRuleWalker* aRuleWalker);
...
>+  // nsIContent
>+  NS_IMETHOD WalkContentStyleRules(nsRuleWalker* aRuleWalker);
>+

Why move that?  Please put it back where it came from.

>Index: layout/reftests/bugs/379178-html-ref.xht

.xhtml, please.

And for both the ref and the test, add a style rule that first sets all span backgrounds to white, so the difference will be obvious no matter what the tester's UA settings?

Might also have an HTML (not XHTML) test, if desired.
Attachment #263346 - Flags: review?(jwatt)
Attachment #263346 - Flags: review?(bzbarsky)
Attachment #263346 - Flags: review-
This patch should address all of your comments; I'm not requesting review for it nor marking the previous one as obsolete, as I'm not sure whether to wait for jwatt's review first; if this isn't good practice, please correct me.

Here are my comments:

* I did fill in the header as I had done for a new file created; now I changed some parts, and if you suggest some header-comment, I'll take that one.

* Removed nsISupport-stuff.

* Moved WalkContentStyleRule back to HTML; I was not sure whether the HTML-implementation was "generally useable", I just assumed that.

* Restored the HTML-version of GetInlineStyleRule -- but this in turn *adds* that logic to SVG, is this a problem?  How to test this, I thought this is somewhat connected with style-attribute?

* GetStyle now has the correct signature; BTW, the SVG-version did something similiar but not exactly the same as HTML -- please check that isn't going to be a problem.

* I believe I was both declaring and implementing Clone -- but now I removed it entirely.

* Changed the xhtml to .xhtml and added HTML-versions of them; hope this is what you were thinking of?

Please excuse my mistakes, I'm not yet really familiar with Mozilla's styling engine -- I did more trial-and-error work here.
Yeah, I'd wait for jwatt's comments, though I would move the review request to the updated patch.

> but this in turn *adds* that logic to SVG, is this a problem

jwatt?  ;)

> How to test this, I thought this is somewhat connected with style-attribute?

Honestly, given that we now have a GetOwnerDoc(), I'm not quite sure what the purpose of this code is... it basically handles a case where a node starts out with no owner document and later gets one, which I don't think can happen. jst?  sicking?

> hope this is what you were thinking of?

Yep.

> I'm not yet really familiar with Mozilla's styling engine

Hey, neither is anyone else, completely.  ;)
> > How to test this, I thought this is somewhat connected with style-attribute?
> 
> Honestly, given that we now have a GetOwnerDoc(), I'm not quite sure what the
> purpose of this code is... it basically handles a case where a node starts out
> with no owner document and later gets one, which I don't think can happen. 
> jst? sicking?

Actually, I'm pretty sure that this is a leftover from long long time ago, and I've just left it in as I've been rewriting that code. I know the old code was calling ReposeStyleAttribute left and right 'just in case'. So I think it's safe to go with the SVG version.

What you probably want to do though is to move the call to ReparseStyleAttribute from nsGenericHTMLElement::BindToTree to nsStyledElement::BindToTree. Unfortunately that call is somewhat useless often, see comment in nsGenericHTMLElement::BindToTree, but lets fix that in a separate bug.

For now I think you can go with the SVG version of GetInlineStyleRule and move the ReparseStyleAttribute call to nsStyleElement::BindToTree
Btw, once you're done with the review comments i'd like to sr the final patch.
Oh, and is there a reason you're not making nsXULElement inherit nsStyleableElement?
So I should implement BindToTree in nsStyledElement with that reparse-call?  Or did you even mean this to be done with a seperate bug?

Well, I thought of nsXULElement just when changing my patch today (as it used ParseStyleAttribute), but I don't know much XUL, so I couldn't really tell whether this accepts class/id/style the way SVG and HTML do.  So for this one I'm waiting for other one's comments; but if this can be done, I'll do that of course.

You know, deleting superfluous code is always joyful :P
Yes, add, in this patch, and implementation of nsStyledElement::BindToTree (and make sure that the subclasses BindToTree call through that, but I think that's already the case).

XUL does have the same style/class/id yes. The only tricky part is that you'll have to call GetAttrInfo rather than going to mAttrsAndChildren directly since XUL stores attributes elsewhere too.
Do we really want to do that, then?  That'll add a virtual function call, and these methods are used a fair amount during style resolution...  Might be ok, I guess.
Yeah, i doubt it'll matter.

On a separate note, we could make GetAttrInfo non-virtual if we made it check if it's a XUL element and if so cast to nsXULElement. But that's a different bug again :)
Here I override BindToTree in nsStyledElement, making this ReparseStyleAttribute-call; in turn, I use now the SVG-version of GetInlineStyleRule and removed the call from nsGenericHTMLElement (but for the latter case nothing should have changed).  Hope I understood Jonas correctly.

I took a look at nsXULElement; for me it looks as if we could only get rid of GetID/ClassAttributeName and the two if's in ParseAttribute for style and class.  Everything else looks different from the implementation of nsStyledElement; is this right and should I proceed with that one?
Attachment #263346 - Attachment is obsolete: true
Attachment #263720 - Attachment is obsolete: true
Attachment #263848 - Flags: review?(jwatt)
Attachment #263346 - Flags: review?(jwatt)
First of all, it's good to see attempts to keep the code clean and to avoid code duplication. It's also important to make sure that design and most importantly the behavior is correct though.

First of all I'm not keen on having elements that shouldn't have a concept of inline style and styling classes inherit a base class with that knowledge. What's definitely wrong though is that in this case your new base class doesn't just have some abstract knowledge of this stuff, it implements it and will lead to incorrect behavior as Boris said. For many SVG elements we should not do anything special with 'class' and 'style' attributes but with this patch we would. In the generic SVG case GetClassAttributeName should return nsnull (note it's currently wrong that it returns nsGkAtoms::_class for all SVG elements) and ParseAttribute should not do anything with 'class' and 'style' attributes. As a result I don't think nsSVGElement should inherit a nsStyledElement class.
So do you think we should simply use nsStyledElement only for HTML and MathML to come?  If SVG can't really benefit directly from this class, this would be the way which seems to be reasonable to me.
Grrr.. why does the SVG spec have to make everything so bloody complicated :( :(

I assume fixing the spec is out.

But could we subclass nsSVGElement into nsSVGUnstyledElement that overrides GetClassAttributeName etc and disables the styling code for them? Not sure if that would be more or less code though...

Additionally, nsSVGElement today does return nsGkAtoms::_class, is that something that someone really intends to fix? If not I don't see what we'd loose by moving nsSVGElement over to nsStyledElement.
Well, that override-to-disable approach was something I thought of, too -- but would this bring any benefit?  BTW, virtual inheritance is not used in Mozilla, is it?  If it was, couldn't we make nsSVGStyledElement inherit both nsStyledElement and nsSVGElement instead, so direct childs of nsSVGElement are "unstyled"?
I suspect that it'd be less code to let nsSVGElement inherit nsStyledElement and then override parts of it in nsSVGUnstyledElement, than to do what we do today.

I could be wrong though.
Daniel, are you still waiting on jwatt here?
In general, yes.  If there's a decision to exclude SVG and take MathML/HTML, I'd change my patch.
Tim, ignoring the details of Daniel's patch for now, what do you think of making all SVG elements recognize the 'style' attribute? I don't think this will cause any real issues. The SVG 1.1 Full elements that wouldn't otherwise recognize 'style' are:

animation:
 animate
 set
 animateMotion
 animateColor
 animateTransform
 mpath

filter:
 feBlend
 feColorMatrix
 feComponentTransfer
 feComposite
 feConvolveMatrix
 feDisplacementMap
 feDistantLight
 feFuncR
 feFuncG
 feFuncB
 feFuncA
 feGaussianBlur
 feMerge
 feMergeNode
 feMorphology
 feOffset
 fePointLight
 feSpotLight
 feTile
 feTurbulence

other:
 altGlyphDef
 altGlyphItem
 color-profile
 cursor
 metadata
 script
 style
 view
 hkern
 vkern
 font-face
 font-face-src
 font-face-uri
 font-face-format
 font-face-name
 definition-src
(In reply to comment #38)
> Tim, ignoring the details of Daniel's patch for now, what do you think of
> making all SVG elements recognize the 'style' attribute? I don't think this
> will cause any real issues.

While I don't see adding 'style' to those elements causing problems, do we really want to go against the spec if not necessary?
Does applying style to any of those elements make a difference? I.e. are we going to actually behave differently than the spec mandates?

And is there any chance we can get the spec changed? Seems more painful for everyone to not be consistent.
I can't think of a scenario in SVG 1.1 where an SVG author would/should be able to see any difference on those elements. Given that, I'm not sure we need care about fixing the SVG spec. In any case, I've long since given up on trying to get bugs in the SVG spec. fixed, let alone arguing that an attribute should be specified on elements where it is useless just to make implementers lives easier.
I'm not against this in principle, but it seems we are slowly diverging more and more from SVG 1.1. This isn't bad in and of itself, but it will make it harder for other browser vendors to stay interoperable. Could we make sure we have publically, fully, and clearly documented our various extensions and differences and informed the www-svg mailing list of the existence of this documentation?
I'm not actually sure if this will affect content since none of these elements are apparently rendered? The possible effect is if someone calls getComputedStyle on one of these elements, but I don't know what that'll do for non-displayed elements.
Could we decide something here?  This is rapidly getting to the "misses 1.9" point given current scheduling plans.
Given that no-one has protested, and it doesn't seem like the change will affect any end-users anyway, i say we should just do it.
I'm for this. I'm deferring to tor as SVG module owner to make the decision for whether this is okay for SVG though.
Comment on attachment 263848 [details] [diff] [review]
Call ReparseStyleAttribute from within nsStyledElement::BindToTree

>+nsStyledElement::SetInlineStyleRule(nsICSSStyleRule* aStyleRule, PRBool aNotify)
>+{
>+  PRBool modification = PR_FALSE;
>+  nsAutoString oldValueStr;
>+
>+  PRBool hasListeners = aNotify &&
>+    nsContentUtils::HasMutationListeners(this,
>+                                         NS_EVENT_BITS_MUTATION_ATTRMODIFIED);
>+
>+  // There's no point in comparing the stylerule pointers since we're always
>+  // getting a new stylerule here. And we can't compare the stringvalues of
>+  // the old and the new rules since both will point to the same declaration
>+  // and thus will be the same.
>+  if (hasListeners) {
>+    // save the old attribute so we can set up the mutation event properly
>+    // XXXbz if the old rule points to the same declaration as the new one,
>+    // this is getting the new attr value, not the old one....
>+    modification = GetAttr(kNameSpaceID_None, nsGkAtoms::style,
>+                           oldValueStr);
>+  }
>+  else if (aNotify && IsInDoc()) {
>+    modification = !!mAttrsAndChildren.GetAttr(nsGkAtoms::style);
>+  }

Actually, you should remove the IsInDoc() test here.

>+  nsAttrValue attrValue(aStyleRule);
>+
>+  return SetAttrAndNotify(kNameSpaceID_None, nsGkAtoms::style, nsnull,
>+                          oldValueStr, attrValue, modification, hasListeners,
>+			  aNotify);

Fix indentation. Never use tabs in cpp files.

>+nsStyledElement::GetStyle(nsIDOMCSSStyleDeclaration** aStyle)
>+{
>+  nsGenericElement::nsDOMSlots *slots = GetDOMSlots();

You need to null-check slots. In case of out-of-memory.

>+  if (!slots->mStyle) {
>+    // Just in case...
>+    ReparseStyleAttribute();

Didn't we conclude that this was pointless?

>+    nsresult rv;
>+    if (!gCSSOMFactory) {
>+      rv = CallGetService(kCSSOMFactoryCID, &gCSSOMFactory);
>+      if (NS_FAILED(rv)) {
>+        return rv;
>+      }

Use NS_ENSURE_SUCCESS(rv, rv)

>+    }
>+
>+    rv = gCSSOMFactory->CreateDOMCSSAttributeDeclaration(this,
>+                                                 getter_AddRefs(slots->mStyle));
>+    if (NS_FAILED(rv)) {
>+      return rv;
>+    }

Use NS_ENSURE_SUCCESS(rv, rv)

>+  }
>+
>+  // Why bother with QI?
>+  NS_ADDREF(*aStyle = slots->mStyle);

Why this comment?

>+nsStyledElement::ParseStyleAttribute(nsIContent* aContent,
>+                                     const nsAString& aValue,
>+                                     nsAttrValue& aResult)
>+{
>+  nsresult result = NS_OK;
>+  nsIDocument* doc = aContent->GetOwnerDoc();
>+
>+  if (doc) {
>+    PRBool isCSS = PR_TRUE; // assume CSS until proven otherwise
>+
>+    if (!aContent->IsNativeAnonymous()) {  // native anonymous content
>+                                           // always assumes CSS
>+      nsAutoString styleType;
>+      doc->GetHeaderData(nsGkAtoms::headerContentStyleType, styleType);
>+      if (!styleType.IsEmpty()) {
>+        static const char textCssStr[] = "text/css";
>+        isCSS = (styleType.EqualsIgnoreCase(textCssStr, sizeof(textCssStr) - 1));
>+      }
>+    }
>+
>+    if (isCSS) {
>+      nsICSSLoader* cssLoader = doc->CSSLoader();
>+      nsCOMPtr<nsICSSParser> cssParser;
>+      result = cssLoader->GetParserFor(nsnull, getter_AddRefs(cssParser));
>+      if (cssParser) {
>+        nsCOMPtr<nsIURI> baseURI = aContent->GetBaseURI();
>+
>+        nsCOMPtr<nsICSSStyleRule> rule;
>+        result = cssParser->ParseStyleAttribute(aValue, doc->GetDocumentURI(),
>+                                                baseURI,
>+						aContent->NodePrincipal(),
>+                                                getter_AddRefs(rule));

Fix tabs.

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

>+// Forward to nsStyledElement::GetStyle
>+NS_IMETHODIMP
> nsGenericHTMLElement::GetStyle(nsIDOMCSSStyleDeclaration** aStyle)
> {

What's the purpose of this function?
Attachment #263848 - Flags: superreview+
Well, I actually mostly copied this code from nsGenericHTMLElement, so I can't really answer why some details are there...

But if you guys approve, I will simply address these comments.
Comment on attachment 263848 [details] [diff] [review]
Call ReparseStyleAttribute from within nsStyledElement::BindToTree

This looks okay to me. r=jwatt with the following comments addressed. Sorry this has taken an absolute age. :-(

>+const nsAttrValue*
>+nsStyledElement::GetClasses() const
>+{
>+  return mAttrsAndChildren.GetAttr(nsGkAtoms::_class);
>+}

We don't want this in SVG. Please override it to return nsnull on nsSVGElement.

>+PRBool
>+nsStyledElement::ParseAttribute(PRInt32 aNamespaceID, nsIAtom* aAttribute,
>+                                const nsAString& aValue, nsAttrValue& aResult)
>+{
>+  if (aNamespaceID == kNameSpaceID_None) {
>+    // Style-Attribute?
>+    if (aAttribute == nsGkAtoms::style) {
>+      ParseStyleAttribute(this, aValue, aResult);
>+      return PR_TRUE;
>+    }
>+    // Class-Attribute?
>+    if (aAttribute == nsGkAtoms::_class) {

Please assert that the element's namespace is not SVG here. (Also, I'm not sure why you added the comments there. I'd just remove them. The code is self explanatory and we don't do that anywhere else.)

>-NS_IMPL_ADDREF_INHERITED(nsSVGElement,nsGenericElement)
>-NS_IMPL_RELEASE_INHERITED(nsSVGElement,nsGenericElement)
>+NS_IMPL_ADDREF_INHERITED(nsSVGElement,nsSVGElementBase)
>+NS_IMPL_RELEASE_INHERITED(nsSVGElement,nsSVGElementBase)

Can you insert a space after the commas while you're here?
Attachment #263848 - Flags: review?(jwatt) → review+
(In reply to comment #20)
> >+nsStyledElement::ReparseStyleAttribute()
> 
> jwatt, is this correct for SVG?

Well, I'm not sure bz. It looks okay to me. Is there any particular reason you had for asking about the reparse method? SVG has been using the ParseStyleAttribute method for ages but hasn't been using the ReparseStyleAttribute method trick. It looks like it should.
(In reply to comment #49)
> >+PRBool
> >+nsStyledElement::ParseAttribute(PRInt32 aNamespaceID, nsIAtom* aAttribute,
> >+                                const nsAString& aValue, nsAttrValue& aResult)
> >+{
> >+  if (aNamespaceID == kNameSpaceID_None) {
> >+    // Style-Attribute?
> >+    if (aAttribute == nsGkAtoms::style) {
> >+      ParseStyleAttribute(this, aValue, aResult);
> >+      return PR_TRUE;
> >+    }
> >+    // Class-Attribute?
> >+    if (aAttribute == nsGkAtoms::_class) {
> 
> Please assert that the element's namespace is not SVG here. (Also, I'm not sure
> why you added the comments there. I'd just remove them. The code is self
> explanatory and we don't do that anywhere else.)

What do you mean by this? Some sort of debug assert or another condition inside the if()?

The other ones will be easy to fix (of course), I'll file a new patch ASAP.
> Is there any particular reason you had for asking

Yes.  Unlike HTML (where the code just got moved), this was a code change for SVG.
Hmm.. why do you not want a GetClasses implementation for SVG. Doesn't that break class-matching rules like "circle.foo { ... }" Though apparently not since there is no implementation there now.
(In reply to comment #53)
> Unlike HTML (where the code just got moved), this was a code change for SVG.

Yup. Looks okay to me though.

(In reply to comment #54)
> Hmm.. why do you not want a GetClasses implementation for SVG. Doesn't that
> break class-matching rules like "circle.foo { ... }" Though apparently not
> since there is no implementation there now.

It is implemented, but only for elements implementing SVGStylableElement. See

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/svg/content/src/nsSVGStylableElement.cpp&rev=1.8&mark=94#91

It has to be implemented in a different way though as you can see there.
(In reply to comment #52)
> > Please assert that the element's namespace is not SVG here.
> 
> What do you mean by this?

Sorry, I missed that question. I mean add this line to the top of the if block:

      NS_ASSERTION(GetNameSpaceID() != kNameSpaceID_SVG,
                   "SVG code should have parsed the 'class' attribute itself!");
Btw, in light of http://dev.w3.org/cvsweb/~checkout~/2006/webapi/selectors-api/draft/selectors-api.htm?content-type=text/html
it would IMHO make a lot of sense to let even non-styleable svg elements implement GetClasses. Does the spec actually say this is not allowed?
What specifically are you referring to from that?
The spec linked to in comment 57 adds a DOM API that lets you get all elements with a specific class. It seems to me that it could be useful for all SVG elements, not just styleable ones.

Or to put it another way. Classes are used for things other than styling, so why only support them on styleable elements?
Good point. Okay, yeah, I'm convinced. Daniel, forget about adding GetClasses to nsSVGElement.
Oh, and you'll need to change the assertion to:

      NS_ASSERTION(!nsCOMPtr<nsIDOMSVGStylable>(do_QueryInterface(this)),
                   "SVG code should have handled this 'class' attribute!");

(including the header nsIDOMSVGStylable.h of course).
Though adding class-support for all svg-elements needs a bit more changes since mClass doesn't exist on nsSVGElement, no? I'd say it should be done in a separate patch, so for now i think your initial comments were right.
Attached patch Addresses comments so far (obsolete) — Splinter Review
This is a new patch in sync with current CVS and addressing the comments of Jonathan (Comment #49) and Jonas (Comment #47).

Jonas' points about IsInDoc() and ReparseStyleAttribute were excluded; as I did just copy this code I'd like to have one more assurance this can be done as Jonas suggested.

Unfortunatelly, I was not yet able to test the compiled binary with this new patch because of some problems on my system with GCC/Glibc, but please have a look at this new patch.
Attachment #263848 - Attachment is obsolete: true
you need to set review flags to make sure this gets reviewed.
Comment on attachment 273634 [details] [diff] [review]
Addresses comments so far

Well, I was not sure whether I should do so at the moment.  However, it sounds like a good idea now ;)
Attachment #273634 - Flags: review?(jwatt)
Comment on attachment 273634 [details] [diff] [review]
Addresses comments so far

>+nsStyledElement::ParseAttribute(PRInt32 aNamespaceID, nsIAtom* aAttribute,
>+                                const nsAString& aValue, nsAttrValue& aResult)
>+{
>+  if (aNamespaceID == kNameSpaceID_None) {
>+    NS_ASSERTION(!nsCOMPtr<nsIDOMSVGStylable>(do_QueryInterface(this)),
>+                 "SVG code should have handled this 'class' attribute!");
>+    if (aAttribute == nsGkAtoms::style) {
>+      ParseStyleAttribute(this, aValue, aResult);
>+      return PR_TRUE;
>+    }
>+    if (aAttribute == nsGkAtoms::_class) {
>+      aResult.ParseAtomArray(aValue);
>+      return PR_TRUE;
>+    }

The assertion needs to be inside the nsGkAtoms::_class check. So:

    if (aAttribute == nsGkAtoms::_class) {
      NS_ASSERTION(!nsCOMPtr<nsIDOMSVGStylable>(do_QueryInterface(this)),
                   "SVG code should have handled this 'class' attribute!");
      aResult.ParseAtomArray(aValue);
      return PR_TRUE;
    }

r=jwatt with that change.

(In reply to comment #62)
> Though adding class-support for all svg-elements needs a bit more changes since
> mClass doesn't exist on nsSVGElement, no?

No. mClass is only needed for the SVGStylable stuff, which we don't want/need for all SVG elements. Using the nsStyledElement handling for the class attribute for non-SVGStylable SVG elements is fine. I.e. it's fine as-is.
Attachment #273634 - Flags: review?(jwatt) → review+
Attached patch Fixes assertion (obsolete) — Splinter Review
This one fixes the assertion as mentioned by jwatt.  I hope it is pretty mature now, and am not sure what to do next -- is any further review needed and if so, whom shall I ask?
Attachment #273634 - Attachment is obsolete: true
There are some indentation issues in nsGenericHTMLElement at the return points, but that can be fixed at checkin.

The next step is to request approval for 1.9 checkin once that flag exists.  :(
Keywords: checkin-needed
Comment on attachment 274785 [details] [diff] [review]
Fixes assertion

This patch factors out some common styling-related code from nsGenericHTMLElement and nsSVGElement into a superclass, eliminating redundancy and laying the groundwork for low-cost implementations of styling for other sorts of elements (e.g. MathML).

Risk is fairly low, especially for HTML: most of the code is just moving directly from nsGenericHTMLElement.
Attachment #274785 - Flags: approval1.9?
Attachment #274785 - Flags: approval1.9? → approval1.9+
Will sync and post bug 276267 fix once this is committed.
I can't help feel uncomfortable with the copyright attribution. I kinda feel like we are demeaning it. I feel a proper attribution would be to replicate what is in nsGenericHTMLElement and then append daniel as a contributor.  (I don't know/remember what is there in nsGenericHTMLElement, and in a sense, I am glad that I don't know--so as not to be influenced by it. I am making this comment on face value.)
As I said above, I don't really care about this--feel free to change the header to whatever is appropriate or suggest something here and I can change it and submit a new patch.
Checked in.  Daniel, thanks for the patch and for putting up with the whole process.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You're welcome, and thank you guys that this finally happened ;) I'm glad to see this in there!
Seems that this checkin busted Thunderbird and Lightning tinderboxen:
<http://tinderbox.mozilla.org/showbuilds.cgi?tree=Thunderbird>
<http://tinderbox.mozilla.org/showbuilds.cgi?tree=Sunbird>

  Cannot open include file: 'nsIDOMSVGStylable.h': No such file or directory
Jeff Walden checked in a fix for that.  I still don't understand why those apps build with SVG disabled, but we've got bugs on that...
The class selector still doesn't work for MathML.  (You can see that with the testcase in comment 11.)  Should I file a new bug?
MathML uses generic element class which does not use this bug's code. Bug 276267 is all about that, and is (hopefully) fixed by attachment 275478 [details] [diff] [review] once it or its adjusted version gets in.
Keywords: checkin-needed
style-property-not-on-script-element-01.svg was added as a *failing* reftest for this bug. The funny thing is, it failed for the wrong reasons. SVG 'script' elements don't have a 'style' DOM attribute because no interface with that attribute is exposed through nsDOMClassInfo. The test did fail on trunk anyway, but I'm not sure why; the 'rect' fill successfully gets set to 'lime', but the rect still displays red. This bug has gone away since my checkin of 'part 1' in bug 344258. This is disturbing but I'm not going to fully debug it now.
Depends on: 573340
You need to log in before you can comment on or make changes to this bug.