::first-letter incorrectly selects grapheme pairs
Created attachment 330512 [details] Patch
Created attachment 330513 [details] Patch
Created attachment 330514 [details] Patch
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
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
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
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
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
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
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
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
Created attachment 330550 [details] Patch
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
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
Created attachment 330557 [details] Patch
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).
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?
Created attachment 330633 [details] Patch
Created attachment 330634 [details] Patch
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.
Created attachment 330650 [details] Patch
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.
(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.
(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.
Created attachment 330721 [details] Patch
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.
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?
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.
(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.
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
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
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
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
(In reply to Chris Nardi from comment #25) > Created attachment 330721 [details] > Patch Yikes, lots of red. Please fix this before you land it.
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
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
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
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
Created attachment 330778 [details] Patch
(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.
Comment on attachment 330778 [details] Patch Clearing flags on attachment: 330778 Committed r226614: <https://trac.webkit.org/changeset/226614>
All reviewed patches have been landed. Closing bug.
<rdar://problem/36369465>
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.
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.