Fix incorrect bounding box position for '\n'

Fix incorrect bounding box position for '\n'

absoluteQuadsForRange function generates unrealted empty quads
with even not overlapping InlineTextBox.
This fix considers inclusive InlineTextbox, overlapping InlineTextbox,
and exclusive InlineTextbox cases separately.
Considering exclusive InlineTextbox is still required because non
rendering characters(e.g. whitespace) are missing and to cover if range
offset is only in this area.

BUG=698038
Review-Url: https://codereview.chromium.org/2763013002
Cr-Commit-Position: refs/heads/master@{#460310}
Committed: https://chromium.googlesource.com/chromium/src/+/19d5f8b3d7cd8ba33ba3f2dd331c339384b299dd

[wanchang, 2017-03-21 12:44:10]

wanchang.ryu@lge.com changed reviewers:
+ skobes@chromium.org

[wanchang, 2017-03-21 12:44:11]

PTAL

[skobes, 2017-03-21 20:59:44]

skobes@chromium.org changed reviewers:
+ eae@chromium.org

[skobes, 2017-03-21 20:59:45]

I'm not the best reviewer for this.  Emil do you know this code (or who does)?

[eae, 2017-03-21 21:06:52]

Thanks for trying to fix this, I don't understand the difference that the
nonRounded vs rects vectors is trying to solve though. You push to both but if
there are any items in nonRounded the other list is discarded?

[wanchang, 2017-03-22 07:23:10]

On 2017/03/21 21:06:52, eae wrote:
> Thanks for trying to fix this, I don't understand the difference that the
> nonRounded vs rects vectors is trying to solve though. You push to both but if
> there are any items in nonRounded the other list is discarded?

Thanks for your comment.

I'd like to explain what I get from my debugging this issue.
LayoutText::absoluteQuadsForRange is expected to create FloatQuad vector of each
inlinetextbox between startOffset and endOffset. But with current implemenation,
it creates not only correct rect of InlineTextBox but also an empty FloatQuad
with adjecent inlinebox with startOffset or endOffset of given range.
If text is "a\n\nb" with "white-space: pre" style, it makes 4 inlinetextbox
box1 - 'a',
box2 - '\n'
box3 - '\n'
box4 - 'b'
With range offset (1,2) -- first '\n' is between range, absoluteQuadsForRange
creates three rects.
1. empty rect at position (right,top) of box1
2. empty rect of box2
3. empty rect at position (left,top) of box3
with this rect vector, Range::boundingRect checks all rects are empty, if so
just returns first rect
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Range...
In this case, empty rect of position (right,top) of box1 is returned.
This is incorrect and it should be empty rect of box2
I think this can be fixed by checking only the boxes overlappping between
start,end offset of range.
Since InlineTextBox is created only for rendered text (the leading or trailing
whitespace does not create the box) if range offset is between this non rendered
characters, non of rect created. To cover this, rect with not overlapping box is
pushed to nonRenderedVector and takes this if all of InlineTextBox are not
overlapping with range offset.

Thanks

[eae, 2017-03-24 18:21:07]

That makes sense, thanks for the explanation.
Would it be possible to refactor this to only populate one of the two structs
though without the disconnect between rounded/unrounded? Perhaps by branching
the loop?

[wanchang, 2017-03-27 05:29:13]

On 2017/03/24 18:21:07, eae wrote:
> That makes sense, thanks for the explanation.
> Would it be possible to refactor this to only populate one of the two structs
> though without the disconnect between rounded/unrounded? Perhaps by branching
> the loop?

Thanks for you comment.
I refactor to populate one vector by checking whether the rect of the box in the
range offset has been pushed.
Once the rect of box of the range has been pushed, it doesn't need to check
boxes out of offset range.

Please take a look, thanks

[eae, 2017-03-27 21:42:19]

eae@chromium.org changed reviewers:
+ kojii@chromium.org
- skobes@chromium.org

[eae, 2017-03-27 21:42:19]

This is much easier to read, thank you. I'm still not sure about the logic
though, adding kojii who knows this code well.

[kojii, 2017-03-29 03:29:33]

lgtm, it looks like there's a room to refactor, but better interop with Gecko is
good. Thank you for working on this.

[wanchang, 2017-03-29 04:14:22]

The CQ bit was checked by wanchang.ryu@lge.com

[commit-bot: I haz the power, 2017-03-29 04:15:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wanchang, 2017-03-29 04:18:33]

On 2017/03/29 03:29:33, kojii wrote:
> lgtm, it looks like there's a room to refactor, but better interop with Gecko
is
> good. Thank you for working on this.

Thanks kojii and eae for reviewing the patch.

[commit-bot: I haz the power, 2017-03-29 07:10:27]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490760862231340,
"parent_rev": "068669c24287ba5fbd0b541649ecc65fa08019ee", "commit_rev":
"19d5f8b3d7cd8ba33ba3f2dd331c339384b299dd"}

[commit-bot: I haz the power, 2017-03-29 07:11:15]

Description was changed from

==========
Fix incorrect bounding box position for '\n'

absoluteQuadsForRange function generates unrealted empty quads
with even not overlapping InlineTextBox.
This fix considers inclusive InlineTextbox, overlapping InlineTextbox,
and exclusive InlineTextbox cases separately.
Considering exclusive InlineTextbox is still required because non
rendering characters(e.g. whitespace) are missing and to cover if range
offset is only in this area.

BUG=698038
==========

to

==========
Fix incorrect bounding box position for '\n'

absoluteQuadsForRange function generates unrealted empty quads
with even not overlapping InlineTextBox.
This fix considers inclusive InlineTextbox, overlapping InlineTextbox,
and exclusive InlineTextbox cases separately.
Considering exclusive InlineTextbox is still required because non
rendering characters(e.g. whitespace) are missing and to cover if range
offset is only in this area.

BUG=698038

Review-Url: https://codereview.chromium.org/2763013002
Cr-Commit-Position: refs/heads/master@{#460310}
Committed:
https://chromium.googlesource.com/chromium/src/+/19d5f8b3d7cd8ba33ba3f2dd331c...
==========

[commit-bot: I haz the power, 2017-03-29 07:11:16]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/19d5f8b3d7cd8ba33ba3f2dd331c...

[Xiaocheng, 2017-03-31 18:02:18]

xiaochengh@chromium.org changed reviewers:
+ xiaochengh@chromium.org

[Xiaocheng, 2017-03-31 18:02:18]

This patch seems to have broken Range::textQuads.

Please see discussion in https://codereview.chromium.org/2775663008