308928 - Invalid input for the canvas fillStyle/strokeStyle throws an exception but should be ignored

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Kamiel Martinet]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

When the fillStyle property gets an invalid colour/pattern/gradient assigned,
this results in a NS_ERROR_INVALID_ARG exception being thrown while according to
the specification these should be ignored:

'If the value is a string but is not a valid colour, or is neither a string, a
CanvasGradient, nor a CanvasPattern, then it must be ignored, and the attribute
must retain its previous value.'

Reproducible: Always

Steps to Reproduce:
1. Assign invalid string value to fillStyle (rgb(100%,0,0))

Actual Results:  
The scripts halts because of the exeception while the original value for
fillStyle/strokeStyle should be retained

Expected Results:  
Script should continue execution

Comment 1

[Kamiel Martinet]

Attachment: Testcase

This should draw a black rectangle on the canvas but doesn't.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hrm.  This may be a spec issue; I think that it should throw an exception -- silent failure sucks in so many ways, that I can't think of a good reason to specify it.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

CSS parsing normally specifies swallowing errors, mainly as a forward-compatibility mechanism.  That said, it doesn't necessarily make sense to apply that to the DOM as well.

That said, either way, the spec could be clearer, but I think it says that it shouldn't throw, and I expect that's what Ian intended.  If nobody else (Safari, Opera) throws, we should match.

This bug is mentioned in http://annevankesteren.nl/2006/09/moving-square .

Comment 4

[Nickolay_Ponomarev]

http://whatwg.org/specs/web-apps/current-work/#fillstyle
"On setting, strings should be parsed as CSS <color> values. [CSS3COLOR] If the value is a string but is not a valid colour, or is neither a string, a CanvasGradient, nor a CanvasPattern, then it must be ignored, and the attribute must retain its previous value."

I think it's pretty clear. Opera doesn't throw.

Comment 5

[Nickolay_Ponomarev]

Attachment: patch

Comment 6

[Nickolay_Ponomarev]

Or perhaps we want to report the error to the Error console?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We shouldn't have an NS_WARNING because content is broken -- they're for problems in the code.

Comment 8

[Nickolay_Ponomarev]

Attachment: patch

OK, what about this? (Ignore the nsCanvasRenderingContext2D::StyleColorToString part, it's for another bug.)

I don't like that reported warnings are too unspecific in this and other cases, but I wouldn't like to fix it now in this bug.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 241651 [details] [diff] [review]
patch

I'm not excited about the changing of the returned format for colors, but I guess it /is/ in the spec, in excruciating detail. :)

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Actually, please don't land the color getter parts as part of this patch -- they should be dealt with in bug 329593.  The error handling stuff is fine, though.

Comment 11

[Nickolay_Ponomarev]

Attachment: patch for checkin

Removed the irrelevant part of the patch and fixed a dump typo.

Comment 12

[Nickolay_Ponomarev]

Attachment: another testcase

Clearer green/red testcase, which also includes more tests.

Comment 13

[(not currently active) Ted Mielczarek]

Checked in on trunk:

Checking in content/canvas/src/nsCanvasRenderingContext2D.cpp;
/cvsroot/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp,v  <--  nsCanvasRenderingContext2D.cpp
new revision: 1.73; previous revision: 1.72
done
Checking in dom/locales/en-US/chrome/dom/dom.properties;
/cvsroot/mozilla/dom/locales/en-US/chrome/dom/dom.properties,v  <--  dom.properties
new revision: 1.6; previous revision: 1.5
done
Checking in layout/style/nsCSSParser.cpp;
/cvsroot/mozilla/layout/style/nsCSSParser.cpp,v  <--  nsCSSParser.cpp
new revision: 3.329; previous revision: 3.328
done