Bug 92974

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

Created attachment 158320 [details] Patch

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

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

Created attachment 158329 [details] Patch

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

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

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

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

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

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

(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?

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

(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.

(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.

Created attachment 158351 [details] Patch Take Kenneth's feedback into consideration.

Comment on attachment 158351 [details] Patch Clearing flags on attachment: 158351 Committed r125575: <http://trac.webkit.org/changeset/125575>

All reviewed patches have been landed. Closing bug.

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 ?