Bug 181315 – ::first-letter incorrectly selects grapheme pairs

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Chris Nardi]

::first-letter incorrectly selects grapheme pairs

[Chris Nardi]

Created attachment 330512 [details]
Patch

[Chris Nardi]

Created attachment 330513 [details]
Patch

[Chris Nardi]

Created attachment 330514 [details]
Patch

[EWS Watchlist]

Comment on attachment 330514 [details]
Patch

Attachment 330514 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5935814

New failing tests:
fast/css/first-letter-punctuation.html

[EWS Watchlist]

Created attachment 330520 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 330514 [details]
Patch

Attachment 330514 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5935783

New failing tests:
fast/css/first-letter-punctuation.html

[EWS Watchlist]

Created attachment 330521 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 330514 [details]
Patch

Attachment 330514 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5935885

New failing tests:
fast/css/first-letter-punctuation.html

[EWS Watchlist]

Created attachment 330522 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 330514 [details]
Patch

Attachment 330514 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5935927

New failing tests:
fast/css/first-letter-punctuation.html

[EWS Watchlist]

Created attachment 330527 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Nardi]

Created attachment 330550 [details]
Patch

[EWS Watchlist]

Comment on attachment 330550 [details]
Patch

Attachment 330550 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5942920

New failing tests:
fast/css/first-letter-punctuation.html

[EWS Watchlist]

Created attachment 330554 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Chris Nardi]

Created attachment 330557 [details]
Patch

[Chris Nardi]

I think this is ready for review. All bots seem to be passing except for wincairom, which for some reason cannot apply the patch (even though the others can). I'm sure there's more rebaselineing that needs to occur, but I've just been working off of what the bots provide and GTK/Win don't seem to be providing any new expectation files.

This was found from CSS2 tests in WPT (https://github.com/w3c/web-platform-tests/blob/master/css/CSS2/selectors/first-letter-punctuation-335.xht). I used that test as the basis for what I included in fast/css/first-letter-punctuation.html.

A similar version of this patch was also submitted to Chromium (https://chromium-review.googlesource.com/c/chromium/src/+/847293).

[Myles C. Maxfield]

Comment on attachment 330557 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330557&action=review

> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:225
> +        length += U16_LENGTH(oldText.characterStartingAt(length));

Are we sure this is going to work? What if the first character is a combining sequence like U+0065 LATIN SMALL LETTER E followed by U+0300 COMBINING GRAVE ACCENT?

[Chris Nardi]

Created attachment 330633 [details]
Patch

[Chris Nardi]

Created attachment 330634 [details]
Patch

[Chris Nardi]

Comment on attachment 330557 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330557&action=review

>> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:225
>> +        length += U16_LENGTH(oldText.characterStartingAt(length));
> 
> Are we sure this is going to work? What if the first character is a combining sequence like U+0065 LATIN SMALL LETTER E followed by U+0300 COMBINING GRAVE ACCENT?

That's true, that didn't occur to me. I updated the rest of the patch with this consideration.

[Chris Nardi]

Created attachment 330650 [details]
Patch

[Darin Adler]

Comment on attachment 330650 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330650&action=review

> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:240
> +            if (numCharacters > 1)
> +                scanLength += numCharacters - 1;

The is a strange way to write the loop. I would instead suggest either:

1) Removing the ++scanLength from the for loop above and instead writing this here:

    scanLength += numCharacters;

or

2) Defining numCharacters before the loop so it can be used in the for loop and then writing:

    for (unsigned scanLength = length; scanLength < oldText.length(); scanLength += numCharacters)

Also seems that numCharacters and numCharacterInGraphemeClusters are both not great names. Since these are "code units", not "characters". Should be numCodeUnitsInGraphemeClusters and numCodeUnits. Someone should come back and rename.

[Darin Adler]

