Bug 92974 – Fix LayoutTests/canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html

• Component: Canvas
• Status: RESOLVED FIXED

[Pravin D]

Fix LayoutTests/canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html. This is failing because we are collapsing some whitespace characters in canvas.

[Pravin D]

The issue is that when calculating the text run width in CanvasRenderingContext2D::drawTextInternal() the character \x0b(vertical tab) is not replaced by space(U+0020) character(according to SPEC). \x0b being the zero width space character for the given font text run width is calculated incorrectly.

SPEC: http://www.w3.org/TR/2dcontext/#drawing-text-to-the-canvas

[Chris Dumez]

Created attachment 158320 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158320 [details]
Patch

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

[Build Bot]

Comment on attachment 158320 [details]
Patch

Attachment 158320 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13491611

[Chris Dumez]

Created attachment 158329 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158329 [details]
Patch

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

[Build Bot]

Comment on attachment 158329 [details]
Patch

Attachment 158329 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13493648

[Kenneth Rohde Christiansen]

Comment on attachment 158329 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2135
> +static String replaceInString(const String& text, WTF::CharacterMatchFunctionPtr matchFunction, const String& replacement)

replaceOccurrenceInString ? InString makes it sounds a bit like it is replacing in the original string

Maybe substituteOccurancesInString ?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2141
> +        index += replacement.length();

Why not store the length?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2164
> +    String cleanedText = replaceInString(text, isSpaceOrNewline, "\u0020");

cleaned doesn't sounds so nice, what about legalText or normalizedText or similar

[Early Warning System Bot]

Comment on attachment 158329 [details]
Patch

Attachment 158329 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13503222

[Build Bot]

Comment on attachment 158329 [details]
Patch

Attachment 158329 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13501241

[Pravin D]

Christophe Dumez u might want to see the https://bugs.webkit.org/show_bug.cgi?id=61799 ...

[Chris Dumez]

(In reply to comment #11)
> Christophe Dumez u might want to see the https://bugs.webkit.org/show_bug.cgi?id=61799 ...

Thanks for the heads up. I had a quick look but that patch and it is not going to help for this present test (canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html) since we do NOT want to simplify white spaces. We want to replaces space characters by u+0020, that's it, no collapsing. If we collapse spaces, then canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html will still fail.

Do we want to merge the patches?

[Early Warning System Bot]

Comment on attachment 158329 [details]
Patch

Attachment 158329 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13486956

[Pravin D]

(In reply to comment #12)
> (In reply to comment #11)
> > Christophe Dumez u might want to see the https://bugs.webkit.org/show_bug.cgi?id=61799 ...
> 
> Thanks for the heads up. I had a quick look but that patch and it is not going to help for this present test (canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html) since we do NOT want to simplify white spaces. We want to replaces space characters by u+0020, that's it, no collapsing. If we collapse spaces, then canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html will still fail.

> 

I'm don't think the space is getting collapsed in the other patch(bug 61799). He is doing a very similar thing to what you are doing. However the function that decides if a character is space or newline seems to be the difference.

> 
> Do we want to merge the patches?
> 
Maybe u can ask some reviewer on this :) .

PS: 

String cleanedText = replaceInString(text, isSpaceOrNewline, "\u0020"); u might want to replace "\u0020" with space (' '). I think its ok to do so not sure though.

[Chris Dumez]

(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Christophe Dumez u might want to see the https://bugs.webkit.org/show_bug.cgi?id=61799 ...
> > 
> > Thanks for the heads up. I had a quick look but that patch and it is not going to help for this present test (canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html) since we do NOT want to simplify white spaces. We want to replaces space characters by u+0020, that's it, no collapsing. If we collapse spaces, then canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html will still fail.
> 
> > 
> 
> I'm don't think the space is getting collapsed in the other patch(bug 61799). He is doing a very similar thing to what you are doing. However the function that decides if a character is space or newline seems to be the difference.

well, the other patch calls simplifyWhiteSpace(), which collapses spaces.

> 
> > 
> > Do we want to merge the patches?
> > 
> Maybe u can ask some reviewer on this :) .
> 
> PS: 
> 
> String cleanedText = replaceInString(text, isSpaceOrNewline, "\u0020"); u might want to replace "\u0020" with space (' '). I think its ok to do so not sure though.

Right, this is why EWS bots are complaining. I'm fixing it now. \u0020 is equivalent to an ASCII space.

[Chris Dumez]

Created attachment 158351 [details]
Patch

Take Kenneth's feedback into consideration.

[WebKit Review Bot]

Comment on attachment 158351 [details]
Patch

Clearing flags on attachment: 158351

Committed r125575: <http://trac.webkit.org/changeset/125575>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Pravin D]

There is a bug in w3 regarding a similar issue.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=15925 

Is it a little contradicting to this fix ?