Bug 112623

Created attachment 193650 [details] Empty-viewbox content should not be visible.

Currently, size-related methods seem to simply ignore empty viewBoxes. Looks like we'll just have to explicitly check for this case when painting. Unfortunately, this exposes an unrelated problem: there is no way to distinguish an empty, correctly specified viewBox from an invalid/unparsable attribute (which also results in an empty viewBox). This causes a couple Hixie tests to fail - we should open a separate bug for it.

Created attachment 193838 [details] Patch

Comment on attachment 193838 [details] Patch Attachment 193838 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17120586 New failing tests: html5lib/generated/run-tests11-write.html html5lib/generated/run-tests11-data.html

Comment on attachment 193838 [details] Patch Attachment 193838 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17221380 New failing tests: html5lib/generated/run-tests11-write.html html5lib/generated/run-tests11-data.html

(In reply to comment #5) > (From update of attachment 193838 [details]) > Attachment 193838 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-commit-queue.appspot.com/results/17221380 > > New failing tests: > html5lib/generated/run-tests11-write.html > html5lib/generated/run-tests11-data.html Looking at the failing tests, I'm starting to think that removing an invalid viewBox attribute might not be the best idea (unexpected, possibly causing other failures). A general fix for https://bugs.webkit.org/show_bug.cgi?id=112630 is needed instead.

Comment on attachment 193838 [details] Patch R=me. Looks fine. I reset the CQ bit to get past the flakey tests.

Comment on attachment 193838 [details] Patch Attachment 193838 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17242103 New failing tests: html5lib/generated/run-tests11-write.html html5lib/generated/run-tests11-data.html

(In reply to comment #7) > (From update of attachment 193838 [details]) > R=me. Looks fine. I reset the CQ bit to get past the flakey tests. Yeah, unfortunately those are real failures triggered by the removal of invalid viewBox attributes. We can either a) update the expectations for the failing tests and commit in this form (I'm somewhat wary of this now, thinking that messing with the attribute may have unintended consequences) b) drop the attribute removal bit - this will cause a couple of other tests to fail <- marks these as such, pending a fix for https://bugs.webkit.org/show_bug.cgi?id=112630 c) fix https://bugs.webkit.org/show_bug.cgi?id=112630 first, then re-factor this patch to detect whether the viewBox is valid without removing the attribute Looking at what it would take for c) now.

Comment on attachment 193838 [details] Patch Attachment 193838 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17252075 New failing tests: html5lib/generated/run-tests11-write.html html5lib/generated/run-tests11-data.html

(In reply to comment #9) > (In reply to comment #7) > > (From update of attachment 193838 [details] [details]) > > R=me. Looks fine. I reset the CQ bit to get past the flakey tests. > > Yeah, unfortunately those are real failures triggered by the removal of invalid viewBox attributes. We can either > > a) update the expectations for the failing tests and commit in this form (I'm somewhat wary of this now, thinking that messing with the attribute may have unintended consequences) > > b) drop the attribute removal bit - this will cause a couple of other tests to fail <- marks these as such, pending a fix for https://bugs.webkit.org/show_bug.cgi?id=112630 > > c) fix https://bugs.webkit.org/show_bug.cgi?id=112630 first, then re-factor this patch to detect whether the viewBox is valid without removing the attribute > > Looking at what it would take for c) now. My bad - I should have looked more carefully at the test and thought some more. I'll clear the R while you investigate. Removing the attribute is an issue if the person subsequently sets it, right? But then you need some way of saying a rect is "invalid" rather than empty, which is something that WebKit doesn't have yet.

(In reply to comment #11) > Removing the attribute is an issue if the person subsequently sets it, right? The problem is that currently invalid values are allowed. For example, e.setAttribute('width', 'mwahaha') does report a parsing error, but subsequently e.getAttribute('width') still returns 'mwahaha'. So we can't just drop invalid values under these semantics... > But then you need some way of saying a rect is "invalid" rather than empty, which is something that WebKit doesn't have yet. Yes, there's no such thing currently. I was thinking of adding an 'isValid' field to SVGSynchronizableAnimatedProperty which defaults to false and gets reset to true in the (macroed) set##UpperProperty##BaseValue() setter. Then we should only call the setter if the value is successfully parsed (most parsers seem to already return a success result) and later we can query it via some other macroed getter (LowerProperty##IsValid()?) when using the attribute. This would allow us to detect the "attribute present but invalid" case.

Created attachment 194061 [details] Patch

Comment on attachment 194061 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194061&action=review LGTM with a few small changes. > Source/WebCore/ChangeLog:17 > + macroed getter (LowerProperty##IsValid()). Can we use an emum here instead of a bool? > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:280 > + if (svg->hasAttribute(SVGNames::viewBoxAttr) && svg->viewBoxIsValid() && svg->viewBox().isEmpty()) This same set of checks is called many times in this patch. Can we roll it all into "viewBoxIsValid()"? > Source/WebCore/rendering/svg/RenderSVGViewportContainer.cpp:154 > + Extra space.

Comment on attachment 194061 [details] Patch Thanks Philip. View in context: https://bugs.webkit.org/attachment.cgi?id=194061&action=review >> Source/WebCore/ChangeLog:17 >> + macroed getter (LowerProperty##IsValid()). > > Can we use an emum here instead of a bool? Not sure I follow this... do you mean internally? This seems consistent with other isXYZ() methods return type. Besides, isn't bool the ultimate enum? :) >> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:280 >> + if (svg->hasAttribute(SVGNames::viewBoxAttr) && svg->viewBoxIsValid() && svg->viewBox().isEmpty()) > > This same set of checks is called many times in this patch. Can we roll it all into "viewBoxIsValid()"? Yes, but unfortunately on different/unrelated types. There's minimal duplication for SVGSVGElement so maybe we can add a helper for this class alone.

Comment on attachment 194061 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194061&action=review >>> Source/WebCore/ChangeLog:17 >>> + macroed getter (LowerProperty##IsValid()). >> >> Can we use an emum here instead of a bool? > > Not sure I follow this... do you mean internally? This seems consistent with other isXYZ() methods return type. Besides, isn't bool the ultimate enum? :) I should have been clearer. The "validValue' bool can be made into an enum. An example is http://trac.webkit.org/browser/trunk/Source/WebCore/svg/SVGLocatable.h#L43.

(In reply to comment #16) > (From update of attachment 194061 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194061&action=review > > >>> Source/WebCore/ChangeLog:17 > >>> + macroed getter (LowerProperty##IsValid()). > >> > >> Can we use an emum here instead of a bool? > > > > Not sure I follow this... do you mean internally? This seems consistent with other isXYZ() methods return type. Besides, isn't bool the ultimate enum? :) > > I should have been clearer. The "validValue' bool can be made into an enum. An example is http://trac.webkit.org/browser/trunk/Source/WebCore/svg/SVGLocatable.h#L43. I'm not convinced it's a good idea in this case: enums are preferred to bools as input params if the argument is likely to be constant (http://www.webkit.org/coding/coding-style.html#names-enum-to-bool). That's not the case here: callers would have to always convert a bool (the parse result) to this enum - which pretty much defeats the readability purpose.

(In reply to comment #17) > (In reply to comment #16) > > (From update of attachment 194061 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=194061&action=review > > > > >>> Source/WebCore/ChangeLog:17 > > >>> + macroed getter (LowerProperty##IsValid()). > > >> > > >> Can we use an emum here instead of a bool? > > > > > > Not sure I follow this... do you mean internally? This seems consistent with other isXYZ() methods return type. Besides, isn't bool the ultimate enum? :) > > > > I should have been clearer. The "validValue' bool can be made into an enum. An example is http://trac.webkit.org/browser/trunk/Source/WebCore/svg/SVGLocatable.h#L43. > > I'm not convinced it's a good idea in this case: enums are preferred to bools as input params if the argument is likely to be constant (http://www.webkit.org/coding/coding-style.html#names-enum-to-bool). That's not the case here: callers would have to always convert a bool (the parse result) to this enum - which pretty much defeats the readability purpose. Sounds good to me; that's a reasonable argument.

Created attachment 194112 [details] Patch Updated per comments.

Comment on attachment 194112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194112&action=review > Source/WebCore/svg/SVGURIReference.h:57 > + virtual void setHrefBaseValue(const String&, const bool validValue = true) = 0; One last question: If I set an invalid CSS value, it gets thrown away. For example: document.body.style.background = "we will not go to space today"; console.log(document.body.style.background); // prints "" This makes me think we are not doing the right thing to pass both a string and a validValue here. Can you explain this discrepancy?

(In reply to comment #20) > (From update of attachment 194112 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194112&action=review > > > Source/WebCore/svg/SVGURIReference.h:57 > > + virtual void setHrefBaseValue(const String&, const bool validValue = true) = 0; > > One last question: If I set an invalid CSS value, it gets thrown away. For example: > document.body.style.background = "we will not go to space today"; > console.log(document.body.style.background); // prints "" > > This makes me think we are not doing the right thing to pass both a string and a validValue here. Can you explain this discrepancy? That's a good point. This approach is based on the assumption that the current SVG attribute behavior is appropriate - hence the need to differentiate the "set-but-invalid" state. Personally I find CSS' property handling to make more sense, but I suspect the discrepancy is not arbitrary (maybe someone more familiar with the spec can clarify): 1) simply ignoring invalid viewBox values triggers failures in a html5lib test 2) FF also allows random attribute values: [17:16:00.569] r.getAttribute('width') [17:16:00.572] "100%" [17:16:04.449] r.setAttribute('width', 'mwahaha') [17:16:04.452] undefined [17:16:04.452] Unexpected value mwahaha parsing width attribute. @ Web Console:1 [17:16:08.121] r.getAttribute('width') [17:16:08.125] "mwahaha"

(In reply to comment #21) > 2) FF also allows random attribute values: It looks like the same behavior is not true for Firefox's regular CSS values: -- [14:20:42.051] document.body.style.backgroundImage = "pasta batman" [14:20:42.052] "pasta batman" [14:20:42.052] Error in parsing value for 'background-image'. Declaration dropped. @ http://cuteoverload.com/ [14:20:44.403] document.body.style.backgroundImage [14:20:44.405] ""

> Personally I find CSS' property handling to make more sense, but I suspect the discrepancy is not arbitrary (maybe someone more familiar with the spec can clarify): I guess the answer is here: http://www.w3.org/TR/SVG/svgdom.html#InvalidValues "If a script sets a DOM attribute to an invalid value [...], unless this specification indicates otherwise, no exception shall be raised on setting, but the given document fragment shall become technically in error" The implication is that invalid attribute values are sticky.

(In reply to comment #23) > > Personally I find CSS' property handling to make more sense, but I suspect the discrepancy is not arbitrary (maybe someone more familiar with the spec can clarify): > > I guess the answer is here: http://www.w3.org/TR/SVG/svgdom.html#InvalidValues > > "If a script sets a DOM attribute to an invalid value [...], unless this specification indicates otherwise, no exception shall be raised on setting, but the given document fragment shall become technically in error" > > The implication is that invalid attribute values are sticky. You are right. I didn't realize the difference between dom attributes and style; attributes should be "sticky" (and they are on regular DOM attributes too). I think this patch is going in the right direction now. Please add a test of setting and reading a completely nonsense string so we have test coverage. R=me

Comment on attachment 194112 [details] Patch R=me with an additional test

Created attachment 194291 [details] Patch for landing

Comment on attachment 194291 [details] Patch for landing Clearing flags on attachment: 194291 Committed r146495: <http://trac.webkit.org/changeset/146495>

All reviewed patches have been landed. Closing bug.