234861 - [FIXr]presentation attributes don't work in anonymous content

Status: RESOLVED FIXED

(Core :: SVG, defect, P2)

Description

[Alex Fritze]

I am generating some anonymous SVG content for a frame through the
nsIAnonymousContentCreator interface.
The problem is that in the generated content, SVG presentation attributes
("fill", "stroke", etc) don't get mapped into the element's style.
This is a result of the bug#233370 patch which delegated the style mapping to
document sheets. Apparently document sheets don't get applied to anonymous content.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yep.  The problem is that the attr sheet is not applied to anonymous content,
because it's a document sheet.

It really shouldn't be a document sheet but a UA one, in my opinion.... except
possibly in non-XHTML HTML documents (per what CSS2.1 is saying, if I follow it
correctly). Or am I on crack?

Comment 2

[Hixie (not reading bugmail)]

SVG attributes should indeed be in the UA stylesheet. Only certain HTML
presentational attributes are supposed to be implemented at the author level.

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK.  So HTML and XHTML should behave differently here?  XHTML should be like all
other *ML and the attributes should be at the ua level, while HTML they should
be at the author level?

(I'm just clarifying so I only have to change this code once.... ;) )

Comment 4

[Hixie (not reading bugmail)]

Correct.

And the list of presentational attributes for HTML is defined as any attribute
that is not in the following list: abbr, accept-charset, accept, accesskey,
action, alt, archive, axis, charset, checked, cite, class, classid, code,
codebase, codetype, colspan, coords, data, datetime, declare, defer, dir,
disabled, enctype, for, headers, href, hreflang, http-equiv, id, ismap, label,
lang, language, longdesc, maxlength, media, method, multiple, name, nohref,
object, onblur, onchange, onclick, ondblclick, onfocus, onkeydown, onkeypress,
onkeyup, onload, onload, onmousedown, onmousemove, onmouseout, onmouseover,
onmouseup, onreset, onselect, onsubmit, onunload, onunload, other event handler
attributes that are non-standard, profile, prompt, readonly, rel, rev, rowspan,
scheme, scope, selected, shape, span, src, standby, start, style, summary,
title, type (except on LI, OL and UL elements), usemap, value, valuetype, version.

Elements (e.g. <center>) must always be at the UA-level, even in HTML.

Comment 5

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

This seems strange to me, i thought that presentational attributes in (x)html
and most other markups were supposed to be overruled by CSS. (svg would be
logical if it was different since SVG is mostly about presentation anyway). I
take it that i've missunderstood things then?

Anyway, this still means that presentational attributes in HTML-elements in
anonymous content still won't work. Not sure how big of a deal that is since we
always use XHTML there. And i guess since XBL is an namespace aware language we
don't support putting non-namespace aware HTML in it, even through DOM.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yes, but the host document is what matters, not the type of element, since the
document is what's putting sheets in the style set....

At the moment, the thought I'm having is to explicitly put mAttrStyleSheet in
the ua level in the style set for XML docs (this includes XHTML) and to create a
new "preshint" level for use by HTML docs.

At the same time, I am trying to look forward to when we implement attr mapping
for XUL.  At that point, if a document contains both XHTML and XUL, say, their
attrs would get mapped at different places in the cascade... so I'm trying to
figure out a clean way to handle the whole mess.

Comment 7

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

If it's the host document that matters then wouldn't we get bitten in the ass if
we did proper attrmapping for XUL and then used XBL formcontrols?

I don't see any big problems with having two mapped-attrs stylesheets at
different places in the cascade. The stylesystem doesn't care that a stylesheet
is a mapped-attrs stylesheet right? And we already have different stylesheets
for inlinestyle and mappedattrs.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> and then used XBL formcontrols?

Only if we expected the XUL attr weirdness to work in there.  It's really only
useful for persist stuff to work, so it's easy not to depend on it.

On the other hand, I guess we could do what you suggest.  Have all documents
have two mapped attr sheets, and have those sheets check whether a node is in
the XUL namespace to decide whether to map its attributes...

Comment 9

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

That does not sound as attractive as I first thought, I had hoped that the
elements could manage this themselfs. But i realize that that might not be all
that easy.

I'm not very oppinionated either way, especially since i don't know enough about
the changes involved.

Comment 10

[Hixie (not reading bugmail)]

> i thought that presentational attributes in (x)html and most other markups were
> supposed to be overruled by CSS.

That's what I said, no?

Comment 11

[Alex Fritze]

So, can anyone point me towards the attrib sheet that currently does the mapping
... and to where it needs to be moved? 

Comment 12

[Boris Zbarsky [:bzbarsky]]

http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.h#567

As for where it needs to be moved, that's the question.  It needs to be moved to
the UA level, unless we're in HTML, when it needs to be moved to the preshint
level, which needs to be added.

Also, in the UA level we need to make sure that it comes after other UA sheets
no matter what.

Alternately, we create several preshint levels at different points in the
cascade and decide which one it goes in based on the document type.  Silly HTML
exception.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Er, actually more like this

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 145851 [details] [diff] [review]
Er, actually more like this

I suppose I could have created two different preshint levels (for HTML and
others), at the two different places in the cascade.  But that seemed like
overkill; unless someone does something weird with UA sheets the attr sheet
should be the most important sheet in the UA level for XML documents with this
patch.

Comment 16

[Johnny Stenback (:jst)]

Comment on attachment 145851 [details] [diff] [review]
Er, actually more like this

Sorry for dragging my butt with this review.

sr=jst, assuming dbaron is fine with this change.

Comment 17

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

Comment on attachment 145851 [details] [diff] [review]
Er, actually more like this

I'm not crazy about nsDocument.h including nsStyleSet.h, but I guess it's OK. 
You added a line of extra whitespace in nsHTMLDocument.h, but other than that,
r=dbaron.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Yeah, the alternative was rewriting all this code to move ownership of the attr
sheet and such out of the document, which I wasn't quite ready for....

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 20

[Wolfgang Rosenauer [:wolfiR]]

I get the following build error now:

c++ -o nsXBLBinding.o -c -DOSTYPE=\"Linux2.6\" -DOSARCH=\"Linux\"
-D_IMPL_NS_LAYOUT -I./../../base/src -I./../../html/style/src
-I./../../html/base/src
 -I./../../html/document/src -I./../../xml/document/src
-I./../../xul/content/src  -I../../../dist/include/xpcom
-I../../../dist/include/string -I../..
/../dist/include/js -I../../../dist/include/dom -I../../../dist/include/gfx
-I../../../dist/include/layout -I../../../dist/include/xultmpl -I../../../d
ist/include/widget -I../../../dist/include/view -I../../../dist/include/caps
-I../../../dist/include/htmlparser -I../../../dist/include/necko -I../../.
./dist/include/xpconnect -I../../../dist/include/pref
-I../../../dist/include/docshell -I../../../dist/include/webshell
-I../../../dist/include/lwbrk -
I../../../dist/include/xul -I../../../dist/include/xuldoc
-I../../../dist/include/rdf -I../../../dist/include/imglib2
-I../../../dist/include/unicharut
il -I../../../dist/include/locale -I../../../dist/include/intl
-I../../../dist/include/content -I../../../dist/include
-I/usr/src/packages/BUILD/mozill
a/dist/include/nspr     -I/usr/X11R6/include   -fPIC  -I/usr/X11R6/include
-fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -W
overloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor
-Wno-long-long -pedantic -O2 -march=i586 -mcpu=i686 -fmessage-length=0 -Wall -f
no-strict-aliasing -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED
-ffunction-sections -O2 -march=i586 -mcpu=i686 -fmessage-length=0 -Wall -fno-strict
-aliasing  -I/usr/X11R6/include -DMOZILLA_CLIENT -include
../../../mozilla-config.h -Wp,-MD,.deps/nsXBLBinding.pp nsXBLBinding.cpp
In file included from ../../../dist/include/necko/nsNetUtil.h:58,
                 from nsXBLBinding.cpp:58:
../../../dist/include/xpcom/nsIInterfaceRequestorUtils.h:42: warning: `class
nsGetInterface' has virtual functions but non-virtual destructor
In file included from ../../../dist/include/xpcom/nsIServiceManager.h:178,
                 from ../../../dist/include/necko/nsNetUtil.h:60,
                 from nsXBLBinding.cpp:58:
../../../dist/include/xpcom/nsIServiceManagerUtils.h:48: warning: `class
nsGetServiceByCID' has virtual functions but non-virtual destructor
../../../dist/include/xpcom/nsIServiceManagerUtils.h:81: warning: `class
nsGetServiceByContractID' has virtual functions but non-virtual destructor
../../../dist/include/xpcom/nsIServiceManagerUtils.h:114: warning: `class
nsGetServiceFromCategory' has virtual functions but non-virtual destructor
In file included from ../../base/src/nsDocument.h:45,
                 from ../../xml/document/src/nsXMLDocument.h:42,
                 from nsXBLBinding.cpp:67:
../../../dist/include/xpcom/nsWeakReference.h:36: warning: `class
nsSupportsWeakReference' has virtual functions but non-virtual destructor
../../../dist/include/xpcom/nsWeakReference.h:71: warning: `class
nsWeakReference' has virtual functions but non-virtual destructor
In file included from ../../xml/document/src/nsXMLDocument.h:42,
                 from nsXBLBinding.cpp:67:
../../base/src/nsDocument.h:460: error: syntax error before `virtual'
In file included from nsXBLBinding.cpp:70:
../../../dist/include/xpcom/nsSupportsArray.h:50: warning: `class
nsSupportsArray' has virtual functions but non-virtual destructor
gmake[4]: *** [nsXBLBinding.o] Error 1

Comment 21

[Wolfgang Rosenauer [:wolfiR]]

forget it. It worked the second time :-(

Comment 22

[Alex Fritze]

Reopening because the original problem is still not fixed:
nsIAnonymousContentCreator-created SVG contents' presentation attributes remain
dysfunctional.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Alex, please attach a testcase.  I can't fix a bug I can't reproduce.  It's an
XML document, right?  Not HTML?

Comment 24

[Alex Fritze]

I'd love to attach a test case, but I haven't got one that would be suitable.
The  testcase I'm currently using is dependent on code on the xtf branch and is
huge. 

I've got this loaded up in the debugger and i can see that basically for svg
content created via nsCSSFrameConstructor::CreateAnonymousFrames() ->
nsCSSFrameCTOR::ConstructFrame(), the style resolution in
nsCSSFrameCTOR::ResolveStyleContext() never calls
nsSVGElement::UpdateContentStyleRule().

For 'normally' constructed svg elements, presentation attribs work fine. The
stack in this case looks like this:

#1  0x4153f144 in nsSVGElement::UpdateContentStyleRule() (this=0x88a5a50)
    at nsSVGElement.cpp:711
#2  0x4153dbbf in nsSVGElement::WalkContentStyleRules(nsRuleWalker*) (
    this=0x88a5a50, aRuleWalker=0x8775808) at nsSVGElement.cpp:269
#3  0x41383a85 in nsHTMLStyleSheet::RulesMatching(ElementRuleProcessorData*,
nsIAtom*) (this=0x87ab230, aData=0xbfffe580, aMedium=0x80bc380)
    at nsHTMLStyleSheet.cpp:605
