Support language-appropriate position for "text-underline-position:under"

Support language-appropriate position for "text-underline-position:under"

This patch supports language-appropriate position[1], and matches Blink
behavior to Edge and Gecko.

[1] https://drafts.csswg.org/css-text-decor-3/#default-stylesheet

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

Review-Url: https://codereview.chromium.org/2647923002
Cr-Commit-Position: refs/heads/master@{#449174}
Committed: https://chromium.googlesource.com/chromium/src/+/7affa3729f4c422d9db4076e4806fddc49008560

[kojii, 2017-01-20 17:17:53]

Description was changed from

==========
ul-ja

BUG=
==========

to

==========
ul-ja

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

[kojii, 2017-01-20 18:07:13]

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

[commit-bot: I haz the power, 2017-01-20 18:07:40]

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

[kojii, 2017-01-20 19:06:26]

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

[commit-bot: I haz the power, 2017-01-20 19:06:45]

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

[kojii, 2017-01-20 21:56:33]

Description was changed from

==========
ul-ja

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

to

==========
Fix gap and language-appropriate position for text-underline-position:under

This patch fixes following issues in text-underline-position: under
1. Doesn't support language-appropriate position[1], different behavior
   from Edge/Gecko
2. Gap for "under" doesn't match to the spec nor to the use cases

This patch does not include following fixes, they will be in future
patches.
1. Performance implications (O(n^2) for under)
2. Automatic positions for CJK text due to the performance concern
3. Incorrect under position for fonts with a large internal leading
   such as Meiryo crbug.com/681857

[1] https://drafts.csswg.org/css-text-decor-3/#default-stylesheet

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

[kojii, 2017-01-20 21:56:49]

kojii@chromium.org changed reviewers:
+ drott@chromium.org

[kojii, 2017-01-20 21:56:50]

...and the split one.

[commit-bot: I haz the power, 2017-01-20 22:38:26]

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

[commit-bot: I haz the power, 2017-01-20 22:38:26]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-01-21 05:09:53]

Description was changed from

==========
Fix gap and language-appropriate position for text-underline-position:under

This patch fixes following issues in text-underline-position: under
1. Doesn't support language-appropriate position[1], different behavior
   from Edge/Gecko
2. Gap for "under" doesn't match to the spec nor to the use cases

This patch does not include following fixes, they will be in future
patches.
1. Performance implications (O(n^2) for under)
2. Automatic positions for CJK text due to the performance concern
3. Incorrect under position for fonts with a large internal leading
   such as Meiryo crbug.com/681857

[1] https://drafts.csswg.org/css-text-decor-3/#default-stylesheet

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

to

==========
Fix gap and language-appropriate position for "text-underline-position:under"

This patch fixes following issues in text-underline-position: under
1. Doesn't support language-appropriate position[1], different behavior
   from Edge/Gecko
2. Gap for "under" doesn't match to the spec nor to the use cases

This patch does not include following fixes, they will be in future
patches.
1. Performance implications (O(n^2) for under)
2. Automatic positions for CJK text due to the performance concern
3. Incorrect under position for fonts with a large internal leading
   such as Meiryo crbug.com/681857

[1] https://drafts.csswg.org/css-text-decor-3/#default-stylesheet

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

[kojii, 2017-01-21 05:14:12]

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

[commit-bot: I haz the power, 2017-01-21 05:14: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-01-21 06:46:55]

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

[commit-bot: I haz the power, 2017-01-21 06:46:56]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-01-21 10:44:39]

Description was changed from

==========
Fix gap and language-appropriate position for "text-underline-position:under"

This patch fixes following issues in text-underline-position: under
1. Doesn't support language-appropriate position[1], different behavior
   from Edge/Gecko
2. Gap for "under" doesn't match to the spec nor to the use cases

This patch does not include following fixes, they will be in future
patches.
1. Performance implications (O(n^2) for under)
2. Automatic positions for CJK text due to the performance concern
3. Incorrect under position for fonts with a large internal leading
   such as Meiryo crbug.com/681857

