Bug 92974

Summary: Fix LayoutTests/canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html
Product: WebKit Reporter: Pravin D <pravind.2k4>
Component: CanvasAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: arpitabahuguna, cdumez, dglazkov, d-r, gyuyoung.kim, hyatt, jamesr, kenneth, kling, pravind, rakuco, rashmi.s2, rashmi.shyam, s.choi, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://w3c-test.org/html/tests/approved/canvas/2d.text.draw.space.collapse.nonspace.html
Bug Depends on: 61799    
Bug Blocks: 46506    
Attachments:
Description Flags
Patch
webkit.review.bot: commit-queue-
Patch
webkit.review.bot: commit-queue-
Patch none

Description Pravin D 2012-08-02 04:16:40 PDT
Fix LayoutTests/canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html. This is failing because we are collapsing some whitespace characters in canvas.
Comment 1 Pravin D 2012-08-06 13:35:43 PDT
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
Comment 2 Chris Dumez 2012-08-14 07:10:30 PDT
Created attachment 158320 [details]
Patch
Comment 3 WebKit Review Bot 2012-08-14 07:26:39 PDT
Comment on attachment 158320 [details]
Patch

Attachment 158320 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13489685
Comment 4 Build Bot 2012-08-14 07:28:58 PDT
Comment on attachment 158320 [details]
Patch

Attachment 158320 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13491611
Comment 5 Chris Dumez 2012-08-14 07:52:14 PDT
Created attachment 158329 [details]
Patch
Comment 6 WebKit Review Bot 2012-08-14 07:55:20 PDT
Comment on attachment 158329 [details]
Patch

Attachment 158329 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13489695
Comment 7 Build Bot 2012-08-14 07:58:58 PDT
Comment on attachment 158329 [details]
Patch

Attachment 158329 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13493648
Comment 8 Kenneth Rohde Christiansen 2012-08-14 08:09:38 PDT
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 9 Early Warning System Bot 2012-08-14 08:14:27 PDT
Comment on attachment 158329 [details]
Patch

Attachment 158329 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13503222
Comment 10 Build Bot 2012-08-14 08:31:19 PDT
Comment on attachment 158329 [details]
Patch

Attachment 158329 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13501241
Comment 11 Pravin D 2012-08-14 08:31:32 PDT
Christophe Dumez u might want to see the https://bugs.webkit.org/show_bug.cgi?id=61799 ...
Comment 12 Chris Dumez 2012-08-14 08:51:39 PDT
(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 13 Early Warning System Bot 2012-08-14 08:53:10 PDT
Comment on attachment 158329 [details]
Patch

Attachment 158329 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13486956
Comment 14 Pravin D 2012-08-14 09:40:05 PDT
(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.
Comment 15 Chris Dumez 2012-08-14 09:47:55 PDT
(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.
Comment 16 Chris Dumez 2012-08-14 09:50:38 PDT
Created attachment 158351 [details]
Patch

Take Kenneth's feedback into consideration.
Comment 17 WebKit Review Bot 2012-08-14 10:16:35 PDT
Comment on attachment 158351 [details]
Patch

Clearing flags on attachment: 158351

Committed r125575: <http://trac.webkit.org/changeset/125575>
Comment 18 WebKit Review Bot 2012-08-14 10:16:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Pravin D 2012-08-17 05:18:49 PDT
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 ?