#4  0x4121ef49 in EnumRulesMatching (aProcessor=0x87ab234, aData=0xbfffe580)
    at nsStyleSet.cpp:333
#5  0x40793538 in nsVoidArray::EnumerateForwards(int (*)(void*, void*), void*)
    (this=0x887f110, aFunc=0x4121ef1a <EnumRulesMatching>, aData=0xbfffe580)
    at nsVoidArray.cpp:648
#6  0x412215c5 in nsCOMArray_base::EnumerateForwards(int (*)(void*, void*),
void*) (this=0x887f110, aFunc=0x4121ef1a <EnumRulesMatching>, aData=0xbfffe580)
    at nsCOMArray.h:65
#7  0x41221177 in nsCOMArray<nsIStyleRuleProcessor>::EnumerateForwards(int
(*)(nsIStyleRuleProcessor*, void*), void*) (this=0x887f110, 
    aFunc=0x4121ef1a <EnumRulesMatching>, aData=0xbfffe580) at nsCOMArray.h:221
#8  0x4121f2cb in nsStyleSet::FileRules(int (*)(nsIStyleRuleProcessor*, void*),
RuleProcessorData*) (this=0x887f0d0, 
    aCollectorFunc=0x4121ef1a <EnumRulesMatching>, aData=0xbfffe580)
    at nsStyleSet.cpp:433
