[webkit-reviews] review denied: [Bug 39863] innerText does not include a letter with :first-letter applied : [Attachment 57316] Proposed Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yoshiki Hayashi
<yhayashi at google.com>'s request for review:
Bug 39863: innerText does not include a letter with :first-letter applied
https://bugs.webkit.org/show_bug.cgi?id=39863

Attachment 57316: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=57316&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.