Make first-letter style to work with editing

Make first-letter style to work with editing

This patch changes to consider a renderer for first letter with first-letter style is different from renderer associate to Node object, e.g. node->renderer(), for following actions to work:
 - Mouse click
 - Caret/Selection motion implemented in Position and VisiblePosition
 - Caret rendering
 - Selection range rendering

For mouse click, this patch adds two function HitTestResult::renderer() and HitTestResult::setIsFirstLetter() to carry first letter hit from renderer to event handler. Note: Caching renderer in HitTestResult doesn't work because during event handling layout can be changed.

For caret motion, this patch adds Position::rendererOfAnchorNode() and PostionIterator::renderer() to get renderer for first-letter when offset in DOM node points first letter rather than DOM node associate renderer. This change leads replacement from Position::anchorNode()->renderer() to Position::renderer() in Editor code and other use sites.

For selection rendering, this patch changes FrameSelection::updateAppearance() to call RenderView::setSelection() with  Postion::renderer() and Postion::offsetInRendererOfAnchorNode() 

Test file chagnes:
* editing/deleting/merge-paragraph-with-first-letter.html
 Split(insert line break) a paragraph with first letter and merge(delete line break) to restore original state.

* editing/selection/caret-with-first-letter.html
 Move caret around a first letter.

* editing/selection/click-first-letter.html
 Set caret at first letter.
 Set caret at middle of text

* editing/selection/extend-by-word-002.html
 Changed to JS test for portability with considering selection containing a first letter.

* editing/selection/select-first-letter.html
 Check rendering selection range contains first-letter CSS property.

* editing/text-iterator/first-letter-word-boundary.html
 First letter text fragment renderer contains two characters, a space and a letter.

BUG=174349

[yosin_UTC9, 2013-08-02 01:22:18]

Hi,
Could you review this patch?
Thanks in advance.

P.S.
Mac and Win bots failures aren't related to this patch.

[tkent, 2013-08-02 02:22:18]

This looks workable, but I'm not confident that the design is good.  I'd like to
ask Ojan's opinion.

https://codereview.chromium.org/20681004/diff/41001/Source/core/rendering/Ren...
File Source/core/rendering/RenderText.cpp (right):

https://codereview.chromium.org/20681004/diff/41001/Source/core/rendering/Ren...
Source/core/rendering/RenderText.cpp:531: return
box->renderer()->createPositionWithAffinity(offset +
toRenderText(box->renderer())->textStartOffset(), affinity);
Is it safe to assume box->renderer() is RenderText?

https://codereview.chromium.org/20681004/diff/41001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTextFragment.h (right):

https://codereview.chromium.org/20681004/diff/41001/Source/core/rendering/Ren...
Source/core/rendering/RenderTextFragment.h:67: virtual void
updateHitTestResult(HitTestResult&, const LayoutPoint&);
nit: OVERRIDE

[tkent, 2013-08-02 03:25:48]

https://codereview.chromium.org/20681004/diff/41001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/20681004/diff/41001/Source/core/rendering/Ren...
Source/core/rendering/RenderTextFragment.cpp:59: if (RenderText* found =
findRenderText(current))
Using RenderObject::nextInPreOrder is preferable to recursion.

[yosin_UTC9, 2013-08-02 05:01:55]

Updated.
PTAL

https://codereview.chromium.org/20681004/diff/41001/Source/core/rendering/Ren...
File Source/core/rendering/RenderText.cpp (right):

https://codereview.chromium.org/20681004/diff/41001/Source/core/rendering/Ren...
Source/core/rendering/RenderText.cpp:531: return
box->renderer()->createPositionWithAffinity(offset +
toRenderText(box->renderer())->textStartOffset(), affinity);
On 2013/08/02 02:22:19, tkent wrote:
> Is it safe to assume box->renderer() is RenderText?
No. Good catch!

https://codereview.chromium.org/20681004/diff/41001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTextFragment.cpp (right):

https://codereview.chromium.org/20681004/diff/41001/Source/core/rendering/Ren...
Source/core/rendering/RenderTextFragment.cpp:59: if (RenderText* found =
findRenderText(current))
On 2013/08/02 03:25:48, tkent wrote:
> Using RenderObject::nextInPreOrder is preferable to recursion.

Done.

https://codereview.chromium.org/20681004/diff/41001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTextFragment.h (right):

https://codereview.chromium.org/20681004/diff/41001/Source/core/rendering/Ren...
Source/core/rendering/RenderTextFragment.h:67: virtual void
updateHitTestResult(HitTestResult&, const LayoutPoint&);
On 2013/08/02 02:22:19, tkent wrote:
> nit: OVERRIDE

Done.

[yosin_UTC9, 2013-08-08 04:34:01]

Updated for Caret.{cpp,h} spawned from FrameSelection.{cpp,h}

Could you review this patch?
Thanks in advance.

[yosin_UTC9, 2013-09-06 08:18:18]

+eseidel +leviw

4 weeks passed since the last message.

Could you review this patch?
Thanks in advance.

[eseidel, 2013-09-09 20:28:26]

Ojan is much more up to date with this code than I.

