Show selection highlight even if we have a composition underline

Show selection highlight even if we have a composition underline

We currently have some logic (dating back to 2005) to not show the selection
highlight if we're also showing a composition underline. We're not 100% sure
why this (probably to avoid some sort of interaction between the selection
color and the underline), but we think it's unnecessary. There are already
certain interactions we don't do anything about, for example the composition
underline matching the background color of the text.

This CL removes this logic so we can show the selection highlight while the
Android Voice IME (which keeps an underlined composition range open) is
active.

BUG=628670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2896273003
Cr-Commit-Position: refs/heads/master@{#476178}
Committed: https://chromium.googlesource.com/chromium/src/+/1053faaf3d0458556000acde907116a5d869c815

[rlanday, 2017-05-23 20:03:48]

Description was changed from

==========
Show selection highlight if we have a generic composition underline

We currently have some logic to not show the selection highlight if we're also
showing a composition underline. I'm not 100% sure why this is (the logic has
been around for over a decade and I couldn't find where it was originally added)
but I think it's because the IME could be adding a background color with the
underline, in which case we don't want to blend the background color and the
selection highlight color.

This CL modifies the logic so we draw the selection highlight if none of the
composition underline markers has a background color associated with it (i.e.
the background color is specified as transparent).

BUG=628670
==========

to

==========
Show selection highlight if we have a generic composition underline

We currently have some logic to not show the selection highlight if we're also
showing a composition underline. I'm not 100% sure why this is (the logic has
been around for over a decade and I couldn't find where it was originally added)
but I think it's because the IME could be adding a background color with the
underline, in which case we don't want to blend the background color and the
selection highlight color.

This CL modifies the logic so we draw the selection highlight if none of the
composition underline markers has a background color associated with it (i.e.
the background color is specified as transparent).

BUG=628670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[rlanday, 2017-05-23 20:04:18]

The CQ bit was checked by rlanday@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 20:05:27]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-23 22:00:34]

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

[commit-bot: I haz the power, 2017-05-23 22:00:35]

Dry run: This issue passed the CQ dry run.

[rlanday, 2017-05-23 22:37:41]

rlanday@chromium.org changed reviewers:
+ senorblanco@chromium.org

[rlanday, 2017-05-23 22:59:17]

Description was changed from

==========
Show selection highlight if we have a generic composition underline

We currently have some logic to not show the selection highlight if we're also
showing a composition underline. I'm not 100% sure why this is (the logic has
been around for over a decade and I couldn't find where it was originally added)
but I think it's because the IME could be adding a background color with the
underline, in which case we don't want to blend the background color and the
selection highlight color.

This CL modifies the logic so we draw the selection highlight if none of the
composition underline markers has a background color associated with it (i.e.
the background color is specified as transparent).

BUG=628670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Show selection highlight even if we have a composition underline

We currently have some logic (dating back to 2005) to not show the selection
highlight if we're also showing a composition underline. We're not 100% sure
why this (probably to avoid some sort of interaction between the selection
color and the underline), but we think it's unnecessary. There are already
certain interactions we don't do anything about, for example the composition
underline matching the background color of the text.

This CL removes this logic so we can show the selection highlight while the
Android Voice IME (which keeps an underlined composition range open) is
active.

BUG=628670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[rlanday, 2017-05-23 22:59:55]

Per discussion on the bug, I'm changing this to show the selection highlight
regardless of whether or not the composition underline has a background color.

[Stephen White, 2017-05-24 20:36:23]

Description was changed from

==========
Show selection highlight even if we have a composition underline

We currently have some logic (dating back to 2005) to not show the selection
highlight if we're also showing a composition underline. We're not 100% sure
why this (probably to avoid some sort of interaction between the selection
color and the underline), but we think it's unnecessary. There are already
certain interactions we don't do anything about, for example the composition
underline matching the background color of the text.

This CL removes this logic so we can show the selection highlight while the
Android Voice IME (which keeps an underlined composition range open) is
active.

BUG=628670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Show selection highlight even if we have a composition underline

We currently have some logic (dating back to 2005) to not show the selection
highlight if we're also showing a composition underline. We're not 100% sure
why this (probably to avoid some sort of interaction between the selection
color and the underline), but we think it's unnecessary. There are already
certain interactions we don't do anything about, for example the composition
underline matching the background color of the text.

This CL removes this logic so we can show the selection highlight while the
Android Voice IME (which keeps an underlined composition range open) is
active.

BUG=628670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Stephen White, 2017-05-24 20:36:23]