[1] https://drafts.csswg.org/css-text-decor-3/#default-stylesheet

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

to

==========
Fix language-appropriate position and gap for "text-underline-position:under"

This patch fixes following issues in text-underline-position: under
1. Doesn't support language-appropriate position[1], different behavior
   from Edge/Gecko
2. Gap for "under" doesn't match to the spec nor to the use cases

This patch does not include following fixes, they will be in future
patches.
1. Performance implications (O(n^2) for under)
2. Automatic positions for CJK text due to the performance concern
3. Incorrect under position for fonts with a large internal leading
   such as Meiryo crbug.com/681857

[1] https://drafts.csswg.org/css-text-decor-3/#default-stylesheet

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

[kojii, 2017-01-21 11:15:13]

Description was changed from

==========
Fix language-appropriate position and gap for "text-underline-position:under"

This patch fixes following issues in text-underline-position: under
1. Doesn't support language-appropriate position[1], different behavior
   from Edge/Gecko
2. Gap for "under" doesn't match to the spec nor to the use cases

This patch does not include following fixes, they will be in future
patches.
1. Performance implications (O(n^2) for under)
2. Automatic positions for CJK text due to the performance concern
3. Incorrect under position for fonts with a large internal leading
   such as Meiryo crbug.com/681857

[1] https://drafts.csswg.org/css-text-decor-3/#default-stylesheet

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

to

==========
Fix language-appropriate position for "text-underline-position:under"

This patch supports language-appropriate position[1], and matches Blink
behavior to Edge and Gecko.

[1] https://drafts.csswg.org/css-text-decor-3/#default-stylesheet

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

[kojii, 2017-01-21 11:23:29]

Description was changed from

==========
Fix language-appropriate position for "text-underline-position:under"

This patch supports language-appropriate position[1], and matches Blink
behavior to Edge and Gecko.

[1] https://drafts.csswg.org/css-text-decor-3/#default-stylesheet

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

to

==========
Support language-appropriate position for "text-underline-position:under"

This patch supports language-appropriate position[1], and matches Blink
behavior to Edge and Gecko.

[1] https://drafts.csswg.org/css-text-decor-3/#default-stylesheet

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

[kojii, 2017-01-21 11:23:54]

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

[commit-bot: I haz the power, 2017-01-21 11:23:59]

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-01-21 12:46:12]

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

[commit-bot: I haz the power, 2017-01-21 12:46:14]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-01-21 20:37:00]

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

[commit-bot: I haz the power, 2017-01-21 20:37:12]

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-01-22 00:16:13]

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

[commit-bot: I haz the power, 2017-01-22 00:16:14]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-01-22 15:46:05]

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

[commit-bot: I haz the power, 2017-01-22 15:46:13]

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

[kojii, 2017-01-22 15:56:59]

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

[commit-bot: I haz the power, 2017-01-22 15:57:03]

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

[kojii, 2017-01-22 16:07:53]

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

[commit-bot: I haz the power, 2017-01-22 16:07:58]

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

[kojii, 2017-01-22 16:08:03]

Patchset #8 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-01-22 18:17:06]

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

[commit-bot: I haz the power, 2017-01-22 18:17:07]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-01-23 13:43:32]

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

[commit-bot: I haz the power, 2017-01-23 13:43:43]

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-01-23 15:16:23]

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

[commit-bot: I haz the power, 2017-01-23 15:16:23]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-01-24 09:21:49]

PTAL this one too? Sorry for this many...

[kojii, 2017-01-27 18:38:53]

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

[commit-bot: I haz the power, 2017-01-27 18:39:33]

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-01-27 20:41:10]

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

[commit-bot: I haz the power, 2017-01-27 20:41:11]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kojii, 2017-02-07 06:56:30]

drott@, gentle reminder if you happen to have some spare time.