#9  0x4122002d in nsStyleSet::ResolveStyleFor(nsIContent*, nsStyleContext*) (
    this=0x887f0d0, aContent=0x88a5a50, aParentContext=0x87d65f4)
    at nsStyleSet.cpp:532
#10 0x41092902 in nsCSSFrameConstructor::ResolveStyleContext(nsIPresContext*,
nsIFrame*, nsIContent*) (this=0x85fcb50, aPresContext=0x8806580, 
    aParentFrame=0x87d664c, aContent=0x88a5a50)
    at nsCSSFrameConstructor.cpp:6584
#11 0x4109410f in nsCSSFrameConstructor::ConstructFrame(nsIPresShell*,
nsIPresContext*, nsFrameConstructorState&, nsIContent*, nsIFrame*, nsFrameItems&) (
    this=0x85fcb50, aPresShell=0x88c2498, aPresContext=0x8806580, 
    aState=@0xbfffe9b0, aContent=0x88a5a50, aParentFrame=0x87d664c, 
    aFrameItems=@0xbfffe808) at nsCSSFrameConstructor.cpp:7165

Interestingly, looking at frame #8, I can see that the stylesheet that
ultimately leads us to nsSVGElement::UpdateContentStyleRule() is in the
eDocSheet list, not the eAgentSheet list.
Any ideas?

