Bug 61799

Created attachment 95596 [details] Fix LayoutTests/canvas/suite/tests/2d.text.measure.width.space.html

Created attachment 95607 [details] Adds simplifyHTMLWhiteSpace and call it in drawText as well as measureText, which makes us pass 5 of Philip's canvas tests.

Created attachment 95608 [details] Adds simplifyHTMLWhiteSpace and call it in drawText as well as measureText, which makes us pass 5 of Philip's canvas tests

Comment on attachment 95608 [details] Adds simplifyHTMLWhiteSpace and call it in drawText as well as measureText, which makes us pass 5 of Philip's canvas tests View in context: https://bugs.webkit.org/attachment.cgi?id=95608&action=review Wow, is the behavior really specified this way? That sounds like a mistake in the design of this API. Seems bizarre to collapse the spaces out of a string that is being passed in from JavaScript. review- because I’d like to see a faster version of the “no collapsing needed” code path. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1879 > + const String& str = simplifyHTMLWhiteSpace(text); We frown on abbreviations like “str”. Maybe “simplifiedText” or the argument could be named rawText and this local could be text. > Source/WebCore/html/parser/HTMLParserIdioms.cpp:65 > + const UChar* fromend = from + length; We’d normally capitalize the E here. > Source/WebCore/html/parser/HTMLParserIdioms.cpp:66 > + int outc = 0; We try to avoid variable names like this with abbreviations rather than words. > Source/WebCore/html/parser/HTMLParserIdioms.cpp:69 > + StringBuffer data(length); Unfortunately, this allocates a new buffer in memory every time, even if nothing changes. I think we want to further optimize the no-collapse case so the buffer is only created the first time we have to change something. Since most strings won’t need to be collapsed, I think we could just do a complete pass checking if there is anything to collapse before actually creating the buffer.

Can someone raise this issue on the relevant standards list? This collapsing behavior seems like the wrong choice. Maybe we can nip this in the bud before it’s too late.

(In reply to comment #5) > Can someone raise this issue on the relevant standards list? This collapsing behavior seems like the wrong choice. Maybe we can nip this in the bud before it’s too late. I asked for clarification on the whitespace collapse issue on IRC and it's clear that it isn't clear what the correct behavior is. I posted the following to the public-canvas-api list asking for clarification: http://lists.w3.org/Archives/Public/public-canvas-api/2011AprJun/0037.html

Created attachment 95919 [details] Adds replaceHTMLWhiteSpace and call it in drawText as well as measureText, which makes us pass 5 of Philip's canvas tests From my reading of the spec, it looks like white space should not be collapsed (meaning the tests are wrong and my patch was wrong), but please hold off on an actual review until it's confirmed. I updated this patch to reflect the reviewer comments and have a more optimized conversion step from html white space characters to ' '. I also temporarily used simplifyWhiteSpace() to do the collapsing. When I get a response on this message (http://lists.w3.org/Archives/Public/public-canvas-api/2011AprJun/0037.html), I'll update this bug with the final patch.

Hi Philip, I was trying to work on http://w3c-test.org/html/tests/approved/canvas/2d.text.draw.space.collapse.nonspace.html Which similar this issue I guess... Is there any updates on this issue?

(In reply to comment #8) > Hi Philip, > > I was trying to work on > http://w3c-test.org/html/tests/approved/canvas/2d.text.draw.space.collapse.nonspace.html > > Which similar this issue I guess... > > Is there any updates on this issue? Hey Pravin, there was an update to this. The spec was modified slightly so that it now contains the line "'white-space' property of the inline element set to 'pre'", which is what I think we wanted. I have sent a patch to Philip (the canvas test author) to update the tests but I haven't heard back yet. I wonder... should we just update the tests in WebKit and mark it as differing from the original? WRT your bug, I think these are very similar indeed. Does this patch fix your test as well?

(In reply to comment #9) > (In reply to comment #8) > > Hi Philip, > > > > I was trying to work on > > http://w3c-test.org/html/tests/approved/canvas/2d.text.draw.space.collapse.nonspace.html > > > > Which similar this issue I guess... > > > > Is there any updates on this issue? > > Hey Pravin, there was an update to this. The spec was modified slightly so that it now contains the line "'white-space' property of the inline element set to 'pre'", which is what I think we wanted. > > I have sent a patch to Philip (the canvas test author) to update the tests but I haven't heard back yet. I wonder... should we just update the tests in WebKit and mark it as differing from the original? > > WRT your bug, I think these are very similar indeed. Does this patch fix your test as well? > Did more analysis on the issue I was looking into... I guess its a little different from one you are tackling... However I'm guessing that the failure in LayoutTests/canvas/suite/tests/2d.text.measure.width.space.html maybe partly due to LayoutTests/canvas/philip/tests/2d.text.draw.space.collapse.nonspace.html ... I'll raise another bug for the same n get back to you...

Hi Philip, Kindly see the bug https://bugs.webkit.org/show_bug.cgi?id=92974 ... it is also dependent on this bug... Expecting a patch soon :)

It looks like we still have some level of failure here: https://wpt.fyi/results/html/canvas/element/text-styles/2d.text.measure.width.space.html?label=master&label=experimental&aligned&q=2d.text.measure.width.space.html

<rdar://problem/97218118>