Avoid creating first-letter RenderTextFragments for unsuitable text nodes
First-letter code would formerly find the first RenderText in a RenderBlock
and pass it off to be split into RenderTextFragments. If no suitable letter
was found in that RenderText, we'd use its entire contents (be it whitespace
or punctuation) in the RenderTextFragment. When the RenderText is whitespace
only and was combined with column handling code that splits blocks, this
could lead to creating first-letter renderers out of pre-existing first-
letter renderers, which could cause crashes.
Adding some assertions and changing the search algorithm for the first
text node to also check for a valid first-letter character. This avoids
the crashes and leads to results that are closer to my interpretation of
what the spec suggests (i.e. we won't apply first-letter to only punctuation).
BUG=264574
Review URL: https://codereview.chromium.org/26315006
git-svn-id: svn://svn.chromium.org/blink/trunk@159205 bbb929c8-8fbe-4397-9dbb-9b2b20218538
diff --git a/LayoutTests/editing/selection/extend-by-word-002-expected.txt b/LayoutTests/editing/selection/extend-by-word-002-expected.txt
index b9cb8ea..d40cc2b 100644
--- a/LayoutTests/editing/selection/extend-by-word-002-expected.txt
+++ b/LayoutTests/editing/selection/extend-by-word-002-expected.txt
@@ -1,8 +1,8 @@
PASS selection.type is "Range"
-PASS selection.anchorNode is $("test").querySelectorAll("li")[0].childNodes[1].firstChild
+PASS selection.anchorNode is $("test").querySelectorAll("li")[0].childNodes[0]
PASS selection.anchorOffset is 0
PASS selection.focusNode is $("test").querySelectorAll("li a")[3].firstChild
-PASS selection.focusOffset is 5
+PASS selection.focusOffset is 4
PASS successfullyParsed is true
TEST COMPLETE
diff --git a/LayoutTests/editing/selection/extend-by-word-002.html b/LayoutTests/editing/selection/extend-by-word-002.html
index d963877..1259c57 100644
--- a/LayoutTests/editing/selection/extend-by-word-002.html
+++ b/LayoutTests/editing/selection/extend-by-word-002.html
@@ -62,10 +62,10 @@
for (var i = 0; i < 6; ++i)
selection.modify('extend', 'forward', 'word');
shouldBeEqualToString('selection.type', 'Range');
-shouldBe('selection.anchorNode', '$("test").querySelectorAll("li")[0].childNodes[1].firstChild');
+shouldBe('selection.anchorNode', '$("test").querySelectorAll("li")[0].childNodes[0]');
shouldBe('selection.anchorOffset', '0');
shouldBe('selection.focusNode', '$("test").querySelectorAll("li a")[3].firstChild');
-shouldBe('selection.focusOffset', '5');
+shouldBe('selection.focusOffset', '4');
if (window.testRunner)
$('container').outerHTML = '';
</script>
diff --git a/LayoutTests/fast/css-generated-content/empty-first-letter-with-columns-crash-expected.txt b/LayoutTests/fast/css-generated-content/empty-first-letter-with-columns-crash-expected.txt
new file mode 100644
index 0000000..12fb55c
--- /dev/null
+++ b/LayoutTests/fast/css-generated-content/empty-first-letter-with-columns-crash-expected.txt
@@ -0,0 +1,2 @@
+
+PASS. Test didn't crash.
diff --git a/LayoutTests/fast/css-generated-content/empty-first-letter-with-columns-crash.html b/LayoutTests/fast/css-generated-content/empty-first-letter-with-columns-crash.html
new file mode 100644
index 0000000..706bd8f
--- /dev/null
+++ b/LayoutTests/fast/css-generated-content/empty-first-letter-with-columns-crash.html
@@ -0,0 +1,21 @@
+<!DOCTYPE html>
+<style>
+.slider { -webkit-appearance: media-volume-slider-mute-button; }
+*:first-letter { pointer-events: fill; }
+</style>
+<body style="-webkit-column-count: 2">
+ <div>
+ <button></button>
+ <table id="table" style="-webkit-column-span: all"></table>
+ </div>
+<script>
+document.body.offsetHeight;
+document.body.appendChild(document.createElement('div'));
+document.getElementById('table').className = 'slider'
+
+if (window.testRunner)
+ testRunner.dumpAsText();
+
+document.write("PASS. Test didn't crash.");
+</script>
+</body>
diff --git a/LayoutTests/fast/css-generated-content/quote-first-letter-expected.html b/LayoutTests/fast/css-generated-content/quote-first-letter-expected.html
index 01a07a5..9ad8763 100644
--- a/LayoutTests/fast/css-generated-content/quote-first-letter-expected.html
+++ b/LayoutTests/fast/css-generated-content/quote-first-letter-expected.html
@@ -1,11 +1,11 @@
<!DOCTYPE html>
<style>
- .first-letter { color: red; }
+ .quote:first-letter { color: red; }
.line { position: absolute; top: 50px; left: 10px; }
</style>
" "
<span class="line">
- <span class="first-letter">'</span>Should not crash or assert and all four quotes should be displayed.'
+ <span class="quote">'</span>Should not crash or assert and all four quotes should be displayed.'
</span>
diff --git a/Source/core/rendering/RenderBlock.cpp b/Source/core/rendering/RenderBlock.cpp
index 24f3f02..ed818ee 100644
--- a/Source/core/rendering/RenderBlock.cpp
+++ b/Source/core/rendering/RenderBlock.cpp
@@ -5740,6 +5740,10 @@
while (length < text.length() && shouldSkipForFirstLetter((text)[length]))
length++;
+ // Bail if we didn't find a letter
+ if (text.length() && length == text.length())
+ return 0;
+
// Account for first letter.
length++;
@@ -5758,8 +5762,10 @@
return length;
}
-void RenderBlock::createFirstLetterRenderer(RenderObject* firstLetterBlock, RenderObject* currentChild)
+void RenderBlock::createFirstLetterRenderer(RenderObject* firstLetterBlock, RenderObject* currentChild, unsigned length)
{
+ ASSERT(length && currentChild->isText());
+
RenderObject* firstLetterContainer = currentChild->parent();
RenderStyle* pseudoStyle = styleForFirstLetter(firstLetterBlock, firstLetterContainer);
RenderObject* firstLetter = 0;
@@ -5778,11 +5784,6 @@
String oldText = textObj->originalText();
ASSERT(oldText.impl());
- unsigned length = firstLetterLength(oldText);
-
- if (!length)
- return;
-
// Construct a text fragment for the text after the first letter.
// This text fragment might be empty.
RenderTextFragment* remainingText =
@@ -5821,12 +5822,18 @@
// Drill into inlines looking for our first text child.
RenderObject* currChild = firstLetterBlock->firstChild();
+ unsigned length = 0;
while (currChild) {
- if (currChild->isText())
- break;
- if (currChild->isListMarker())
+ if (currChild->isText()) {
+ // FIXME: If there is leading punctuation in a different RenderText than
+ // the first letter, we'll not apply the correct style to it.
+ length = firstLetterLength(toRenderText(currChild)->originalText());
+ if (length)
+ break;
currChild = currChild->nextSibling();
- else if (currChild->isFloatingOrOutOfFlowPositioned()) {
+ } else if (currChild->isListMarker()) {
+ currChild = currChild->nextSibling();
+ } else if (currChild->isFloatingOrOutOfFlowPositioned()) {
if (currChild->style()->styleType() == FIRST_LETTER) {
currChild = currChild->firstChild();
break;
@@ -5859,7 +5866,7 @@
// adding and removing children of firstLetterContainer.
LayoutStateDisabler layoutStateDisabler(view());
- createFirstLetterRenderer(firstLetterBlock, currChild);
+ createFirstLetterRenderer(firstLetterBlock, currChild, length);
}
// Helper methods for obtaining the last line, computing line counts and heights for line counts
diff --git a/Source/core/rendering/RenderBlock.h b/Source/core/rendering/RenderBlock.h
index d584de1..b379c5e 100644
--- a/Source/core/rendering/RenderBlock.h
+++ b/Source/core/rendering/RenderBlock.h
@@ -576,7 +576,7 @@
// Called to lay out the legend for a fieldset or the ruby text of a ruby run.
virtual RenderObject* layoutSpecialExcludedChild(bool /*relayoutChildren*/, SubtreeLayoutScope&) { return 0; }
- void createFirstLetterRenderer(RenderObject* firstLetterBlock, RenderObject* currentChild);
+ void createFirstLetterRenderer(RenderObject* firstLetterBlock, RenderObject* currentChild, unsigned length);
void updateFirstLetterStyle(RenderObject* firstLetterBlock, RenderObject* firstLetterContainer);
Node* nodeForHitTest() const;