Comment 25

[Alex Fritze]

Ok, I could be totally wrong here, but your patch assumes that the PresShell(s)
has/have been created by the time nsDocument::ResetStyleSheetsToURI is called,
right? It looks like that is not the case and mAttrStyleSheet just gets added as a 
doc stylesheet in DocumentViewerImpl::CreateStyleSet() when the PresShell is
constructed.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Yeah, that's probably it.  I hate the document construction sprawl.  :(

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix up content viewer too

Alex, does this patch make things work for you?

Comment 28

[Alex Fritze]

Boris: 
Yeah, this fixes all of my testcases :-)

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 149349 [details] [diff] [review]
Fix up content viewer too

I had to move the preshint level out of the UA level for non-HTML because the
code in nsStyleSet that munges the quirks stylesheet assumes (and asserts, and
crashes if the assumption is violated) that all sheets in the UA level are
nsICSSStyleSheet.  If we ever fix that, we can probably eliminate the
ePresHintSheet level....

Comment 30

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

I think I prefer keeping the levels separate -- do we now have the invariant
that we have a single rule processor for each level?  (That's how it should be,
IMO.)  In some cases, the rule processor is a CSSRuleProcessor pointing to
multiple sheets.

Comment 31

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

Comment on attachment 149349 [details] [diff] [review]
Fix up content viewer too

>@@ -1192,10 +1209,9 @@ nsDocument::doCreateShell(nsIPresContext
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // Note: we don't hold a ref to the shell (it holds a ref to us)
>   mPresShells.AppendElement(shell);
>-  *aInstancePtrResult = shell.get();
>-  NS_ADDREF(*aInstancePtrResult);
>+  shell.swap(*aInstancePtrResult);

Is |*aInstancePtrResult| guaranteed to be null?

> nsStyleSet::FileRules(nsIStyleRuleProcessor::EnumFunc aCollectorFunc, 
>                       RuleProcessorData* aData)

>   AddImportantRules(lastDocRN, lastUserRN);   //doc
>   AddImportantRules(lastOvrRN, lastDocRN);    //ovr

Can't these two be condensed?

>-  // There should be no imporant rules in the preshint level
>+  // There should be no imporant rules in the preshint or HTMLpreshint level

Fix spelling of "important"?

>   AddImportantRules(lastUserRN, lastAgentRN); //user
>   AddImportantRules(lastAgentRN, nsnull);     //agent

Both user->doc and agent->user include pres-hint rules.  I'd rather you not
walk them, but instead have a DEBUG-only function that asserts that
GetImportantRule is always null.

I didn't really review the ordering stuff, since I figured one person tracing
through all the necessary ordering stuff is enough.

r=dbaron with those changes

If the assertion in my previous comment about one rule processor per level is
correct, then perhaps it's worth cleaning that up in a followup patch (and
perhaps making the way stylesheets are combined into a CSSRuleProcessor a
little less side-effect-ish)?

Comment 32

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

(And I think that followup patch would allow me to finish my next patch for bug
239008.)

Comment 33

[Boris Zbarsky [:bzbarsky]]

> Is |*aInstancePtrResult| guaranteed to be null?

Good point.  I'll check.

> Can't these two be condensed?

Hmm.. probably.  Again, I'll check.

I'm not sure I'll get to fixing this up before the 14th... if I don't, I'll pick
this up when I get back in July.

Comment 34

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

note that nsCOMPtrs will always be nulled before passed as return-value-pointer.
This might not however be the case with rawpointers which means that you
shouldn't rely on it in the general case.

Comment 35

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

(In reply to comment #33)
> > Is |*aInstancePtrResult| guaranteed to be null?
> Good point.  I'll check.

It was really more of a rhetorical question.  Our convention is that it isn't,
so you need to assign null before swapping.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Yeah, that's what I've done in the updated patch (assign null up front in that
function).

Comment 37

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153218 [details] [diff] [review]
Updated to comments

David, would you give this a last once-over?

Comment 39

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

Comment on attachment 153218 [details] [diff] [review]
Updated to comments

Perhaps AssertNoImportantRules should even assert if something QIs to
nsICSSStyleRule, although then we'd need a different function for style
attribute rules once that's it's own level.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Attachment: Assert that preshints aren't CSS rules

Comment 41

[Boris Zbarsky [:bzbarsky]]

Fixed (again, dammit :) ).