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

[bugzilla-daemon at webkit.org]

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





--- Comment #10 from Nikolas Zimmermann <zimmermann at kde.org>  2010-10-25 03:13:58 PST ---
(From update of attachment 71695)
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...

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

s/misses/missing/; s/stylesheet/stylesheets/

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

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

s/an SVG/a SVG/

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

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

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

Ditto.

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

Ditto.

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

s/parseMode_/useParseMode/

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

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

> WebCore/css/CSSStyleSheet.cpp:42
> +    , m_parseMode(!parentSheet || parentSheet->parseMode())

See above, this is wrong.

> WebCore/css/CSSStyleSheet.cpp:63
> +    , m_parseMode(!ownerRule || ownerRule->parseMode())

See above, this is wrong.

> WebCore/css/CSSStyleSheet.cpp:187
> +    setParseMode(strict ? HTMLStrictMode : HTMLQuirksMode);
> +    CSSParser p(strict ? HTMLStrictMode : HTMLQuirksMode);

Same problem, what about SVG here?

> WebCore/css/MediaList.cpp:123
> -    CSSParser p(true);
> +    CSSParser p;

You're disabling strict mode here?

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

Ditto.

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

Ditto.

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

> WebCore/dom/Element.cpp:1554
> +    CSSParser p(strictParsing ? HTMLStrictMode : HTMLQuirksMode);

Are we sure this is never used for SVG?

> WebCore/dom/Node.cpp:1560
> +    CSSParser p(strictParsing ? HTMLStrictMode : HTMLQuirksMode);

Are we sure this is never used for SVG?

> WebCore/dom/Node.cpp:1607
> +    CSSParser p(strictParsing ? HTMLStrictMode : HTMLQuirksMode);

Are we sure this is never used for SVG?

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

> WebCore/inspector/InspectorStyleSheet.cpp:489
> +    m_pageStyleSheet->parseString(text, m_pageStyleSheet->parseMode() == HTMLStrictMode);

Ditto, what about SVG here?

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