I thought that Positions were all about DOM positions and that VisiblePosition
was what you used when dealing with rendered content?  I'm concerned that it's
difficult for Position::renderer() to make sense for all possible clients of
that method.

What renderer should we return for a before-anchor'd position?

https://codereview.chromium.org/20681004/diff/59001/Source/core/dom/Position.cpp
File Source/core/dom/Position.cpp (right):

https://codereview.chromium.org/20681004/diff/59001/Source/core/dom/Position....
Source/core/dom/Position.cpp:464: RenderObject* renderer =
m_anchorNode->renderer();
Positions can be relative to anchor nodes other than being inside them, right?

https://codereview.chromium.org/20681004/diff/59001/Source/core/dom/Position....
Source/core/dom/Position.cpp:476: case PositionIsBeforeAnchor:
Wouldn't this possibly have a different renderer depending on what you're trying
to do with this position?

[eseidel, 2013-09-09 20:29:11]

Also, please squeak louder.  Start scheduling meetings on my or Ojan's calendar
if need be.  It is completely unacceptable for us to let your patches sit for
more than a couple days, let alone 3 months!

[ojan, 2013-09-12 23:54:38]

Can you split the hit testing and caret-motion parts of this into different
patches? It's hard to review such a large patch.

I don't have a strong opinion on whether it's OK to expose rendering information
from Position. It's a bit unfortunate and would be good to avoid if possible
though. The less that Position knows about the rendertree, the better.

[yosin_UTC9, 2013-09-20 08:54:01]

On 2013/09/12 23:54:38, ojan wrote:
> Can you split the hit testing and caret-motion parts of this into different
> patches? It's hard to review such a large patch.
I tried to do so, although I couldn't write test for hit testing.
I removed changes of replacing anchorNode()->renderer(), patch #4 is smaller
than previous patch.
Other than Position.cpp and Caret.cpp, changes are small.

> I don't have a strong opinion on whether it's OK to expose rendering
information
> from Position. It's a bit unfortunate and would be good to avoid if possible
> though. The less that Position knows about the rendertree, the better.
Position should offer mapping from Position to RenderObject because Node is
associated to more than
one RenderObject when :first-letter CSS property used. This means
Position.anchorNode()->renderer()
returns wrong RenderObject if Position points first letter.

[yosin_UTC9, 2013-09-20 09:30:26]

PTAL

https://codereview.chromium.org/20681004/diff/59001/Source/core/dom/Position.cpp
File Source/core/dom/Position.cpp (right):

https://codereview.chromium.org/20681004/diff/59001/Source/core/dom/Position....
Source/core/dom/Position.cpp:464: RenderObject* renderer =
m_anchorNode->renderer();
On 2013/09/09 20:28:27, eseidel wrote:
> Positions can be relative to anchor nodes other than being inside them, right?

This function is replacement of Position.anchorNode()->renderer(), so I renamed
this function to rendererOfAnchorNode().

https://codereview.chromium.org/20681004/diff/59001/Source/core/dom/Position....
Source/core/dom/Position.cpp:476: case PositionIsBeforeAnchor:
On 2013/09/09 20:28:27, eseidel wrote:
> Wouldn't this possibly have a different renderer depending on what you're
trying
> to do with this position?

This function does as same as deprecatedEditingOffset() except for :first-letter
support. Only FrameSelection::updateApperance() uses this. When we move Range
based FrameSelection, we can get rid of this function.

[ojan, 2013-09-23 23:01:44]

It's very difficult to review such a large patch...I did my best.

https://codereview.chromium.org/20681004/diff/82001/Source/core/dom/Position.cpp
File Source/core/dom/Position.cpp (right):

https://codereview.chromium.org/20681004/diff/82001/Source/core/dom/Position....
Source/core/dom/Position.cpp:882: ContainerNode* Position::findParent(const
Node* node)
This looks unused.

https://codereview.chromium.org/20681004/diff/82001/Source/core/dom/PositionI...
File Source/core/dom/PositionIterator.cpp (right):

https://codereview.chromium.org/20681004/diff/82001/Source/core/dom/PositionI...
Source/core/dom/PositionIterator.cpp:107: RenderObject*
PositionIterator::renderer() const
This is confusing since it's only the renderer of the anchorNode. Since this is
called from only once place, I'm not sure this function provides much value. I'd
just inline this code into PositionIterator::isCandidate.

https://codereview.chromium.org/20681004/diff/82001/Source/core/dom/PositionI...
Source/core/dom/PositionIterator.cpp:109: if (!m_anchorNode)
This null check is redundant with the one in PositionIterator::isCandidate.

https://codereview.chromium.org/20681004/diff/82001/Source/core/editing/Caret...
File Source/core/editing/Caret.cpp (right):

https://codereview.chromium.org/20681004/diff/82001/Source/core/editing/Caret...
Source/core/editing/Caret.cpp:123: RenderObject*
CaretBase::caretRenderer(RenderObject* renderer)
This function looks to me like it's only used in the one place below. No need to
have the overloaded function. Just the one that takes Position is enough.

[yosin_UTC9, 2013-10-04 10:27:13]

I make another patch for hit testing part, because I found how to test it:
https://codereview.chromium.org/25668008/