Apply the correct style to first-letter pseudo elements composed of different renderers

Source/core/rendering/RenderBlock.cpp

Index: Source/core/rendering/RenderBlock.cpp
diff --git a/Source/core/rendering/RenderBlock.cpp b/Source/core/rendering/RenderBlock.cpp
index a23bfbb8af6da719c0329019d101c1b0f6ab86d1..f72d4925810e2abc29e458e437444a23701af77f 100644
--- a/Source/core/rendering/RenderBlock.cpp
+++ b/Source/core/rendering/RenderBlock.cpp
@@ -4,6 +4,7 @@
  *           (C) 2007 David Smith (catfish.man@gmail.com)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) Research In Motion Limited 2010. All rights reserved.
+ * Copyright (C) 2014 Samsung Electronics. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -4106,40 +4107,6 @@ void RenderBlock::updateFirstLetterStyle(RenderObject* firstLetterBlock, RenderO
     }
 }
 
-static inline unsigned firstLetterLength(const String& text)
-{
-    unsigned length = 0;
-    unsigned textLength = text.length();
-
-    // Account for leading spaces first.
-    while (length < textLength && isSpaceForFirstLetter(text[length]))
-        length++;
-
-    // Now account for leading punctuation.
-    while (length < textLength && isPunctuationForFirstLetter(text[length]))
-        length++;
-
-    // Bail if we didn't find a letter before the end of the text or before a space.
-    if (isSpaceForFirstLetter(text[length]) || (textLength && length == textLength))
-        return 0;
-
-    // Account the next character for first letter.
-    length++;
-
-    // Keep looking allowed punctuation for the :first-letter.
-    for (unsigned scanLength = length; scanLength < textLength; ++scanLength) {
-        UChar c = text[scanLength];
-
-        if (!isPunctuationForFirstLetter(c))
-            break;
-
-        length = scanLength + 1;
-    }
-
-    // FIXME: If textLength is 0, length may still be 1!
-    return length;
-}
-
 void RenderBlock::createFirstLetterRenderer(RenderObject* firstLetterBlock, RenderObject* currentChild, unsigned length)
 {
     ASSERT(length && currentChild->isText());
@@ -4184,69 +4151,203 @@ void RenderBlock::createFirstLetterRenderer(RenderObject* firstLetterBlock, Rend
     textObj->destroy();
 }
 
-void RenderBlock::updateFirstLetter()
+static String rendererTextForFirstLetter(RenderObject* renderer)
 {
-    if (!document().styleEngine()->usesFirstLetterRules())
-        return;
-    // Don't recur
-    if (style()->styleType() == FIRST_LETTER)
-        return;
+    ASSERT(renderer->isText());
 
-    // FIXME: We need to destroy the first-letter object if it is no longer the first child. Need to find
-    // an efficient way to check for that situation though before implementing anything.
-    RenderObject* firstLetterBlock = findFirstLetterBlock(this);
-    if (!firstLetterBlock)
-        return;
+    RenderText* textRenderer = toRenderText(renderer);
+    String result = textRenderer->originalText();
 
-    // Drill into inlines looking for our first text child.
-    RenderObject* currChild = firstLetterBlock->firstChild();
+    // BR and Word breaks are not allowed to be part of the first-letter
+    // pseudo element, so we don't fallback to text() in those cases.
+    if (!result && !textRenderer->isBR() && !textRenderer->isWordBreak())

[eseidel, 2014/03/27 21:48:11]

Are you sure !result does what you want?   That's checking null, are these empty
or null?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[mario.prada, 2014/03/28 01:08:48]

My intention here was to check null and, if that's the case, fallback to text().
But if I'm getting an empty string after calling originalText() then I believe I
should no call text() because that would mean that I valid string was found,
despite of it happening to be an empty one (but of course, I might be wrong)

+        result = textRenderer->text();
+
+    return result;
+}
+
+static bool shouldConsiderTextForFirstLetter(const String& text, unsigned& length, bool& leadingSpacesChecked, bool& leadingPunctuationChecked, bool& firstLetterRenderersFound)
+{
+    // Early return in case we encounter the wrong characters at the beginning.
+    if ((leadingSpacesChecked && isSpaceForFirstLetter(text[0]))

[eseidel, 2014/03/27 21:45:30]

What if text is empty?

[mario.prada, 2014/03/28 01:08:48]

I think in that case the flow will continue all the way down until the end of
this block. Anyway, I agree that perhaps an early return would be a good thing
here, so I will add one.

+        || (firstLetterRenderersFound && !isPunctuationForFirstLetter(text[0])))
+        return false;
+
+    // Now start looking for valid characters for the firs-letter pseudo element.

[eseidel, 2014/03/27 21:45:30]

"firs-letter"

[mario.prada, 2014/03/28 01:08:48]

English is not my mother tongue, so I won't discuss this :). I'll change it

+    unsigned textLength = text.length();
+    length = 0;
+
+    if (!leadingSpacesChecked && length < textLength) {
+        while (length < textLength && isSpaceForFirstLetter(text[length]))
+            length++;
+
+        // We now we finished checking if there are still more characters.

[eseidel, 2014/03/27 21:45:30]

We now we?

[mario.prada, 2014/03/28 01:08:48]

:) Ok. I'll fix this too, no problem

+        if (length < textLength)
+            leadingSpacesChecked = true;
+    }
+
+    // If leading spaces have been checked, account now for leading punctuation.
+    if (!leadingPunctuationChecked && length < textLength) {
+        while (length < textLength && isPunctuationForFirstLetter(text[length]))
+            length++;
+
+        // We now we finished checking if there are still more characters.
+        if (length < textLength)
+            leadingPunctuationChecked = true;
+    }
+
+    // Now account for the next character for first letter and the trailing punctuation (if any).
+    if (!firstLetterRenderersFound && length < textLength) {
+        // Now spaces are allowed between leading punctuation and the letter.
+        if (isSpaceForFirstLetter(text[length]))
+            return false;
+
+        firstLetterRenderersFound = true;
+        length++;
+    }
+
+    // Keep looking allowed punctuation for the :first-letter.
+    for (unsigned scanLength = length; scanLength < textLength; ++scanLength) {
+        UChar c = text[scanLength];
+        if (!isPunctuationForFirstLetter(c))
+            break;
+
+        length = scanLength + 1;
+    }
+
+    // Tell the caller that we should keep calling this function for the text of the next renderer.
+    return true;
+}
+
+// This helper function fills the vector of RenderObjects with the ones that would be considered
+// as part of the first-letter pseudo element and returns the number of characters in the last
+// renderer found that should be considered as part of such a pseudo element (the other renderers
+// in the Vector will be considered "in full"). Also, this function might update the firstLetterBlock
+// pointer if a lower-level node with first-letter style is found while traversing the subtree under
+// the RenderObject originally passed through that parameter.
+static unsigned findTextRenderersForFirstLetterBlock(RenderObject*& firstLetterBlock, Vector<RenderObject*>& renderers)
+{
     unsigned length = 0;
+    unsigned previousLength = 0;
+    bool leadingSpacesChecked = false;
+    bool leadingPunctuationChecked = false;
+    bool firstLetterRenderersFound = false;
+
+    // Drill into inlines looking for the render objects to be transformed into first-letter pseudoelements.
+    RenderObject* currChild = firstLetterBlock->firstChild();
     while (currChild) {
         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());
+            previousLength = length;
+
+            // shouldConsiderTextForFirstLetter() will set the right values for the 'length' variable and those booleans.
+            String text = rendererTextForFirstLetter(currChild);
+            if (!shouldConsiderTextForFirstLetter(text, length, leadingSpacesChecked, leadingPunctuationChecked, firstLetterRenderersFound))
+                break;
+
+            // Save the renderer if anything valis was found, so we can apply the style to later.
             if (length)
+                renderers.append(currChild);
+
+            // No need to keep looking if we haven't found anything with the
+            // current renderer or if we already made a decision.
+            if (!length || length < text.length())
                 break;
-            currChild = currChild->nextSibling();
+
+            // We need to look the next object traversing the tree in preorder as if the current renderer
+            // was a leaf node (which probably is anyway) but without leaving the scope of the parent block.
+            currChild = currChild->nextInPreOrderAfterChildren(firstLetterBlock);
         } else if (currChild->isListMarker()) {
             currChild = currChild->nextSibling();
         } else if (currChild->isFloatingOrOutOfFlowPositioned()) {
             if (currChild->style()->styleType() == FIRST_LETTER) {
                 currChild = currChild->firstChild();
+                if (currChild) {
+                    // If found a floating/out-of-flow element with the first_letter
+                    // style already applied, it means it has been previously identified
+                    // and so we should discard whatever we found so far and use that.
+                    firstLetterRenderersFound = true;
+                    renderers.append(currChild);
+                }
                 break;
             }
             currChild = currChild->nextSibling();
         } else if (currChild->isReplaced() || currChild->isRenderButton() || currChild->isMenuList())
             break;
         else if (currChild->style()->hasPseudoStyle(FIRST_LETTER) && currChild->canHaveGeneratedChildren())  {
-            // We found a lower-level node with first-letter, which supersedes the higher-level style
+            // We found a lower-level node with first-letter, which supersedes the higher-level style.
             firstLetterBlock = currChild;
             currChild = currChild->firstChild();
         } else
             currChild = currChild->firstChild();
     }
 
