1281800 - ctx.fillText(text, x, y, null) behavior is incorrect

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[xidachen]

Attachment: fillText.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36

Steps to reproduce:

Open the attached html file in firefox nightly build


Actual results:

"Hello world C" is displayed.


Expected results:

"Hello world C" should not be displayed

Comment 1

[Emil Pasca [:emilpasca]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0

I have tested this issue on Ubuntu 14.04 x32, Ubuntu 16.04 x64, Mac OS X 10.9 and Windows 10 x64 with the latest Firefox release (47.0) and the latest Nightly (50.0a1-20160627030215) and managed to reproduce it.
When opening the testcase provided in the description you can observe that "Hello world C" is displayed.

Comment 2

[Vincent Liu[:vliu]]

I can also reproduce it on my Mac OS 10.11 through I am not sure the reasonable behavior at this moment, but I will take it to look into this issue.

Comment 3

[Vincent Liu[:vliu]]

From the spec in [1], the API should return an empty array if maxWidth was provided but is less than or equal to zero or equal to NaN.

[1]: https://html.spec.whatwg.org/multipage/scripting.html#dom-context-2d-filltext

Comment 4

[Vincent Liu[:vliu]]

Attachment: The API should return an empty array if maxWidth was provided but is less than or equal to zero or equal to NaN.

Hi Jeff,
Could you please help me to review this patch? Thanks.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8766971 [details] [diff] [review]
The API should return an empty array if maxWidth was provided but is less than or equal to zero or equal to NaN.

Review of attachment 8766971 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable. Can you make sure there's a web platform test for this? https://github.com/w3c/web-platform-tests/tree/master/2dcontext

Comment 6

[Vincent Liu[:vliu]]

Attachment: Add wpt for fillText() in Canvas. r=jmuizelaar, Ms2ger

Hi Jeff,
The attached patch for adding wpt for fillText(). I found we already have wpt for zero/negative. I noly remove their *.ini file
since they all expect ture for the result. Besides, I add new test for NaN case. Could you please have review for them? Thanks.


Hi Ms2ger,
As talked on IRC, could you please also have review for the test? Really thanks.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8769963 [details] [diff] [review]
Add wpt for fillText() in Canvas. r=jmuizelaar, Ms2ger

Review of attachment 8769963 [details] [diff] [review]:
-----------------------------------------------------------------

The test will not be run; do `./mach test-wpt --manifest-update 2dcontext/drawing-text-to-the-canvas/2d.text.draw.fill.maxWidth.NaN.html`.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8769963 [details] [diff] [review]
Add wpt for fillText() in Canvas. r=jmuizelaar, Ms2ger

Review of attachment 8769963 [details] [diff] [review]:
-----------------------------------------------------------------

The actual content of the test looks fine. I'll defer to Ms2ger for the infrastructure surrounding it.

Comment 9

[Vincent Liu[:vliu]]

Attachment: Add wpt for fillText() in Canvas. r=jmuizelaar, Ms2ger

Hi Ms2ger,
The modification for MANIFEST.json was attached in patch. Could you please have a review? Thanks

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8770481 [details] [diff] [review]
Add wpt for fillText() in Canvas. r=jmuizelaar, Ms2ger

Review of attachment 8770481 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, LGTM. Do fold this into the patch that changes the code, though.

Comment 11

[Vincent Liu[:vliu]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc83a7d59e55

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c09aef633ab
The API should return an empty array if maxWidth was provided but is less than or equal to zero or equal to NaN. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8e504f0e0c
Add wpt for fillText() in Canvas. r=jmuizelaar, Ms2ger

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1c09aef633ab
https://hg.mozilla.org/mozilla-central/rev/2d8e504f0e0c

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to :Ms2ger from comment #10)
> Do fold this into the patch that changes the code, though.

*sigh*