Created attachment 68118 [details] Testcase See attached testcase, which should not have gray text. Changing "eee" to things like "888" or "8ee" indicates that the color parser is just allowing the page to drop the '#' in front of a CSS RGB color in this case, which is not allowed by the SVG specification.
Created attachment 70973 [details] First attempt
Comment on attachment 70973 [details] First attempt I think the quirks mode is a general problem for SVG. At least I saw some examples that looked like they fail because of quirks parsing. Can't we run the complete CSS parser in strict mode somehow? If not, don't use a boolean but an enum CSSStrictMode CSSQuirksMode.
Moin Dirk, (In reply to comment #2) > (From update of attachment 70973 [details]) > I think the quirks mode is a general problem for SVG. At least I saw some examples that looked like they fail because of quirks parsing. Can't we run the complete CSS parser in strict mode somehow? Hmm, I assumed we had no control, but I guess we do. Since the fill attribute parsing fails in this case I suspect MappedAttribute or some related class does not set strict mode in this (SVG) case. Will look at it this evening. > If not, don't use a boolean but an enum CSSStrictMode CSSQuirksMode. Could be more readable, agreed, but I'll invest why strict mode is not set first. Cheers, Rob.
(In reply to comment #3) > Moin Dirk, > (In reply to comment #2) > > (From update of attachment 70973 [details] [details]) > > I think the quirks mode is a general problem for SVG. At least I saw some examples that looked like they fail because of quirks parsing. Can't we run the complete CSS parser in strict mode somehow? > > Hmm, I assumed we had no control, but I guess we do. Since the fill attribute parsing fails in this case > I suspect MappedAttribute or some related class does not set strict mode in this (SVG) case. Will look at it > this evening. Seems to be related to this in WebCore/dom/StyledElement.cpp: decl->setStrictParsing(false); // Mapped attributes are just always quirky. Cheers, Rob.
Created attachment 71695 [details] Patch
Attachment 71695 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/css/CSSParser.cpp:4038: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/css/CSSMutableStyleDeclaration.cpp:629: parseMode_ is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 2 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.
The approach with the enum offers more fine-grained control for SVG than we had before. Using it bug 15012 is easy to fix for instance. I'll await the reviewing now as I know there are a few rough edges. For instance the number of call sites could be less if we would keep CSSParser(bool) overload. Also we may need htmlStrict() vs. strict() in CSSParser. Just some thoughts, but again I think this is the only way to fix bugs like 15012. Cheers, Rob.
Comment on attachment 71695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71695&action=review Do we need pixel tests for this patch? If so, you should have green rects as result images. > WebCore/css/CSSMutableStyleDeclaration.h:136 > + void setStrictParsing(bool b) { m_parseMode = b ? HTMLStrictMode : HTMLQuirksMode; } this should take an enum. We talked about it on IRC. You fear that the patch will be to big if you change it in a row, right? But you should mention it in the ChangeLog, add a FIXME that points to the bug and even better, have a followup ready. > 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?
Attachment 71695 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4698082
Comment on attachment 71695 [details] Patch 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?
Attachment 71695 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4809014
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.
Created attachment 71843 [details] Patch
Attachment 71843 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/css/CSSParser.cpp:4038: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 1 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 71843 [details] Patch SVG is part of HTML now. So it seems we should use HTML parsing quirks when in an HTML document. I'm not sure I understand this change.
Uh.... no. SVG is not part of HTML, and in particular the syntax of SVG attributes is defined by SVG, not by HTML.
(In reply to comment #16) > Uh.... no. SVG is not part of HTML, and in particular the syntax of SVG attributes is defined by SVG, not by HTML. My understanding Boris it that SVG is "part of" HTML5. However you are correct that semantics of the individual elements (including attribute parsing) are left to the SVG specification: http://www.whatwg.org/specs/web-apps/current-work/multipage/the-map-element.html#svg-0 So if I have svg content in a HTML document and wish to have a color specified how many different color parsing rules are in effect? Obviously with this change, setting a color outside of the SVG content (which may be inherited by the SVG content using currentColor) is governed by HTML quirks. What about setting via style="", or via svgElement.fill= or svgElement.setAttribute("fill", quirkyColor); I assume that style is governed by whatever CSS rules quirks are in effect for the document? Or is setting style= dependent on the element type? (in which case that seems strange). And I assume setting fill= or via setAttribute should be identical and governed by SVG's (non-quriky) color parser?
(In reply to comment #17) > Obviously with this change, setting a color outside of the SVG content (which may be inherited by the SVG content using currentColor) is governed by HTML quirks. My sentence structure above was a bit odd. "Obviously with this change" makes no sense with how I ended that sentence and can be ignored. :)
> What about setting via style="" Probably needs to be defined in HTML5, which is what defines quirks mode. But yes, should be the same for all CSS on the page, imo. > or via svgElement.fill= or svgElement.setAttribute("fill", quirkyColor); Governed by the SVG spec.
So I think this patch is OK. But I would like to see a test (which probably doesn't need to be a pixel test, just a text-based test using some fancy JS) which tests *all* the ways we can think of to parse colors in SVG. As I discussed above with Boris, we need to understand which ones follow HTML5 rules vs. CSS rules vs. SVG rules.
Rob, any news on this patch? Have a look at 54800/55119, these are related.
This blocks the old bug 15012 - getPresentationAttribute doesn't support SVGColor objects at present, it returns CSSPrimitiveValues, works fine for fill/stroke==SVGPaint.
ping! I hope Rob has another minute :-)
Comment on attachment 71843 [details] Patch r- because of Erics comments about a more detailed test and Nikos comment about the possible relation to other bugs. Maybe Rob has more time to look at it next week. Welcome to RIM Rob!
Rob, can you look again? :-)
Either me, or someone here in SF will look at this patch soon. As a first step I would replace the strict mode boolean by an enum: enum CSSParseMode { StrictMode, QuirksMode, }; This shouldn't cause any difference on behavior. The next step could be to turn the enum in something like that: enum CSSParseMode { StrictMode = 1 << 1, QuirksMode = 1 << 2, SVGPresentationMode = 1 << 3 }; Than we can still differ between quirks and strict mode, but it still allows us to add more logic into the CSSParser just for SVGs presentation attributes - something that we can use for 'transform' for instance. Any comments?
Created attachment 134911 [details] Patch
Comment on attachment 134911 [details] Patch LGTM.
Committed r112769: <http://trac.webkit.org/changeset/112769>