[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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #57316|review?                     |review-
               Flag|                            |




--- Comment #5 from Darin Adler <darin at apple.com>  2010-05-28 08:43:38 PST ---
(From update of attachment 57316)
Looks good.

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'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.

> +        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.

> +                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.

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