Bug 61528 – Implement maxWidth check for canvas fill/strokeText

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Philip Rogers]

Implement maxWidth for fillText and strokeText, fixing the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test.

[Philip Rogers]

Created attachment 94991 [details]
Implement maxWidth for fillText and strokeText, fixing the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test. See bug https://bugs.webkit.org/show_bug.cgi?id=61528

Complete patch minus removing the fixed test from the skip list.

[Philip Rogers]

Created attachment 95173 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

Added the test skips and updated the test expectation file. This patch should be good to review.

[Philip Rogers]

(In reply to comment #2)
> Created an attachment (id=95173) [details]
> Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test
> 
> Added the test skips and updated the test expectation file. This patch should be good to review.

Friendly ping for a re-review :)

[Eric Seidel (no email)]

Seems reasonable to me. CCing folks who I know have touched canvas in the past.

[Philip Rogers]

(In reply to comment #4)
> Seems reasonable to me. CCing folks who I know have touched canvas in the past.

Thank you. Would love to land this :)

[Matthew Delaney]

Comment on attachment 95173 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

View in context: https://bugs.webkit.org/attachment.cgi?id=95173&action=review

Overall, looks good.

> LayoutTests/ChangeLog:5
> +        Removing "canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface" test from skip lists for bug https://bugs.webkit.org/show_bug.cgi?id=61528

How does this change affect the tests 2d.text.draw.fill.maxWidth.{small,larger,zero}? I just looked at them briefly and they have written in them that they can't be verified automatically, though I'm not sure why. I imagine versions that used a font like Ahem with a maxWidth to try to cover (or not cover) the canvas and then checks the pixel values after. Don't change the philip tests, but you could instead add some tests to fast/canvas that test things such as: a negative/zero/very large/very small 'maxWidth'.

> LayoutTests/ChangeLog:7
> +        * platform/chromium/test_expectations.txt:

Unless it was removed recently, it appears there's also an expected results in platform/chromium of passing that could also be removed.

[Philip Rogers]

Created attachment 100168 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

[Philip Rogers]

Thanks for the review Matthew.

I verified that this patch causes Philip's manual 2d.text.draw.fill.maxWidth.{small,larger,zero} tests to pass and added three new tests that automatically verify this change.

[WebKit Review Bot]

Comment on attachment 100168 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

Attachment 100168 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9006126

New failing tests:
http/tests/misc/slow-loading-mask.html
http/tests/navigation/javascriptlink-frames.html
fast/blockflow/japanese-rl-selection.html
fast/canvas/2d.text.draw.fill.maxWidth.verySmall.html
fast/backgrounds/background-leakage.html
fast/box-shadow/scaled-box-shadow.html
fast/backgrounds/repeat/negative-offset-repeat.html
svg/W3C-SVG-1.1/struct-use-01-t.svg
transforms/2d/hindi-rotated.html
svg/transforms/text-with-mask-with-svg-transform.svg
svg/repaint/filter-repaint.svg
fast/blockflow/japanese-lr-selection.html

[WebKit Review Bot]

Created attachment 100170 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[James Robinson]

The philip test suite is imported, so if you add new tests to that directory it'll cause conflicts the next time we try to update our copy of the suite from upstream.  Can you coordinate with the author upstream to see if he'll accept these tests?

[Matthew Delaney]