senorblanco@chromium.org changed reviewers:
+ chrishtr@chromium.org
- senorblanco@chromium.org

[Stephen White, 2017-05-24 20:36:41]

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

[Stephen White, 2017-05-24 20:36:42]

Probably better that someone on the Paint team review this.

[chrishtr, 2017-05-30 18:21:12]

Change looks ok. Could you please add a test?

[rlanday, 2017-05-30 20:46:00]

On 2017/05/30 at 18:21:12, chrishtr wrote:
> Change looks ok. Could you please add a test?

I'm trying to determine how best to do that and I'm not sure how...do I need to
get the result of the paint operation as a DrawingDisplayItem (as in
SVGInlineTextBoxPainterTest.cpp) and examine it? If so, how do I determine the
selection color we actually painted from that?

[chrishtr, 2017-05-30 21:54:30]

On 2017/05/30 at 20:46:00, rlanday wrote:
> On 2017/05/30 at 18:21:12, chrishtr wrote:
> > Change looks ok. Could you please add a test?
> 
> I'm trying to determine how best to do that and I'm not sure how...do I need
to get the result of the paint operation as a DrawingDisplayItem (as in
SVGInlineTextBoxPainterTest.cpp) and examine it? If so, how do I determine the
selection color we actually painted from that?

I think you can make a layout test that sets composition markers and selection
at the same time.
These examples may help:

https://cs.chromium.org/search/?q=f:layouttests+selection+composition&type=cs

[rlanday, 2017-05-31 23:33:56]

Does this test look OK?

[chrishtr, 2017-06-01 00:47:07]

Test looks fine. I assume it fails without your patch?

https://codereview.chromium.org/2896273003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/selection/text-selection-with-composition.html
(right):

https://codereview.chromium.org/2896273003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/selection/text-selection-with-composition.html:1:
<!DOCTYPE html>
You can use testharnness to avoid the -expected.txt files

<script src="../../resources/testharness.js"></script>
<script src="../../resources/testharnessreport.js"></script>

<script>
test(runTest);
</script>

[rlanday, 2017-06-01 01:12:54]

Yes, the test fails without my patch. But when I use testharness.js, it seems to
ignore the .png files (and therefore pass), which I think I need, since I don't
know of any way to check in JavaScript that the selection highlight is actually
being painted. Is this expected?

[chrishtr, 2017-06-01 01:14:09]

On 2017/06/01 at 01:12:54, rlanday wrote:
> Yes, the test fails without my patch. But when I use testharness.js, it seems
to ignore the .png files (and therefore pass), which I think I need, since I
don't know of any way to check in JavaScript that the selection highlight is
actually being painted. Is this expected?

Ah yes, sorry. Ignore the part about testharness.

[chrishtr, 2017-06-01 01:14:17]

lgtm

[rlanday, 2017-06-01 01:15:09]

The CQ bit was checked by rlanday@chromium.org

[commit-bot: I haz the power, 2017-06-01 01:15:50]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-01 03:50:56]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496279709169110,
"parent_rev": "2553f2817c5d3bedc8ac060789595b74b4443369", "commit_rev":
"1053faaf3d0458556000acde907116a5d869c815"}

[commit-bot: I haz the power, 2017-06-01 03:51:03]

Description was changed from

==========
Show selection highlight even if we have a composition underline

We currently have some logic (dating back to 2005) to not show the selection
highlight if we're also showing a composition underline. We're not 100% sure
why this (probably to avoid some sort of interaction between the selection
color and the underline), but we think it's unnecessary. There are already
certain interactions we don't do anything about, for example the composition
underline matching the background color of the text.

This CL removes this logic so we can show the selection highlight while the
Android Voice IME (which keeps an underlined composition range open) is
active.

BUG=628670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Show selection highlight even if we have a composition underline

We currently have some logic (dating back to 2005) to not show the selection
highlight if we're also showing a composition underline. We're not 100% sure
why this (probably to avoid some sort of interaction between the selection
color and the underline), but we think it's unnecessary. There are already
certain interactions we don't do anything about, for example the composition
underline matching the background color of the text.

This CL removes this logic so we can show the selection highlight while the
Android Voice IME (which keeps an underlined composition range open) is
active.

BUG=628670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2896273003
Cr-Commit-Position: refs/heads/master@{#476178}
Committed:
https://chromium.googlesource.com/chromium/src/+/1053faaf3d0458556000acde9071...
==========

[commit-bot: I haz the power, 2017-06-01 03:51:05]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1053faaf3d0458556000acde9071...