[drott, 2017-02-07 16:20:52]

Sorry for the delay, some comments below.

https://codereview.chromium.org/2647923002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2647923002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1026: crbug.com/677240
fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-cjk.html
[ NeedsRebaseline ]
Could you try to attach the other baselines directly to this CL using
Tools/scripts/webkit-patch rebaseline-cl, as explained in
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/A4HwldX-wFo ?

https://codereview.chromium.org/2647923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2647923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:68: if (script ==
USCRIPT_KATAKANA_OR_HIRAGANA || script == USCRIPT_HANGUL)
Are we sure this couldn't be USCRIPT_KATAKANA or USCRIPT_HIRAGANA, or does this
comparison take care of those?

https://codereview.chromium.org/2647923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:162: LayoutUnit
offset = computeUnderlineOffsetForUnder(style, inlineTextBox);
I don't fully understand why this works and why the
computerUnderlineOffsetForUnder function can help compute the offset for the
overline. Could you explain to which base coordinate the offset is applied and
why negating it works for computing the overline position?

https://codereview.chromium.org/2647923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:1188:
ResolvedUnderlinePosition underlinePosition =
Minor and optional: I find this flipDecoration XOR trick hard to read and a
surprising by-reference argument of this function. If you could find a more
readable way of expressing the idea here, that'd be good.

[kojii, 2017-02-07 23:13:44]

Thank you, I hope PS10 addressed your points and easier to read, replies inline.

Running rebaseline bots and will upload them when they're done.

https://codereview.chromium.org/2647923002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2647923002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1026: crbug.com/677240
fast/css3-text/css3-text-decoration/text-underline-position/text-underline-position-cjk.html
[ NeedsRebaseline ]
On 2017/02/07 at 16:20:52, drott wrote:
> Could you try to attach the other baselines directly to this CL using
Tools/scripts/webkit-patch rebaseline-cl, as explained in
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/A4HwldX-wFo ?

Ok, used before but didn't like very much for myself, but fine.

https://codereview.chromium.org/2647923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2647923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:68: if (script ==
USCRIPT_KATAKANA_OR_HIRAGANA || script == USCRIPT_HANGUL)
On 2017/02/07 at 16:20:52, drott wrote:
> Are we sure this couldn't be USCRIPT_KATAKANA or USCRIPT_HIRAGANA, or does
this comparison take care of those?

The former, we use this convention in several places since Japanese is
multi-script. Conversion here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/text/...
and an exmple here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/text/...

https://codereview.chromium.org/2647923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:162: LayoutUnit
offset = computeUnderlineOffsetForUnder(style, inlineTextBox);
On 2017/02/07 at 16:20:52, drott wrote:
> I don't fully understand why this works and why the
computerUnderlineOffsetForUnder function can help compute the offset for the
overline. Could you explain to which base coordinate the offset is applied and
why negating it works for computing the overline position?

Maybe function name is bad...computeUnderlineOffsetForUnder() computes position
across elements, and on the next line in |computeUnderlineOffset| adds
|logicalHeight| to move it down to the bottom.

I'll rename this in the next CL.

https://codereview.chromium.org/2647923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:1188:
ResolvedUnderlinePosition underlinePosition =
On 2017/02/07 at 16:20:52, drott wrote:
> Minor and optional: I find this flipDecoration XOR trick hard to read and a
surprising by-reference argument of this function. If you could find a more
readable way of expressing the idea here, that'd be good.

Ok, will add if() on every loop. I think it's rare to have multiple decorations,
so it shouldn't differ much.

[kojii, 2017-02-08 02:22:09]

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

[commit-bot: I haz the power, 2017-02-08 02:22:44]

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-02-08 03:30:31]

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

[commit-bot: I haz the power, 2017-02-08 03:30:32]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[kojii, 2017-02-08 04:40:05]

PTAL.

