Bug 61799 – Fix LayoutTests/canvas/suite/tests/2d.text.measure.width.space.html

• Status: NEW
• Component: Canvas

[Philip Rogers]

Fix LayoutTests/canvas/suite/tests/2d.text.measure.width.space.html. This is failing because we don't properly collapse whitespace in canvas.

[Philip Rogers]

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

[Philip Rogers]

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.

[Philip Rogers]

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

[Darin Adler]

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.

[Darin Adler]

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.

[Philip Rogers]

(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

[Philip Rogers]

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.

[Pravin D]

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?

[Philip Rogers]

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

[Pravin D]

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

[Pravin D]

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 :)

[Brent Fulgham]

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

[Radar WebKit Bug Importer]

<rdar://problem/97218118>