(In reply to comment #11)
> The philip test suite is imported, so if you add new tests to that directory it'll cause conflicts the next time we try to update our copy of the suite from upstream.  Can you coordinate with the author upstream to see if he'll accept these tests?

It looks like Philip put all his new tests in fast/canvas and didn't touch Philip Taylor's tests, so that's good

[Matthew Delaney]

Woops, didn't finish that reply - changing the bug title also submitted my text!

James, I'm guessing your previous comment was from reading the previous title for this bug? I updated the title for what the patch is here.

His fix implements maxWidth for text drawing which fixes (2d.text.draw.fill.maxWidth.fontface.html) and those standalone tests philip (Taylor) has.

Philip Rogers (what's your IRC? probably easier to refer to you that way), could you rework those tests you added to have the canvas show only green (all over) on success? I imagine just having that super big "I" cover all the red canvas if it's green if it's supposed to be visible AND a totally green canvas that isn't covered by a big red "I" if the text drawn isn't visible (because of the width limitations). Does that make sense?

Also, keep in mind that each platform will draw that "I" a little differently, so either make it overly large or see if you can use the Ahem font (via context.font) to just draw squares. I'm not sure if I've used non-standard fonts on a canvas before, but looking at the spec quickly it sounds like you should be able to.

[Philip Rogers]

Created attachment 100214 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

[Philip Rogers]

(In reply to comment #13)
> Woops, didn't finish that reply - changing the bug title also submitted my text!
> 
> James, I'm guessing your previous comment was from reading the previous title for this bug? I updated the title for what the patch is here.

Thanks for updating the title
 
> His fix implements maxWidth for text drawing which fixes (2d.text.draw.fill.maxWidth.fontface.html) and those standalone tests philip (Taylor) has.
> 
> Philip Rogers (what's your IRC? probably easier to refer to you that way), could you rework those tests you added to have the canvas show only green (all over) on success? I imagine just having that super big "I" cover all the red canvas if it's green if it's supposed to be visible AND a totally green canvas that isn't covered by a big red "I" if the text drawn isn't visible (because of the width limitations). Does that make sense?
> 
> Also, keep in mind that each platform will draw that "I" a little differently, so either make it overly large or see if you can use the Ahem font (via context.font) to just draw squares. I'm not sure if I've used non-standard fonts on a canvas before, but looking at the spec quickly it sounds like you should be able to.

It's unfortunate Philip and I have the same name! Feel free to call me pdr or pdr2 (my irc nick)

I reworked the tests to use the Ahem font as you suggested. I was hoping to avoid using Ahem because there's some trickiness around getting the fonts to load before using them, but I think we have a much cleaner set of tests now.

[Matthew Delaney]

Comment on attachment 100214 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

View in context: https://bugs.webkit.org/attachment.cgi?id=100214&action=review

I wonder if we can find a way to have all resources that are shared by different groups of tests (CSS, canvas, svg, etc.) in some central location. Such as having a LayoutTests/resources that you'd end up linking to like  "../../resources/Ahem.ttf". This would be a pain if you moved around groups of tests, though I don't think that happens very often.

> LayoutTests/fast/canvas/2d.text.draw.fill.maxWidth.negative-expected.txt:2
> +

Same nit here too.

> LayoutTests/fast/canvas/2d.text.draw.fill.maxWidth.veryLarge-expected.txt:2
> +

Nit: It should say "On success, there should only be a green rectangle." Having no red, but say, some blue would also mean failure.

> LayoutTests/fast/canvas/2d.text.draw.fill.maxWidth.verySmall-expected.txt:2
> +

Same nit here as above.

[Philip Rogers]

Created attachment 101247 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

Updated wording to make the pass case more clear.

[James Robinson]

Comment on attachment 101247 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

View in context: https://bugs.webkit.org/attachment.cgi?id=101247&action=review

Please don't check in _another_ copy of Ahem.ttf.  We already have 5 copies in the tree, refer to one of those or (even better) move one of the existing copies to a more common directory and reference it.  Maybe LayoutTests/resources/ ?

> Source/WebCore/ChangeLog:1
> [phone]  Philip Rogers  <pdr>

Please put your full email address in the changelog.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1937
> +    if (fontWidth <= 0)

This is technically incorrect, although I doubt we have any tests that cover this case.  If the actual width of the text is == 0, we still have to apply the globalCompositeOperation even though this draw call does not touch any pixels because of the way compositing operations are defined in canvas 2d.  For example, if the globalCompositeOperation is "copy" doing a fillText() with text that occupies 0 width should result in a fully transparent black canvas.  http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#drawing-model for reference.

> LayoutTests/ChangeLog:1
> [phone]  Philip Rogers  <pdr>

Need full email

[Philip Rogers]

Created attachment 101354 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

[James Robinson]

Patch doesn't apply, for some reason.  I wonder if we have the wrong svn-props set for Ahem.ttf, as the diff is confusing the review tool as well.

[Philip Rogers]

Created attachment 101423 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

The previous patch was unpatchable due to the existing Ahem.ttf's having svn:executable set. The only I know to produce a formattable diff from the removal of those files is to remove the executable property in one patch, then remove the files in a second patch. @jamesr: do you know of another way?

This patch includes everything from the previous patch, but does not remove the existing Ahem.ttf files.

[James Robinson]

Comment on attachment 101423 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

Looks good, R=me.  Would you mind trying to remove the other copies of Ahem.ttf as a follow up?  You'll probably want to do this from an svn checkout.

[WebKit Review Bot]

Comment on attachment 101423 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

Rejecting attachment 101423 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
0d374810d6a3f4d6ecddf2a765ef6b71c65db192
r91599 = 41501756ab01f21e80655f5cab53e76d1fbb01f5
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9232129

[Philip Rogers]

Created attachment 101784 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

Manually merged to ToT

[WebKit Review Bot]

Comment on attachment 101784 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

Attachment 101784 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9233232

New failing tests:
fast/css/font-face-repeated-url.html
fast/css/font-face-remote.html

[Eric Seidel (no email)]

Comment on attachment 101423 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

Cleared James Robinson's review+ from obsolete attachment 101423 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Philip Rogers]

Created attachment 114559 [details]
Implement maxWidth for fillText and strokeText, fixing the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test.

Due to a release, I let this slip off my radar but I'm back to land this guy. This patch rebases my old patch to ToT and passes all tests (including linux).

[Stephen White]

Comment on attachment 114559 [details]
Implement maxWidth for fillText and strokeText, fixing the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test.

View in context: https://bugs.webkit.org/attachment.cgi?id=114559&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2168
> +            maskImageContext->scale(FloatSize((fontWidth > 0 ? (width / fontWidth) : 0), 1));

A comment here might be helpful.  Do we really want to do a zero X scale in this case, or should we just skip drawing altogether if fontWidth is zero?

Also, is this code path (gradient/pattern text) covered by the given tests?

[Philip Rogers]

Created attachment 115032 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

Replying to review Stephen White and attaching updated patch.

* A comment here might be helpful.  Do we really want to do a zero X scale in this case, or should we just skip drawing altogether if fontWidth is zero?
I added a comment about the composite operations (like copy) requiring this.

* Also, is this code path (gradient/pattern text) covered by the given tests?
Nice catch, they were not. I added the 2d.text.draw.fill.maxWidth.gradient test and made sure it fails on webkit but passes after this patch is applied.

[Stephen White]

Comment on attachment 115032 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

View in context: https://bugs.webkit.org/attachment.cgi?id=115032&action=review

Looks good.  Thanks for the test coverage!  r=me

> LayoutTests/ChangeLog:22
> +        * resources/Ahem.ttf: Added.

(There already look to be at least 5 copies of Ahem.ttf in the tree, but Philip tells me he has a followup patch to consolidate all the copies).

> LayoutTests/fast/canvas/2d.text.draw.fill.maxWidth.negative.html:60
> +}, 50);

Hmm.  This timeout looks like it could introduce flakiness on a heavily-loaded machine (which our layout test bots tend to be).  Unfortunately, I don't know of a way to an onload for @fontface, so I don't have any bright ideas here.

[WebKit Review Bot]

Comment on attachment 115032 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

Clearing flags on attachment: 115032

Committed r100285: <http://trac.webkit.org/changeset/100285>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andreas Kling]

Comment on attachment 115032 [details]
Fixes the canvas/philip/tests/2d.text.draw.fill.maxWidth.fontface.html test

View in context: https://bugs.webkit.org/attachment.cgi?id=115032&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2198
> +        stateSaver.restore();

We don't need this line, GraphicsContextStateSaver calls restore() on destruction.

[Philip Rogers]

Created attachment 115200 [details]
Remove extra GraphicsContextStateSaver restore() call.

Ridiculously small patch to remove an extra restore() call that wasn't found until after the patch had landed.

[Eric Seidel (no email)]

Comment on attachment 115200 [details]
Remove extra GraphicsContextStateSaver restore() call.

View in context: https://bugs.webkit.org/attachment.cgi?id=115200&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:-2208
> -        stateSaver.restore();

I see.  This is extra because the stateSaver RIIA would do it for us?

[Philip Rogers]

(In reply to comment #35)
> (From update of attachment 115200 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115200&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:-2208
> > -        stateSaver.restore();
> 
> I see.  This is extra because the stateSaver RIIA would do it for us?

Yes. The reason for this is stateSaver.restore() is called in StateSaver's destructor which is called when stateSaver goes out of scope (this was pointed out by Andreas). This change just prevents the second, redundant restore call.