[Webkit-unassigned] [Bug 46112] Colors seem to be parsed using HTML quirks in SVG attributes

Tue Oct 26 00:04:14 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=46112

--- Comment #12 from Rob Buis <rwlbuis at gmail.com>  2010-10-26 00:04:13 PST ---
Hi Niko,

Thanks for the extensive review! I am going to address a subset first with a patch and comments, due
to lack of time and because some of the issues below simply need more thinking.
To be clear, the original patch at least caused no regressions.

(In reply to comment #10)
> (From update of attachment 71695 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71695&action=review
> 
> This needs a lot more test coverage, especially the non CSSParser changes. Each function that uses CSSParser, eg. in CanvasRenderingContext2d::setFont.., Node::querySelector, needs to be tested in a SVG and HTML environment, to be sure the whole parsing is context dependant (no HTMLStrict/QuirksMode for example, when using querySelector on a SVG Element). This is a probably complex todo...

Guess there is no way around it.

> > LayoutTests/ChangeLog:9
> > +        like eee(misses #) are not accepted. Test for strict parsing in SVG stylesheet,
> 
> s/misses/missing/; s/stylesheet/stylesheets/

Fixed.

> > LayoutTests/svg/custom/strict-color-parsing.svg:4
> > +<?xml version="1.0" encoding="UTF-8"?>
> > +<svg xmlns="http://www.w3.org/2000/svg">
> > +<text fill="eee">This should not be gray</text>
> > +</svg>
> 
> Hm, I don't really like the tests, I think it would be much nicer, if we had a single testcase for the color parsing, and using three different rects, that should not be gray.
> It's aso easier to spot in the DRT dumps, as for rects the color gets dumped, unlike for text.

I am not sure it can be one test since the stylesheet entry will override the fill PA. Have a look
at the new tests, using text was indeed not convenient.

> > WebCore/ChangeLog:8
> > +        Make CSS parsing more fine-grained for SVG by adding an SVG strict mode.
> 
> s/an SVG/a SVG/

Fixed.

> You should describe here, what's the difference between the individual parsing modes.

Later, once we have thought the enum out :)

> > WebCore/css/CSSMutableStyleDeclaration.cpp:54
> > +    , m_parseMode(!parent || parent->parseMode())
> 
> This doesn't work. !parent is a bool. You want to use m_parseMode(parent ? parent->parseMode() : HTMLQuirksMode)

Agreed, but I think it should be HTMLStrictMode instead of  HTMLQuirksMode above.
Fixed.

> > WebCore/css/CSSMutableStyleDeclaration.cpp:66
> > +    , m_parseMode(!parent || parent->parseMode())
> 
> Ditto.
Fixed.

> > WebCore/css/CSSMutableStyleDeclaration.cpp:79
> > +    , m_parseMode(!parent || parent->parseMode())
> 
> Ditto.
Fixed.

> > WebCore/css/CSSMutableStyleDeclaration.cpp:629
> > +    CSSParseMode parseMode_ = parseMode();
> 
> s/parseMode_/useParseMode/

Even causes compile failures.
Fixed.

> >> WebCore/css/CSSParser.h:218
> >> +        inline bool strict() const { return m_parseMode == HTMLStrictMode; }
> > 
> > We have 2 strict modes, HTMLStrictMode and SVGStrictMode, the function name should be self-explanatory. Also, what does HTMLStrictMode mean for SVGStrictMode? Does HTMLStrictMode include SVGStrictMode? If yes we should rethink the enum entries. Or are they exclusive?
> 
> Good point Dirk...
Comes down to more thinking about the enum. Maybe on IRC...
One loose idea is to add htmlStrict and strict, where the latter tests both kinds of strictness, svg and html. I really think SVGStrictMode and HTMLStrictMode are the unit processing (10px vs. 10),
that for SVG colors SVGColor should be created instead of CSSPrimitiveValue and that color processing
for SVG should be strict too.

> > WebCore/css/CSSStyleSelector.cpp:653
> > +        CSSParser(m_checker.m_strictParsing ? HTMLStrictMode : HTMLQuirksMode).parsePropertyWithResolvedVariables(current.id(), current.isImportant(), newDecl, &resolvedValueList);
> 
> This seems strange as well, it's correct. But are we sure we won't need any SVGStrictMode here?
> It's the same question as Dirk has, does HTMLStrictMode include SVGStrictMode... we need to rethink the enum..
See above.

> > WebCore/css/CSSStyleSheet.cpp:42
> > +    , m_parseMode(!parentSheet || parentSheet->parseMode())
> 
> See above, this is wrong.
Fixed.

> > WebCore/css/CSSStyleSheet.cpp:63
> > +    , m_parseMode(!ownerRule || ownerRule->parseMode())
> 
> See above, this is wrong.
Fixed.

> > WebCore/css/CSSStyleSheet.cpp:187
> > +    setParseMode(strict ? HTMLStrictMode : HTMLQuirksMode);
> > +    CSSParser p(strict ? HTMLStrictMode : HTMLQuirksMode);
> 
> Same problem, what about SVG here?

I think it works now, but haven't thought it through.

> > WebCore/css/MediaList.cpp:123
> > -    CSSParser p(true);
> > +    CSSParser p;
> 
> You're disabling strict mode here?
No, using the default of HTMLStrictMode, but I made that explicit.
Fixed.

> > WebCore/css/MediaList.cpp:177
> > +    CSSParser p;
> 
> Ditto.
Fixed.

> > WebCore/css/MediaList.cpp:229
> > +    CSSParser p;
> 
> Ditto.
Fixed.

> > WebCore/css/StyleBase.h:75
> > +        CSSParseMode parseMode() const { return !m_parent ? HTMLStrictMode : m_parent->parseMode(); }
> 
> You should reverse the check: return m_parent ? m_parent-> ...
Fixed.

> > WebCore/dom/Element.cpp:1554
> > +    CSSParser p(strictParsing ? HTMLStrictMode : HTMLQuirksMode);
> 
> Are we sure this is never used for SVG?
It probably is, as you said needs tests, so still open.

> > WebCore/dom/StyledElement.cpp:135
> > +#if ENABLE(SVG)
> > +    if (isSVGElement())
> > +        m_inlineStyleDecl->setParseMode(SVGStrictMode);
> > +    else
> > +#endif
> 
> Does this really belong here: If the function is virtual (I did not check), then we should reimplement it in SVGStyledElement / SVGElement.
> 
> > WebCore/dom/StyledElement.cpp:412
> > +#if ENABLE(SVG)
> > +    if (isSVGElement())
> > +        decl->setParseMode(SVGStrictMode);
> > +    else
> > +#endif
> > +    decl->setParseMode(HTMLQuirksMode);
> 
> Ditto.
Unfortunately these are not virtual. Maybe the penalty is not that high to introduce it?

> > WebCore/inspector/InspectorStyleSheet.cpp:489
> > +    m_pageStyleSheet->parseString(text, m_pageStyleSheet->parseMode() == HTMLStrictMode);
> 
> Ditto, what about SVG here?

Needs to be tested as well.
Patch coming up.
Cheers,

Rob.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.