238327 - implement <svg:style>

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

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

Patch comming up

Comment 1

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

Attachment: svg-changes

Comment 2

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

Attachment: changes to non-svg code

Comment 3

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

Peter: could you have a look at the transformiix changes? They should be very
straitforward.

Comment 4

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

Attachment: testcase

Note that the new element requires a type="text/css" attribute, as required by
spec. The adobe plugin also requires the attribute so I think it should be fine
that we follow the spec.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 144529 [details] [diff] [review]
changes to non-svg code

r+sr=jst

Comment 6

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

Attachment: svg-changes

Forgot to diff the mozilla/dom directory

Comment 7

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

Attachment: changes to non-svg code

Same here, exact same patch as previous but including changes to mozilla/dom
directory

Comment 8

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

Comment on attachment 144540 [details] [diff] [review]
changes to non-svg code

Ack, didn't see that the previous version was already reviewed (yay, that was
fast!).

Anyway, this patch is exactly the same as previous, i've just appended the
dom-changes.

Jst, could you r/sr those too? I marked peterv as reviewer so that he can take
a look at the transformiix changes.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 144540 [details] [diff] [review]
changes to non-svg code

sr=jst

Comment 10

[Alex Fritze]

Comment on attachment 144539 [details] [diff] [review]
svg-changes

r=afri

>Index: content/shared/public/nsSVGAtomList.h
>===================================================================
>+SVG_ATOM(space, "sacee")

Should be "space".

Also we should probably add some code to nsCSSFrameConstructor to not build
frames for <style> (and <script>) elements. I can put that into the <script>
patch if you want.

Comment 11

[Alex Fritze]

Comment on attachment 144539 [details] [diff] [review]
svg-changes


>Index: content/svg/content/src/nsSVGStyleElement.cpp
>===================================================================
>+ * Contributor(s):
>+ *    Alex Fritze <[e-mail]> (original author)

You should also remove me from the contributor list here.

Comment 12

[Hixie (not reading bugmail)]

> Also we should probably add some code to nsCSSFrameConstructor to not build
> frames for <style> (and <script>) elements. I can put that into the <script>
> patch if you want.

For HTML, that should be covered by ua.css (which sets those elements to 
display:none); for SVG, that should be covered by the "foreign elements in an 
SVG context have no frames" stuff. No?

Comment 13

[Alex Fritze]

(In reply to comment #12)
> > Also we should probably add some code to nsCSSFrameConstructor to not build
> > frames for <style> (and <script>) elements. I can put that into the <script>
> > patch if you want.
> 
> For HTML, that should be covered by ua.css (which sets those elements to 
> display:none); for SVG, that should be covered by the "foreign elements in an 
> SVG context have no frames" stuff. No?

Well, this is about native <svg:style> & <svg:script>, so doesn't fall under
either of these cases afaics. We could do something similar to HTML & set these
elements as display:none in the svg useragent css file, but even then we need
some hacking in nsCSSFrameCTOR to correctly handle 'display:none' for SVG.

Comment 14

[Hixie (not reading bugmail)]

>> for SVG, that should be covered by the "foreign elements in an 
>> SVG context have no frames" stuff. No?
> 
> Well, this is about native <svg:style> & <svg:script>, so doesn't fall under
> either of these cases afaics.

Wouldn't svg:script count as a foreign element, unless it has an explicit 
mapping to an SVG primitive? I don't know how our SVG frame constructor works, 
so I'll just shut up now. :-)

Comment 15

[Alex Fritze]

(In reply to comment #14)

> Wouldn't svg:script count as a foreign element, unless it has an explicit 
> mapping to an SVG primitive? 

No, we build generic svg container frames for unknown SVG elements. The reason
is to get XBL-bindings working. Consider <binding id="foo"
extends="svg:generic">. Here the frame constructor will generate an svg
container frame because it sees the unknown svg tag 'generic'.

We build frames for 'real' foreign content btw. But that's ok in my opinion
since for valid svg docs there shouldn't be any foreign content in an svg
context anyway (unless of course in <svg:foreignObject>).

Comment 16

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

Adding 'svg|script, svg|style { display: none }' to ua.css, and making the
css-frameconstructor honor that seems like the right solution to me. That way
you can also do 'svg|circle { display:none }' and have all circles hidden, which
i think would be kind'a cool

Comment 17

[Alex Fritze]

(In reply to comment #16)
> Adding 'svg|script, svg|style { display: none }' to ua.css, and making the
> css-frameconstructor honor that seems like the right solution to me. That way
> you can also do 'svg|circle { display:none }' and have all circles hidden, which
> i think would be kind'a cool

layout/svg/base/src/svg.css gets pulled into ua.css, so that'll be the right
place. Thinking about it, the frame constructor might already be honouring
display:none, even for svg content.

Comment 18

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

display:none works fine, i.e. the element is not rendered (and I would assume no
frame is created for it). Which is as it should be according to:

http://www.w3.org/TR/SVG10/painting.html#DisplayProperty

So i'll add the following to svg.css:

style {
  display: none;
}

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 144540 [details] [diff] [review]
changes to non-svg code

We're a bit inconsistent between normal element creation and XSLT element
creation, since XSLT checks by QIing to nsIStyleSheetLinkingElement and doesn't
look at the tag at all, maybe somehing we should think about synching.
IsContentOfType(eSTYLE)?

Comment 20

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

jst didn't want an eSTYLE (which i sort of understand). We did come up with an
idea that should compleatly remove the need for the sink/resulthandler to know
about style-elements though. I'll file a separate bug on that.

Comment 21

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

Checked in. Thanks for reviews!

Comment 22

[Scooter Morris]

I think that comment #10 below got overlooked.  The "space" atom is still
"sacee" in the checked in code.

Comment 23

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

Thanks! Fixed that and the licenceerror from comment 11

Comment 24

[Alex Fritze]

*** Bug 236044 has been marked as a duplicate of this bug. ***