Bug 112623 – [SVG] Suppress painting when an empty viewBox is specified

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Florin Malita]

https://code.google.com/p/chromium/issues/detail?id=177148

Per spec, a zero width/height viewBox attribute should suppress rendering the element (http://www.w3.org/TR/SVG/coords.html#ViewBoxAttribute):

  A negative value for <width> or <height> is an error (see Error processing). A value of zero disables rendering of the element.

[Florin Malita]

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

[Florin Malita]

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.

[Florin Malita]

Created attachment 193838 [details]
Patch

[WebKit Review Bot]

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

[Build Bot]

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

[Florin Malita]

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

[Stephen Chenney]

Comment on attachment 193838 [details]
Patch

R=me. Looks fine. I reset the CQ bit to get past the flakey tests.

[Build Bot]

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

[Florin Malita]

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

[Build Bot]

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

[Stephen Chenney]

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

[Florin Malita]

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

[Florin Malita]

Created attachment 194061 [details]
Patch

[Philip Rogers]

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.

[Florin Malita]

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.

[Philip Rogers]

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.

[Florin Malita]

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

[Philip Rogers]

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

[Florin Malita]

Created attachment 194112 [details]
Patch

Updated per comments.

[Philip Rogers]

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?

[Florin Malita]

(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"

[Philip Rogers]

(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] ""

[Florin Malita]

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

[Philip Rogers]

(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

[Philip Rogers]

Comment on attachment 194112 [details]
Patch

R=me with an additional test

[Florin Malita]

Created attachment 194291 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 194291 [details]
Patch for landing

Clearing flags on attachment: 194291

Committed r146495: <http://trac.webkit.org/changeset/146495>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.