Bug 61799

Summary: Fix LayoutTests/canvas/suite/tests/2d.text.measure.width.space.html
Product: WebKit Reporter: Philip Rogers <pdr>
Component: CanvasAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: bfulgham, darin, mdelaney7, pravind.2k4, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 46506, 92974    
Attachments:
Description Flags
Fix LayoutTests/canvas/suite/tests/2d.text.measure.width.space.html
none
Adds simplifyHTMLWhiteSpace and call it in drawText as well as measureText, which makes us pass 5 of Philip's canvas tests.
none
Adds simplifyHTMLWhiteSpace and call it in drawText as well as measureText, which makes us pass 5 of Philip's canvas tests
darin: review-, darin: commit-queue-
Adds replaceHTMLWhiteSpace and call it in drawText as well as measureText, which makes us pass 5 of Philip's canvas tests none

Description Philip Rogers 2011-05-31 12:29:09 PDT
Fix LayoutTests/canvas/suite/tests/2d.text.measure.width.space.html. This is failing because we don't properly collapse whitespace in canvas.
Comment 1 Philip Rogers 2011-06-01 07:02:18 PDT
Created attachment 95596 [details]
Fix LayoutTests/canvas/suite/tests/2d.text.measure.width.space.html
Comment 2 Philip Rogers 2011-06-01 08:40:28 PDT
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.
Comment 3 Philip Rogers 2011-06-01 08:40:59 PDT
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 4 Darin Adler 2011-06-01 09:29:36 PDT
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.
Comment 5 Darin Adler 2011-06-01 09:30:18 PDT
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.
Comment 6 Philip Rogers 2011-06-02 07:39:05 PDT
(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
Comment 7 Philip Rogers 2011-06-03 08:48:45 PDT
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.
Comment 8 Pravin D 2012-07-29 08:41:23 PDT
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?
Comment 9 Philip Rogers 2012-07-29 18:53:22 PDT
(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?
Comment 10 Pravin D 2012-07-30 03:13:08 PDT
(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...
Comment 11 Pravin D 2012-08-06 13:40:27 PDT
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 :)
Comment 13 Radar WebKit Bug Importer 2022-07-18 13:54:48 PDT
<rdar://problem/97218118>