Bug 37686 – Canvas strokeRect strokes rectangle whose dimensions are 0 when lineWidth > 1

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Daniel Bates]

Currently, strokeRect will stroke a rectangle whose dimensions are 0 when lineWidth > 1.

As per the definition of strokeRect in the HTML Canvas 2D Context spec. <http://www.w3.org/TR/2dcontext/#dom-context-2d-strokerect>, this method should have no effect when both the height and width of the rectangle are zero.

[Daniel Bates]

Created attachment 53506 [details]
Patch with test case

[Oliver Hunt]

Comment on attachment 53506 [details]
Patch with test case

I think it would be better to make validateRectForCanvas do the check as that gets the validation in a single place that is used uniformly -- is there any place this might have a bad impact?

[Daniel Bates]

(In reply to comment #2)
> (From update of attachment 53506 [details])
> I think it would be better to make validateRectForCanvas do the check as that
> gets the validation in a single place that is used uniformly -- is there any
> place this might have a bad impact?

I agree, it is better to place this check in validateRectForCanvas. There should not be any negative impact as this is an invalid state for clearRect, and fillRect as well.

[Daniel Bates]

Created attachment 53520 [details]
Patch with test case

[Daniel Bates]

Comment on attachment 53520 [details]
Patch with test case

Will add some more test cases.

[Daniel Bates]

Created attachment 53524 [details]
Patch with test cases

Added additional test cases for fillRect, clearRect, and rect.

[Dirk Schulze]

I think the change and the tests are ok. What do you think Oliver?

[Oliver Hunt]

Comment on attachment 53524 [details]
Patch with test cases

Daniel can you just verify behaviour if you have a shadow set?  eg. should the shadow draw around a 0x0 rect (i know physically it shouldn't i just want to verify firefox compat)

If the shadow behaviour is consistent the patch is good

--Oliver

[Daniel Bates]

Created attachment 54255 [details]
Example strokeRect of 0x0 rect with lineWidth > 1 and shadow

(In reply to comment #8)
> (From update of attachment 53524 [details])
> Daniel can you just verify behaviour if you have a shadow set?  eg. should the
> shadow draw around a 0x0 rect (i know physically it shouldn't i just want to
> verify firefox compat)

Oliver,

Firefox (3.6.3) does not draw a shadow around a 0x0 rect. I've attached one such test case.

Dan

> 
> If the shadow behaviour is consistent the patch is good
> 
> --Oliver

[Daniel Bates]

Created attachment 54256 [details]
Patch with test cases

Added a test case for strokeRect of 0x0 rect with lineWidth > 1 and shadow. Also, rebased results LayoutTests/fast/canvas/canvas-strokeRect-expected.txt.

[Oliver Hunt]

Comment on attachment 54256 [details]
Patch with test cases

r=me

[Daniel Bates]

Comment on attachment 54256 [details]
Patch with test cases

Clearing flags on attachment: 54256

Committed r58381: <http://trac.webkit.org/changeset/58381>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58381 might have broken Qt Linux Release

[Eric Seidel (no email)]

The failure looks real:
http://build.webkit.org/results/Qt%20Linux%20Release/r58381%20(10853)/fast/repaint/selection-gap-overflow-scroll-2-diffs.txt

Not sure if it's related to this bug or not.

[Daniel Bates]

(In reply to comment #14)
> http://trac.webkit.org/changeset/58381 might have broken Qt Linux Release

The failing test, /fast/repaint/selection-gap-overflow-scroll-2.html, is unrelated to this change. Looking at the differences <http://build.webkit.org/results/Qt%20Linux%20Release/r58381%20(10853)/fast/repaint/selection-gap-overflow-scroll-2-pretty-diff.html> they seem to differ by 1-pixel.

For completeness, the stdio from the Qt bot is at: <http://build.webkit.org/builders/Qt%20Linux%20Release/builds/10853/steps/layout-test/logs/stdio>

[Csaba Osztrogonác]

(In reply to comment #16)
I made a clean build, and nothing changed. This test needs only a rebaseline.

[Daniel Bates]

(In reply to comment #17)
> (In reply to comment #16)
> I made a clean build, and nothing changed. This test needs only a rebaseline.

Committed rebased result in change set 58388 <http://trac.webkit.org/changeset/58388>.