-    if (!currChild)
+    if (!firstLetterRenderersFound) {
+        // Empty the list of renderers if we did not find a correct set of them
+        // to further generate new elements to apply the first-letter style over.
+        renderers.clear();
+    } else if (!length) {
+        // If we found renderers for the first-letter and length is zero it means that the last valid
+        // renderer was added in the previous iteration of the loop, so we consider its length instead.
+        length = previousLength;
+    }
+
+    // Return the number of characters used for the first-letter pseudo element coming
+    // from the last text renderer found, needed to create the last generated renderer.
+    return renderers.isEmpty() ? 0 : length;
+}
+
+void RenderBlock::updateFirstLetter()
+{
+    if (!document().styleEngine()->usesFirstLetterRules())
         return;
 
-    // If the child already has style, then it has already been created, so we just want
-    // to update it.
-    if (currChild->parent()->style()->styleType() == FIRST_LETTER) {
-        updateFirstLetterStyle(firstLetterBlock, currChild);
+    // Don't recur.

[eseidel, 2014/03/27 21:45:30]

recurse?

[mario.prada, 2014/03/28 01:08:48]

That's not from my patch, I've just moved it around. Anyway, I will change that
(to be honest, I don't understand what 'recur' means either)

+    if (style()->styleType() == FIRST_LETTER)
         return;
-    }
 
-    // FIXME: This black-list of disallowed RenderText subclasses is fragile.
-    // Should counter be on this list? What about RenderTextFragment?
-    if (!currChild->isText() || currChild->isBR() || toRenderText(currChild)->isWordBreak())
+    // FIXME: We need to destroy the first-letter object if it is no longer the first child. Need to find
+    // an efficient way to check for that situation though before implementing anything.
+    RenderObject* firstLetterBlock = findFirstLetterBlock(this);
+    if (!firstLetterBlock)
         return;
 
-    // Our layout state is not valid for the repaints we are going to trigger by
-    // adding and removing children of firstLetterContainer.
-    LayoutStateDisabler layoutStateDisabler(*this);
+    // Find the renderers to apply the first-letter style over.
+    Vector<RenderObject*> renderers;
+    unsigned lengthForLastRenderer = findTextRenderersForFirstLetterBlock(firstLetterBlock, renderers);
+    if (renderers.isEmpty())
+        return;
+
+    // Create the new renderers for the first-letter pseudo elements.
+    for (Vector<RenderObject*>::const_iterator it = renderers.begin(); it != renderers.end(); ++it) {
+        RenderObject* currentRenderer = *it;
+        ASSERT(currentRenderer->isText());
+
+        // If the child already has style, then it has already been created, so we just want
+        // to update it.
+        if (currentRenderer->parent()->style()->styleType() == FIRST_LETTER) {
+            updateFirstLetterStyle(firstLetterBlock, currentRenderer);
+            continue;
+        }
+
+        // FIXME: This black-list of disallowed RenderText subclasses is fragile.
+        // Should counter be on this list? What about RenderTextFragment?
+        RenderText* textRenderer = toRenderText(currentRenderer);
+        if (!currentRenderer->isText() || currentRenderer->isBR() || textRenderer->isWordBreak())
+            continue;
+
+        // Our layout state is not valid for the repaints we are going to trigger by
+        // adding and removing children of firstLetterContainer.
+        LayoutStateDisabler layoutStateDisabler(*this);
 
-    createFirstLetterRenderer(firstLetterBlock, currChild, length);
+        unsigned lengthForRenderer = 0;
+        if (currentRenderer == renderers.last())
+            lengthForRenderer = lengthForLastRenderer;
+        else
+            lengthForRenderer = rendererTextForFirstLetter(currentRenderer).length();
+
+        if (lengthForRenderer)
+            createFirstLetterRenderer(firstLetterBlock, currentRenderer, lengthForRenderer);
+    }
 }
 
 // Helper methods for obtaining the last line, computing line counts and heights for line counts