Resolve flickering the text selection when hitting culled inline.

Resolve flickering the text selection when hitting culled inline.

Flickering is observed on text selection when hitting culled
inline, because culled inline doesn't get the right position
from RenderInline::positionForPoint for the selection.

This patch changes the condition to firstLineBox to
firstLineBoxIncludingCulling for culled inline.

BUG=460077
R=pdr@chromium.org
TEST=fast/events/drag-selects-culled-inlines.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190622

[Miyoung Shin(g), 2015-02-16 06:56:24]

Patchset #1 (id:1) has been deleted

[Miyoung Shin(g), 2015-02-16 07:04:44]

@pdr, PTAL.

I tested this patch with reverting the patch at http://crrev.com/914573003 and
it's ok.
We could apply it again if this patch is merged.

[pdr., 2015-02-16 20:24:10]

https://codereview.chromium.org/929883003/diff/20001/LayoutTests/fast/events/...
File LayoutTests/fast/events/drag-selects-culled-inlines.html (right):

https://codereview.chromium.org/929883003/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-selects-culled-inlines.html:6: <div id="test"
class="style1" height="106">
Is this explicit height required?

https://codereview.chromium.org/929883003/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-selects-culled-inlines.html:30:
eventSender.mouseMoveTo(element.offsetLeft + 10, element.offsetHeight - 25);
offsetHeight - 25 may be flaky on other platforms. I think you're trying to move
between two lines? If so, can you make this explicit by moving to:
(line1.offsetTop line1.offsetHeight + line2.offsetTop) / 2

https://codereview.chromium.org/929883003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderInline.cpp (right):