(In reply to Chris Nardi from comment #20)
> > Are we sure this is going to work? What if the first character is a combining sequence like U+0065 LATIN SMALL LETTER E followed by U+0300 COMBINING GRAVE ACCENT?
> 
> That's true, that didn't occur to me. I updated the rest of the patch with
> this consideration.

Thanks for doing that.

Did you add a test case that includes a grapheme cluster that is more than one code point? I didn’t notice.

[Chris Nardi]

(In reply to Darin Adler from comment #23)
> (In reply to Chris Nardi from comment #20)
> > > Are we sure this is going to work? What if the first character is a combining sequence like U+0065 LATIN SMALL LETTER E followed by U+0300 COMBINING GRAVE ACCENT?
> > 
> > That's true, that didn't occur to me. I updated the rest of the patch with
> > this consideration.
> 
> Thanks for doing that.
> 
> Did you add a test case that includes a grapheme cluster that is more than
> one code point? I didn’t notice.


The punctuation I included in the test case is two code points long, but I didn't include a test case that involved the first letter itself having multiple code points. I proposed a web platform test upstream (https://github.com/w3c/web-platform-tests/pull/8939) covering this; I could try to import that if it gets merged soon.

[Chris Nardi]

Created attachment 330721 [details]
Patch

[Chris Nardi]

Comment on attachment 330650 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330650&action=review

>> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:240
>> +                scanLength += numCharacters - 1;
> 
> The is a strange way to write the loop. I would instead suggest either:
> 
> 1) Removing the ++scanLength from the for loop above and instead writing this here:
> 
>     scanLength += numCharacters;
> 
> or
> 
> 2) Defining numCharacters before the loop so it can be used in the for loop and then writing:
> 
>     for (unsigned scanLength = length; scanLength < oldText.length(); scanLength += numCharacters)
> 
> Also seems that numCharacters and numCharacterInGraphemeClusters are both not great names. Since these are "code units", not "characters". Should be numCodeUnitsInGraphemeClusters and numCodeUnits. Someone should come back and rename.

I implemented your second suggestion. I did not change the function name or the variable name; I figured that would be better with another patch.

I also added my proposed WPT testcase as I realized that it could just be reviewed here instead of upstream.

[Myles C. Maxfield]

Comment on attachment 330721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330721&action=review

> LayoutTests/platform/mac-wk2/fast/css/first-letter-punctuation-expected.txt:1
> +layer at (0,0) size 800x600

It's surprising that we need a mac-wk2 specific result. Do you know why we need this file?

[Myles C. Maxfield]

Comment on attachment 330721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330721&action=review

> LayoutTests/platform/ios/fast/css/first-letter-punctuation-expected.txt:38
> +      RenderBlock {DIV} at (0,208) size 784x50
> +        RenderInline (generated) at (0,0) size 49x41 [color=#008000]
> +          RenderText at (0,6) size 49x41
> +            text run at (0,6) width 49: "\x{D800}\x{DD00}T\x{D800}\x{DD00}"
> +        RenderText {#text} at (48,24) size 19x19
> +          text run at (48,24) width 19: "est"

We should try to change the test so it's a reftest instead of a DRT test. But this work shouldn't block this patch from landing.

[Chris Nardi]

(In reply to Myles C. Maxfield from comment #27)
> Comment on attachment 330721 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330721&action=review
> 
> > LayoutTests/platform/mac-wk2/fast/css/first-letter-punctuation-expected.txt:1
> > +layer at (0,0) size 800x600
> 
> It's surprising that we need a mac-wk2 specific result. Do you know why we
> need this file?

I'm not sure. My only guess was that one shows the glyph whereas the other shows an empty box, but the images matched so I don't think that's it.

This is my first contribution to WebKit itself; I believe submitting this to the CQ is the proper step but I'm not sure how to.

This will probably also result in failures of fast/css/first-letter-punctuation.html on Win since I wasn't able to rebaseline it there.

[EWS Watchlist]

Comment on attachment 330721 [details]
Patch

Attachment 330721 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5976337

New failing tests:
fast/text/firstline/003.html
imported/blink/fast/css/first-letter-all-inherit-td-crash.html
imported/w3c/web-platform-tests/css/css-pseudo-4/first-letter-004.html
imported/blink/fast/css/first-letter-range-insert.html
fast/dom/inner-text-first-letter.html
imported/blink/editing/text-iterator/read-past-cloned-first-letter.html
fast/css/first-letter-punctuation.html

[EWS Watchlist]

Created attachment 330729 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 330721 [details]
Patch

Attachment 330721 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5976388

New failing tests:
fast/text/firstline/003.html
imported/blink/fast/css/first-letter-all-inherit-td-crash.html
imported/w3c/web-platform-tests/css/css-pseudo-4/first-letter-004.html
imported/blink/fast/css/first-letter-range-insert.html
fast/dom/inner-text-first-letter.html
imported/blink/editing/text-iterator/read-past-cloned-first-letter.html
fast/css/first-letter-punctuation.html

[EWS Watchlist]

Created attachment 330731 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Myles C. Maxfield]

(In reply to Chris Nardi from comment #25)
> Created attachment 330721 [details]
> Patch

Yikes, lots of red. Please fix this before you land it.

[EWS Watchlist]

Comment on attachment 330721 [details]
Patch

Attachment 330721 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5976481

New failing tests:
fast/text/firstline/003.html
imported/blink/fast/css/first-letter-all-inherit-td-crash.html
imported/w3c/web-platform-tests/css/css-pseudo-4/first-letter-004.html
imported/blink/fast/css/first-letter-range-insert.html
fast/dom/inner-text-first-letter.html
imported/blink/editing/text-iterator/read-past-cloned-first-letter.html
fast/css/first-letter-punctuation.html

[EWS Watchlist]

Created attachment 330735 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 330721 [details]
Patch

Attachment 330721 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5976659

New failing tests:
fast/text/firstline/003.html
imported/blink/fast/css/first-letter-all-inherit-td-crash.html
imported/blink/editing/text-iterator/read-past-cloned-first-letter.html
imported/blink/fast/css/first-letter-range-insert.html
fast/dom/inner-text-first-letter.html
imported/w3c/web-platform-tests/css/css-pseudo-4/first-letter-004.html
fast/css/first-letter-punctuation.html

[EWS Watchlist]

Created attachment 330736 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Chris Nardi]

Created attachment 330778 [details]
Patch

[Chris Nardi]

(In reply to Myles C. Maxfield from comment #34)
> (In reply to Chris Nardi from comment #25)
> > Created attachment 330721 [details]
> > Patch
> 
> Yikes, lots of red. Please fix this before you land it.

Yeah, forgot to change the original line when copying & pasting. I'm surprised that didn't generated a compiler warning.

Since I was creating another patch anyway, I changed fast/css/first-letter-punctuation.html into a reftest. That way, there shouldn't need to be any gardening after this patch lands.

[WebKit Commit Bot]

Comment on attachment 330778 [details]
Patch

Clearing flags on attachment: 330778

Committed r226614: <https://trac.webkit.org/changeset/226614>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36369465>

[Darin Adler]

Comment on attachment 330778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330778&action=review

> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:101
>      return isSpaceOrNewline(c) || c == noBreakSpace || isPunctuationForFirstLetter(c);

Oops, I just spotted a bug here!

The isSpaceOrNewline function takes a UChar, not a UChar32, so calling it will chop off the low 16 bits of "c" and possibly return true when the character is not a space or newline. The fix is to change isSpaceOrNewline to take a UChar32.

[Darin Adler]

Comment on attachment 330778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330778&action=review

>> Source/WebCore/rendering/updating/RenderTreeBuilderFirstLetter.cpp:101
>>      return isSpaceOrNewline(c) || c == noBreakSpace || isPunctuationForFirstLetter(c);
> 
> Oops, I just spotted a bug here!
> 
> The isSpaceOrNewline function takes a UChar, not a UChar32, so calling it will chop off the low 16 bits of "c" and possibly return true when the character is not a space or newline. The fix is to change isSpaceOrNewline to take a UChar32.

That might not work if anyone is passing the isSpaceOrNewline function as a CodeUnitMatchFunction. So I don’t know exactly what the fix is, but there is definitely a bug here.