Summary: | Fix LayoutTests/canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pravin D <pravind.2k4> | ||||||||
Component: | Canvas | Assignee: | 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
Pravin D
2012-08-02 04:16:40 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 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 ? |