https://codereview.chromium.org/929883003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderInline.cpp:838: if (firstLineBoxIncludingCulling())
{
There are 35 other callers of firstLineBox(). Is this the only one that needs to
be changed? How do we know which to use?

[Miyoung Shin(g), 2015-02-17 05:01:54]

https://codereview.chromium.org/929883003/diff/20001/LayoutTests/fast/events/...
File LayoutTests/fast/events/drag-selects-culled-inlines.html (right):

https://codereview.chromium.org/929883003/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-selects-culled-inlines.html:6: <div id="test"
class="style1" height="106">
On 2015/02/16 20:24:09, pdr wrote:
> Is this explicit height required?

Done.

https://codereview.chromium.org/929883003/diff/20001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-selects-culled-inlines.html:30:
eventSender.mouseMoveTo(element.offsetLeft + 10, element.offsetHeight - 25);
On 2015/02/16 20:24:09, pdr wrote:
> offsetHeight - 25 may be flaky on other platforms. I think you're trying to
move
> between two lines? If so, can you make this explicit by moving to:
> (line1.offsetTop line1.offsetHeight + line2.offsetTop) / 2

Done.I changed it to use only the offsetTop and offsetHeight  of the last line.

https://codereview.chromium.org/929883003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderInline.cpp (right):

https://codereview.chromium.org/929883003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderInline.cpp:838: if (firstLineBoxIncludingCulling())
{
On 2015/02/16 20:24:09, pdr wrote:
> There are 35 other callers of firstLineBox(). Is this the only one that needs
to
> be changed? 
We need only this to solve the problem that flickers on the text selection.
But I'm not sure if we need to change firstLineBox to
firstLineBoxIncludingCulling more for other cases.

> How do we know which to use?

IMHO, we should choose firstLineBoxIncludingCulling() if we want to calculate
the position such as offsetLeft() and generateLineBoxRects().
But we don't need to use it for the painting job.
For example,
I think we don't need to use firstLineBoxIncludingCulling in
RenderInline::invalidateDisplayItemClients,
because culled inline doesn't have own displaying area.

As you said, there are many other callers of firstLineBox(),
but I can see to branch out the code flow between the normal inline and culled
inline using alwaysCreateLineBoxes() in almost places calling firstLineBox().

For example.
RenderInline::linesBoundingBox() {
...
    if (!alwaysCreateLineBoxes()) {
	...
	}
..
}

If my comment is not clear, let me know.

[pdr., 2015-02-18 00:00:49]

LGTM

https://codereview.chromium.org/929883003/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/drag-selects-culled-inlines.html (right):

https://codereview.chromium.org/929883003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-selects-culled-inlines.html:18: var testDone =
false;
Nit: lets rename this selectionComplete

https://codereview.chromium.org/929883003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-selects-culled-inlines.html:19: description('Test
that flickering is observed on text selection when culled inline is selected')
Nit: missing semicolon.

[Miyoung Shin(g), 2015-02-18 08:28:18]

New patchsets have been uploaded after l-g-t-m from pdr@chromium.org

[Miyoung Shin(g), 2015-02-18 08:28:26]

Thank you for your kind review.

https://codereview.chromium.org/929883003/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/drag-selects-culled-inlines.html (right):

https://codereview.chromium.org/929883003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-selects-culled-inlines.html:18: var testDone =
false;
On 2015/02/18 00:00:49, pdr wrote:
> Nit: lets rename this selectionComplete

Done.

https://codereview.chromium.org/929883003/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/drag-selects-culled-inlines.html:19: description('Test
that flickering is observed on text selection when culled inline is selected')
On 2015/02/18 00:00:49, pdr wrote:
> Nit: missing semicolon.

Done.

[Miyoung Shin(g), 2015-02-18 11:36:30]

On 2015/02/18 08:28:26, Miyoung Shin(g) wrote:
> Thank you for your kind review.
> 
>
https://codereview.chromium.org/929883003/diff/40001/LayoutTests/fast/events/...
> File LayoutTests/fast/events/drag-selects-culled-inlines.html (right):
> 
>
https://codereview.chromium.org/929883003/diff/40001/LayoutTests/fast/events/...
> LayoutTests/fast/events/drag-selects-culled-inlines.html:18: var testDone =
> false;
> On 2015/02/18 00:00:49, pdr wrote:
> > Nit: lets rename this selectionComplete
> 
> Done.
> 
>
https://codereview.chromium.org/929883003/diff/40001/LayoutTests/fast/events/...
> LayoutTests/fast/events/drag-selects-culled-inlines.html:19: description('Test
> that flickering is observed on text selection when culled inline is selected')
> On 2015/02/18 00:00:49, pdr wrote:
> > Nit: missing semicolon.
> 
> Done.

Could I commit it with your LGTM?

[pdr., 2015-02-18 19:18:09]

On 2015/02/18 at 11:36:30, myid.o.shin wrote:
> On 2015/02/18 08:28:26, Miyoung Shin(g) wrote:
> > Thank you for your kind review.
> > 
> >
https://codereview.chromium.org/929883003/diff/40001/LayoutTests/fast/events/...
> > File LayoutTests/fast/events/drag-selects-culled-inlines.html (right):
> > 
> >
https://codereview.chromium.org/929883003/diff/40001/LayoutTests/fast/events/...
> > LayoutTests/fast/events/drag-selects-culled-inlines.html:18: var testDone =
> > false;
> > On 2015/02/18 00:00:49, pdr wrote:
> > > Nit: lets rename this selectionComplete
> > 
> > Done.
> > 
> >
https://codereview.chromium.org/929883003/diff/40001/LayoutTests/fast/events/...
> > LayoutTests/fast/events/drag-selects-culled-inlines.html:19:
description('Test
> > that flickering is observed on text selection when culled inline is
selected')
> > On 2015/02/18 00:00:49, pdr wrote:
> > > Nit: missing semicolon.
> > 
> > Done.
> 
> Could I commit it with your LGTM?

Please file a new bug for this, as crbug.com/455672 is getting pretty crazy and
this isn't strictly fixing that bug. Then update the BUG= line in the change
description here to point to the new bug, and commit.

[Miyoung Shin(g), 2015-02-19 16:29:53]

The CQ bit was checked by myid.o.shin@gmail.com

[commit-bot: I haz the power, 2015-02-19 16:30:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/929883003/60001

[commit-bot: I haz the power, 2015-02-19 21:00:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-19 21:00:53]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/4...)

[Miyoung Shin(g), 2015-02-22 05:38:57]

The CQ bit was checked by myid.o.shin@gmail.com

[commit-bot: I haz the power, 2015-02-22 05:39:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/929883003/60001

[commit-bot: I haz the power, 2015-02-22 07:04:14]

Committed patchset #3 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190622