[Webkit-unassigned] [Bug 39863] innerText does not include a letter with :first-letter applied

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39863





--- Comment #7 from Yoshiki Hayashi <yhayashi at google.com>  2010-05-31 04:30:14 PST ---
Thank you for review!

(In reply to comment #5)

> It's frustrating that this ends up copying and pasting so much code. I would prefer a code path that somehow reused more of what was there for things like handling collapsed space and such.

I created a new method named handleTextNodeFirstLetter as suggested below.  I tried to reuse as much logic as possible but I couldn't get rid of collapsed space handling code from handleTextNodeFirstLetter because there are lots of early exit code in handleTextNode and in each of them, handleTextNodeFirstLetter needs to be called and needs to handle outputting a space character when needed.

I ended up littering around lots of handleTextNodeFirstLetter calling code in handleTextNode and handleTextBox but this should be more correct with respect to right to left language than my previous patch.

> I'd like to see the new logic factored out into a separate function so handleTextNode doesn't get so much bigger. That might also make it clearer how to factor this so it shares more code with the normal text node handling.

This is now done.

> > +        if (fragment->firstLetter() && fragment->firstLetter()->firstChild() && fragment->firstLetter()->firstChild()->isText() && toRenderText(fragment->firstLetter()->firstChild())->isTextFragment()) {
> > +            RenderTextFragment* firstLetter = static_cast<RenderTextFragment*>(fragment->firstLetter()->firstChild());
> 
> Is there a way to write this more generally? It seems fragile to assume here the particular arrangement of renderers. We don't want to have to modify the text iterator any time we change the details of how RenderTextFragment works.

In the new patch, I made the assumption that firstLetter is a container (either RenderBlock or RenderInline) that holds at least one RenderText in children.  I could change it to traverse the whole tree of firstLetter to output all RenderText in that tree but it looked overkill to me.  I can certainly change the code that way if you prefer it.

> 
> > +                emitText(m_node, firstLetter, 0, 1);
> 
> This assumes that the first letter will be a single character at the beginning of the DOM node. That's not necessarily true. The first letter could be a surrogate pair, for example. I think the text length needs to come from a computation rather than a hard-coded 1. And perhaps the offset too. For example there could be collapsed whitespace at the beginning of the text node. We need test cases that cover this, I think.
> 
> I could almost say review+, but I think the "0, 1" thing here is wrong so I'll say review- and ask you to cover that case correctly. I may be mistaken, and if so, please feel free to put this same patch up for review again after explaining what I got wrong.

You are absolutely right about that.  In addition to surrogate pairs, the first letter object could contain multiple letters like '---a' because punctuations are not considered when finding the character to apply :first-letter and thus it needs to hold all previous characters until first non-punctuation letter is found.

As to bug 6185, this patch doesn't fix the problem but I'll see if I can fix that as well.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.