On 2017/02/07 at 23:13:44, kojii wrote:
> On 2017/02/07 at 16:20:52, drott wrote:
> > Could you try to attach the other baselines directly to this CL using
Tools/scripts/webkit-patch rebaseline-cl, as explained in
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/A4HwldX-wFo ?
> 
> Ok, used before but didn't like very much for myself, but fine.

There were more rebaselines, which I mistakenly put into the previous patch when
I split. PS12 includes all of them.

Linux/max failures look unrelated, I'll re-run bots later.

Win failure seems the failure of the rebaseline script. I'll re-include
TestExpectations in PS13.

[kojii, 2017-02-08 06:40:17]

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

[commit-bot: I haz the power, 2017-02-08 06:40:40]

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-02-08 08:06:26]

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

[commit-bot: I haz the power, 2017-02-08 08:06:27]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[drott, 2017-02-08 15:13:18]

On 2017/02/08 at 04:40:05, kojii wrote:
> PTAL.
> 
> On 2017/02/07 at 23:13:44, kojii wrote:
> > On 2017/02/07 at 16:20:52, drott wrote:
> > > Could you try to attach the other baselines directly to this CL using
Tools/scripts/webkit-patch rebaseline-cl, as explained in
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/A4HwldX-wFo ?
> > 
> > Ok, used before but didn't like very much for myself, but fine.
> 
> There were more rebaselines, which I mistakenly put into the previous patch
when I split. PS12 includes all of them.
> 
> Linux/max failures look unrelated, I'll re-run bots later.
> 
> Win failure seems the failure of the rebaseline script. I'll re-include
TestExpectations in PS13.

Mhm, okay, you can file bugs on the tool and assign it to qyearsley@ to take a
look. Do you know what failed in this case? Talking to Walter and Quinten, I
believe the plan is to try and move to this tool only so that code change and
rebaselines are always generally in the same commit and we don't have phases of
losing test coverage temporarily. 

LGTM, thanks for updating the CL.

[kojii, 2017-02-08 23:27:58]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2017-02-08 23:28:37]

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

[kojii, 2017-02-08 23:29:19]

The CQ bit was unchecked by kojii@chromium.org

[kojii, 2017-02-08 23:32:02]

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

[kojii, 2017-02-08 23:32:03]

eae@, PTAL. Forgot this needs paint owner sign off.

drott@:
> you can file bugs on the tool and assign it to qyearsley@ to take a look
will do. I wasn't sure how serious we're on the manual rebaseline tool.

[eae, 2017-02-09 01:05:43]

LGTM for core/paint

[kojii, 2017-02-09 01:08:49]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2017-02-09 01:09:35]

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-02-09 01:16:10]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1486602529328140,
"parent_rev": "0e09de60e479fe2ee105fb731b403c729fce3ac1", "commit_rev":
"7affa3729f4c422d9db4076e4806fddc49008560"}

[commit-bot: I haz the power, 2017-02-09 01:17:07]

Description was changed from

==========
Support language-appropriate position for "text-underline-position:under"

This patch supports language-appropriate position[1], and matches Blink
behavior to Edge and Gecko.

[1] https://drafts.csswg.org/css-text-decor-3/#default-stylesheet

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

to

==========
Support language-appropriate position for "text-underline-position:under"

This patch supports language-appropriate position[1], and matches Blink
behavior to Edge and Gecko.

[1] https://drafts.csswg.org/css-text-decor-3/#default-stylesheet

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

Review-Url: https://codereview.chromium.org/2647923002
Cr-Commit-Position: refs/heads/master@{#449174}
Committed:
https://chromium.googlesource.com/chromium/src/+/7affa3729f4c422d9db4076e4806...
==========

[commit-bot: I haz the power, 2017-02-09 01:17:10]

Committed patchset #13 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/7affa3729f4c422d9db4076e4806...

[kojii, 2017-02-09 12:33:22]

The rebaseline bot didn't change any, so the test failure could be the bot was
flaky, probably due to different font configurations? Not very clear at this
moment.