Use the font height as caret height instead of the line height

Use the font height as caret height instead of the line height

The caret height should be calculated by using the FontMetrics
instead of the selection height.

BUG=588060
TEST=editing/caret/caret-height-multi-line.html

Review-Url: https://codereview.chromium.org/2517383002
Cr-Commit-Position: refs/heads/master@{#442830}
Committed: https://chromium.googlesource.com/chromium/src/+/c5291a0115a65797dbf7992c8aa47136db95ae6a

[joone, 2016-11-22 01:38:13]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 01:39:07]

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, 2016-11-22 01:42:20]

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

[commit-bot: I haz the power, 2016-11-22 01:42:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[joone, 2016-11-22 01:49:35]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 01:50:19]

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, 2016-11-22 02:00:59]

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

[commit-bot: I haz the power, 2016-11-22 02:01:00]

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

[joone, 2016-11-22 03:01:13]

joone.hur@intel.com changed reviewers:
+ yosin@chromium.org

[joone, 2016-11-22 03:01:14]

yosin@ hi, could you review this CL?

[yosin_UTC9, 2016-11-22 03:52:41]

Caret rectangle should be calculated from font-size of inserted character
instead of line height:
 http://crbug.com/588060

This means caret rectangle is located at base line and use font ascendant and
font descendant.

You can obtain "font" CSS property for an inserted character from
FrameSelection::typingStyle(), I think we should move this to |Editor|, BTW, and
default is "font" property of an element containing caret.

To do this, we need to change functions which calculate caret rectangle. I think
we should implement calculating caret rect by using Line Layout API rather than
member functions of
LayoutObject. 

kojii@ and I work line layout API, so we can work together to identify missing
API functions for calculating care rect.

It seems it isn't so simple because of handling ligature. But I think it is OK
to postpone ligature support later.

https://codereview.chromium.org/2517383002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2517383002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:1725: crbug.com/581004
editing/deleting/delete-line-003.html [ NeedsRebaseline ]
It seems once we convert them to use assert_selection(), we don't need to rebase
them.
To make layout test faster could you convert them to use assert_selection()?

[joone, 2016-11-22 22:46:05]

https://codereview.chromium.org/2517383002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2517383002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:1725: crbug.com/581004
editing/deleting/delete-line-003.html [ NeedsRebaseline ]
On 2016/11/22 03:52:41, Yosi_UTC9 wrote:
> It seems once we convert them to use assert_selection(), we don't need to
rebase
> them.
> To make layout test faster could you convert them to use assert_selection()?

Here is a CL for this: https://codereview.chromium.org/2520173004/

[joone, 2016-11-22 23:32:26]

On 2016/11/22 03:52:41, Yosi_UTC9 wrote:
> Caret rectangle should be calculated from font-size of inserted character
> instead of line height:
>  http://crbug.com/588060
> 
> This means caret rectangle is located at base line and use font ascendant and
> font descendant.
> 
> You can obtain "font" CSS property for an inserted character from
> FrameSelection::typingStyle(), I think we should move this to |Editor|, BTW,
and
> default is "font" property of an element containing caret.
> 
> To do this, we need to change functions which calculate caret rectangle. I
think
> we should implement calculating caret rect by using Line Layout API rather
than
> member functions of
> LayoutObject. 
> 

How can I change this CL? 
For temporary, implement some code to get caret rect from |Editor| by using
FrameSelection::typingStyle() instead of accessing LayoutText?

[yosin_UTC9, 2016-11-24 03:54:42]

On 2016/11/22 at 23:32:26, joone.hur wrote:
> On 2016/11/22 03:52:41, Yosi_UTC9 wrote:
> > Caret rectangle should be calculated from font-size of inserted character
> > instead of line height:
> >  http://crbug.com/588060
> > 
> > This means caret rectangle is located at base line and use font ascendant
and
> > font descendant.
> > 
> > You can obtain "font" CSS property for an inserted character from
> > FrameSelection::typingStyle(), I think we should move this to |Editor|, BTW,
and
> > default is "font" property of an element containing caret.
> > 
> > To do this, we need to change functions which calculate caret rectangle. I
think
> > we should implement calculating caret rect by using Line Layout API rather
than
> > member functions of
> > LayoutObject. 
> > 
> 
> How can I change this CL? 
> For temporary, implement some code to get caret rect from |Editor| by using
FrameSelection::typingStyle() instead of accessing LayoutText?

At this time, let's forget about |FrameSelection::typingStyle()|. I don't have
clear idea about it.
For temporary, just return caret rect based the font associated LayoutText's
style == get point of baseline and use ascent and descent of font with
considering bidi == place caret at left side for RTL.

For the future, we need to provide information like DWRITE_TEXT_METRICS1,
https://msdn.microsoft.com/en-us/library/windows/desktop/dn280422(v=vs.85).aspx
See also IDWriteTextLayout::HitTestTextPosition(),
https://msdn.microsoft.com/en-us/library/windows/desktop/dd371469(v=vs.85).aspx

[joone, 2016-11-29 02:15:44]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 02:16:27]

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

[joone, 2016-11-29 02:54:34]

Description was changed from

==========
Set the same caret height for each text line

The caret height should be calculated by using the line top value
instead of the selection top.

BUG=581004
TEST=editing/caret/caret-height-multi-line.html
==========

to

==========
Use the font height as caret height instead of the line height

The caret height should be calculated by using the FontMetrics
instead of the selection height.

BUG=588060
TEST=editing/caret/caret-height-multi-line.html
==========

[commit-bot: I haz the power, 2016-11-29 03:52:10]

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

[commit-bot: I haz the power, 2016-11-29 03:52:11]

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

[joone, 2016-11-29 04:28:20]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 04:28:31]

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, 2016-11-29 07:18:07]

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

[commit-bot: I haz the power, 2016-11-29 07:18:08]

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

[joone, 2016-11-29 07:36:34]

On 2016/11/24 03:54:42, Yosi_UTC9 wrote:
> On 2016/11/22 at 23:32:26, joone.hur wrote:
> > On 2016/11/22 03:52:41, Yosi_UTC9 wrote:
> > > Caret rectangle should be calculated from font-size of inserted character
> > > instead of line height:
> > >  http://crbug.com/588060
> > > 
> > > This means caret rectangle is located at base line and use font ascendant
> and
> > > font descendant.
> > > 
> > > You can obtain "font" CSS property for an inserted character from
> > > FrameSelection::typingStyle(), I think we should move this to |Editor|,
BTW,
> and
> > > default is "font" property of an element containing caret.
> > > 
> > > To do this, we need to change functions which calculate caret rectangle. I
> think
> > > we should implement calculating caret rect by using Line Layout API rather
> than
> > > member functions of
> > > LayoutObject. 
> > > 
> > 
> > How can I change this CL? 
> > For temporary, implement some code to get caret rect from |Editor| by using
> FrameSelection::typingStyle() instead of accessing LayoutText?
> 
> At this time, let's forget about |FrameSelection::typingStyle()|. I don't have
> clear idea about it.
> For temporary, just return caret rect based the font associated LayoutText's
> style == get point of baseline and use ascent and descent of font with
> considering bidi == place caret at left side for RTL.
> 
> For the future, we need to provide information like DWRITE_TEXT_METRICS1,
>
https://msdn.microsoft.com/en-us/library/windows/desktop/dn280422(v=vs.85).aspx
> See also IDWriteTextLayout::HitTestTextPosition(),
>
https://msdn.microsoft.com/en-us/library/windows/desktop/dd371469(v=vs.85).aspx

yosin@ I've updated the CL according to your suggestion.

[joone, 2016-11-29 21:41:42]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 21:42:10]

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, 2016-11-29 21:46:02]

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

[commit-bot: I haz the power, 2016-11-29 21:46:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[joone, 2016-11-29 21:55:10]

Patchset #5 (id:80001) has been deleted

[joone, 2016-11-29 21:55:33]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 21:56:03]

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, 2016-11-29 23:58:21]

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

[commit-bot: I haz the power, 2016-11-29 23:58:21]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[yosin_UTC9, 2016-11-30 01:44:01]

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

[yosin_UTC9, 2016-11-30 01:44:02]

+drott@, since he is an expert of InlineTextBox and font.

Could you also change localCaretRectForEmptyElement() too?

https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutText.cpp:736: int height =
style()->font().primaryFont()->getFontMetrics().height();
Could you use font ascendant and descendant instead of height?

top = baseline - font_ascendant;
bottom = baseline + font_descendant;

BTW, I'm not sure we should use actual font instead of computed font.
The font used by |inlineBox| can be different of |style()->font()| due by
missing glyph, e.g. kanji character with "font-family: Arial".

I found following code fragment in InlineTextBoxPainter:

  const ComputedStyle& styleToUse =
      m_inlineTextBox.getLineLayoutItem().styleRef(
          m_inlineTextBox.isFirstLineStyle());

Note: ::first-letter characters and rest of characters are placed into different
InlineTextBox.

I'm not an expert of this area, drott@ will provide more information.

[joone, 2016-12-01 02:06:48]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 02:07:03]

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, 2016-12-01 02:09:41]

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

[commit-bot: I haz the power, 2016-12-01 02:09:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[joone, 2016-12-02 00:09:10]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 00:09:40]

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

[joone, 2016-12-02 00:20:59]

Patchset #6 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-12-02 01:23:34]

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

[commit-bot: I haz the power, 2016-12-02 01:23:35]

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

[joone, 2016-12-02 01:24:43]

https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutText.cpp:736: int height =
style()->font().primaryFont()->getFontMetrics().height();
On 2016/11/30 01:44:02, Yosi_UTC9 wrote:
> Could you use font ascendant and descendant instead of height?
> 
> top = baseline - font_ascendant;
> bottom = baseline + font_descendant;

But, we need the height value and can get it easily by just calling the height
method of FontMetrics: the height value is calculated by ascent() + descent().

> BTW, I'm not sure we should use actual font instead of computed font.
> The font used by |inlineBox| can be different of |style()->font()| due by
> missing glyph, e.g. kanji character with "font-family: Arial".
> 
> I found following code fragment in InlineTextBoxPainter:
> 
>   const ComputedStyle& styleToUse =
>       m_inlineTextBox.getLineLayoutItem().styleRef(
>           m_inlineTextBox.isFirstLineStyle());
> 

I used the above code to get the computed font.

[yosin_UTC9, 2016-12-02 01:42:35]

https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutText.cpp:736: int height =
style()->font().primaryFont()->getFontMetrics().height();
On 2016/12/02 at 01:24:43, joone wrote:
> On 2016/11/30 01:44:02, Yosi_UTC9 wrote:
> > Could you use font ascendant and descendant instead of height?
> > 
> > top = baseline - font_ascendant;
> > bottom = baseline + font_descendant;
> 
> But, we need the height value and can get it easily by just calling the height
method of FontMetrics: the height value is calculated by ascent() + descent().
> 
This function returns rectangle rather than height. Caret rectangle should be:

line top

  +
  |
  |
  |
 -+- baseline
  |

line bottom


This patch returns:


line top

  +
  |
  |
  |
  |
  +- line bottom

[yosin_UTC9, 2016-12-02 06:27:20]

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

[joone, 2016-12-02 21:32:58]

On 2016/12/02 01:42:35, Yosi_UTC9 wrote:
>
https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):
> 
>
https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutText.cpp:736: int height =
> style()->font().primaryFont()->getFontMetrics().height();
> On 2016/12/02 at 01:24:43, joone wrote:
> > On 2016/11/30 01:44:02, Yosi_UTC9 wrote:
> > > Could you use font ascendant and descendant instead of height?
> > > 
> > > top = baseline - font_ascendant;
> > > bottom = baseline + font_descendant;
> > 
> > But, we need the height value and can get it easily by just calling the
height
> method of FontMetrics: the height value is calculated by ascent() + descent().
> > 
> This function returns rectangle rather than height. Caret rectangle should be:
> 
> line top
> 
>   +
>   |
>   |
>   |
>  -+- baseline
>   |
> 
> line bottom
> 
> 
> This patch returns:
> 
> 
> line top
> 
>   +
>   |
>   |
>   |
>   |
>   +- line bottom

If we use the length of from the line top to the baseline as the height of
caret, it's too short to cover the descent.

[yosin_UTC9, 2016-12-07 05:34:28]

On 2016/12/02 at 21:32:58, joone.hur wrote:
> On 2016/12/02 01:42:35, Yosi_UTC9 wrote:
> >
https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):
> > 
> >
https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/layout/LayoutText.cpp:736: int height =
> > style()->font().primaryFont()->getFontMetrics().height();
> > On 2016/12/02 at 01:24:43, joone wrote:
> > > On 2016/11/30 01:44:02, Yosi_UTC9 wrote:
> > > > Could you use font ascendant and descendant instead of height?
> > > > 
> > > > top = baseline - font_ascendant;
> > > > bottom = baseline + font_descendant;
> > > 
> > > But, we need the height value and can get it easily by just calling the
height
> > method of FontMetrics: the height value is calculated by ascent() +
descent().
> > > 
> > This function returns rectangle rather than height. Caret rectangle should
be:
> > 
> > line top
> > 
> >   +
> >   |
> >   |
> >   |
> >  -+- baseline
> >   |
> > 
> > line bottom
> > 
> > 
> > This patch returns:
> > 
> > 
> > line top
> > 
> >   +
> >   |
> >   |
> >   |
> >   |
> >   +- line bottom
> 
> If we use the length of from the line top to the baseline as the height of
caret, it's too short to cover the descent.

No, we use font ascent and font descent relative to base line. In this way, font
is covered by caret.
See
https://developer.apple.com/library/content/documentation/TextFonts/Conceptua...

Default line height is ascent + descent + line gap, and it is distance between
current baseline to next baseline.
See
https://msdn.microsoft.com/en-us/library/windows/desktop/dd368074(v=vs.85).aspx

Note: baseline is calculated from fonts appeared in line.

[joone, 2016-12-07 08:02:50]

On 2016/12/07 05:34:28, Yosi_UTC9 wrote:
> On 2016/12/02 at 21:32:58, joone.hur wrote:
> > On 2016/12/02 01:42:35, Yosi_UTC9 wrote:
> > >
>
https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):
> > > 
> > >
>
https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/core/layout/LayoutText.cpp:736: int height =
> > > style()->font().primaryFont()->getFontMetrics().height();
> > > On 2016/12/02 at 01:24:43, joone wrote:
> > > > On 2016/11/30 01:44:02, Yosi_UTC9 wrote:
> > > > > Could you use font ascendant and descendant instead of height?
> > > > > 
> > > > > top = baseline - font_ascendant;
> > > > > bottom = baseline + font_descendant;
> > > > 
> > > > But, we need the height value and can get it easily by just calling the
> height
> > > method of FontMetrics: the height value is calculated by ascent() +
> descent().
> > > > 
> > > This function returns rectangle rather than height. Caret rectangle should
> be:
> > > 
> > > line top
> > > 
> > >   +
> > >   |
> > >   |
> > >   |
> > >  -+- baseline
> > >   |
> > > 
> > > line bottom
> > > 
> > > 
> > > This patch returns:
> > > 
> > > 
> > > line top
> > > 
> > >   +
> > >   |
> > >   |
> > >   |
> > >   |
> > >   +- line bottom
> > 
> > If we use the length of from the line top to the baseline as the height of
> caret, it's too short to cover the descent.
> 
> No, we use font ascent and font descent relative to base line. In this way,
font
> is covered by caret.
> See
>
https://developer.apple.com/library/content/documentation/TextFonts/Conceptua...
> 
> Default line height is ascent + descent + line gap, and it is distance between
> current baseline to next baseline.
> See
>
https://msdn.microsoft.com/en-us/library/windows/desktop/dd368074(v=vs.85).aspx
> 
> Note: baseline is calculated from fonts appeared in line.

Do you mean that the height of caret = font ascent + font descent?

[yosin_UTC9, 2016-12-07 09:01:58]

On 2016/12/07 at 08:02:50, joone.hur wrote:
> On 2016/12/07 05:34:28, Yosi_UTC9 wrote:
> > On 2016/12/02 at 21:32:58, joone.hur wrote:
> > > On 2016/12/02 01:42:35, Yosi_UTC9 wrote:
> > > >
> >
https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/2517383002/diff/100001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/core/layout/LayoutText.cpp:736: int height =
> > > > style()->font().primaryFont()->getFontMetrics().height();
> > > > On 2016/12/02 at 01:24:43, joone wrote:
> > > > > On 2016/11/30 01:44:02, Yosi_UTC9 wrote:
> > > > > > Could you use font ascendant and descendant instead of height?
> > > > > > 
> > > > > > top = baseline - font_ascendant;
> > > > > > bottom = baseline + font_descendant;
> > > > > 
> > > > > But, we need the height value and can get it easily by just calling
the
> > height
> > > > method of FontMetrics: the height value is calculated by ascent() +
> > descent().
> > > > > 
> > > > This function returns rectangle rather than height. Caret rectangle
should
> > be:
> > > > 
> > > > line top
> > > > 
> > > >   +
> > > >   |
> > > >   |
> > > >   |
> > > >  -+- baseline
> > > >   |
> > > > 
> > > > line bottom
> > > > 
> > > > 
> > > > This patch returns:
> > > > 
> > > > 
> > > > line top
> > > > 
> > > >   +
> > > >   |
> > > >   |
> > > >   |
> > > >   |
> > > >   +- line bottom
> > > 
> > > If we use the length of from the line top to the baseline as the height of
> > caret, it's too short to cover the descent.
> > 
> > No, we use font ascent and font descent relative to base line. In this way,
font
> > is covered by caret.
> > See
> >
https://developer.apple.com/library/content/documentation/TextFonts/Conceptua...
> > 
> > Default line height is ascent + descent + line gap, and it is distance
between
> > current baseline to next baseline.
> > See
> >
https://msdn.microsoft.com/en-us/library/windows/desktop/dd368074(v=vs.85).aspx
> > 
> > Note: baseline is calculated from fonts appeared in line.
> 
> Do you mean that the height of caret = font ascent + font descent?

Yes. font height = ascent + descent

[drott, 2016-12-07 11:30:50]

Can we define/discuss/describe the desired behavior including font fallback
first, perhaps in the bug description of the meta bug? I believe the
InlineTextBox's line height takes into account several fonts and accumulates
their metrics, originating from font fallback or styling combined in the line.
One run of text with a style of 'font-family: Arial, Segoe UI Emoji, Kochi
Mincho;" for example can have a couple characters represented in Arial, some
"tall" emoji in Segoe UI emoji, and some Japanese Characters in Kochi Mincho,
plus - as an example - some Arabic characters in a fallback font. What is the
desired caret height while you move the caret through this run of text? Should
the caret height be the SimpleFontData->getFontMetrics() height at each glyph
position? What about the caret positions between two different fonts? Should the
caret height be the maximum of the fonts used in the run of text or the whole
InlineTextBox?

[joone, 2016-12-07 23:24:32]

On 2016/12/07 09:01:58, Yosi_UTC9 wrote:
> On 2016/12/07 at 08:02:50, joone.hur wrote:
> > On 2016/12/07 05:34:28, Yosi_UTC9 wrote:
> > > On 2016/12/02 at 21:32:58, joone.hur wrote:
> > > > If we use the length of from the line top to the baseline as the height
of
> > > caret, it's too short to cover the descent.
> > > 
> > > No, we use font ascent and font descent relative to base line. In this
way,
> font
> > > is covered by caret.
> > > See
> > >
>
https://developer.apple.com/library/content/documentation/TextFonts/Conceptua...
> > > 
> > > Default line height is ascent + descent + line gap, and it is distance
> between
> > > current baseline to next baseline.
> > > See
> > >
>
https://msdn.microsoft.com/en-us/library/windows/desktop/dd368074(v=vs.85).aspx
> > > 
> > > Note: baseline is calculated from fonts appeared in line.
> > 
> > Do you mean that the height of caret = font ascent + font descent?
> 
> Yes. font height = ascent + descent

If so, we can call FontMetrics::height() to get the height of caret. 

  int height(FontBaseline baselineType = AlphabeticBaseline) const {
    return ascent() + descent();
  }

It was already used in my CL.

[joone, 2016-12-07 23:29:03]

On 2016/12/07 11:30:50, drott wrote:
> Can we define/discuss/describe the desired behavior including font fallback
> first, perhaps in the bug description of the meta bug? I believe the
> InlineTextBox's line height takes into account several fonts and accumulates
> their metrics, originating from font fallback or styling combined in the line.
> One run of text with a style of 'font-family: Arial, Segoe UI Emoji, Kochi
> Mincho;" for example can have a couple characters represented in Arial, some
> "tall" emoji in Segoe UI emoji, and some Japanese Characters in Kochi Mincho,
> plus - as an example - some Arabic characters in a fallback font. What is the
> desired caret height while you move the caret through this run of text? Should
> the caret height be the SimpleFontData->getFontMetrics() height at each glyph
> position? What about the caret positions between two different fonts? Should
the
> caret height be the maximum of the fonts used in the run of text or the whole
> InlineTextBox?

I will add more test cases for Asian fonts.
Regarding the caret height, the caret height should be the
SimpleFontData->getFontMetrics() height at each glyph
position.

[yosin_UTC9, 2016-12-08 04:59:19]

On 2016/12/07 at 11:30:50, drott wrote:
> Can we define/discuss/describe the desired behavior including font fallback
first, perhaps in the bug description of the meta bug? I believe the
InlineTextBox's line height takes into account several fonts and accumulates
their metrics, originating from font fallback or styling combined in the line.
One run of text with a style of 'font-family: Arial, Segoe UI Emoji, Kochi
Mincho;" for example can have a couple characters represented in Arial, some
"tall" emoji in Segoe UI emoji, and some Japanese Characters in Kochi Mincho,
plus - as an example - some Arabic characters in a fallback font. What is the
desired caret height while you move the caret through this run of text? Should
the caret height be the SimpleFontData->getFontMetrics() height at each glyph
position? What about the caret positions between two different fonts? Should the
caret height be the maximum of the fonts used in the run of text or the whole
InlineTextBox?

Thanks for deep consideration.

Caret height should be obtained from a font of character *before* caret, e.g.
for "ab|cd", caret height is a font of "b" where "|" represent caret, in this
example |caretOffset| is 2 instead of 1, rather than a font of character after
caret.

Note: "character at caret" means "character after caret" in editing, see
char-after in Emacs[1]

You can see this behavior in Google Docs, try to type "abc" and make size of "a"
and "c" to 11 and "b" to 30 then place caret before "b" and "c" to see height of
caret. This is because HTML is something like: 
 - Caret before "b": <font size="11">a|</font><font size="30">b</font><font
size="11">c</font> 
 - Caret before "c": <font size="11">a</font><font size="30">b|</font><font
size="11">c</font>

We can think as font-fallback as same as above, and assume user may type same
family of a character before caret, e.g. type a Latin character after Latin
character, e.g. we assume user may type "d" on "abc|漢字ef" rather than
Kanji-character.

For |caretOffset| = 0, no character before caret, we should use a first
available font in user specified style for enclosing element, e.g. we use
"font-B" for |<span style="font-family: font-A, font-B, font-C"></span>| if
"font-A" is unavailable.


[1] https://www.gnu.org/software/emacs/manual/html_node/elisp/Near-Point.html

[yosin_UTC9, 2016-12-08 05:00:00]

I've also updated issue 588060.

[drott, 2016-12-08 08:21:41]

Thanks for the details explaining the intended behavior and implications, that
helped.

> Regarding the caret height, the caret height should be the
SimpleFontData->getFontMetrics() height at each glyph
position.

Joone, what's the status of this CL now and which case or cases is it intending
to fix? We seem to agree that the caret height should be
SimpleFontData->getFontMetrics() height at each glyph position. Does this CL aim
for solving that, or is it only fixing font at position 0, where - as yosin@
explains - it would be the first available font, i.e.
Font->primaryFont()->getFontMetrics().height()

How about a case like a Latin primary font, e.g. Arial, and then typing tall
Burmese characters, like Myanmar Letter O ဩ, which span and exceed the usual
Latin ascenders - is the caret height adjusting for this situation and is the
intention to cover this in this CL or in separate ones?

[yosin_UTC9, 2016-12-08 09:30:36]

On 2016/12/08 at 08:21:41, drott wrote:
> Thanks for the details explaining the intended behavior and implications, that
helped.
> 
> > Regarding the caret height, the caret height should be the
SimpleFontData->getFontMetrics() height at each glyph
> position.
> 
> Joone, what's the status of this CL now and which case or cases is it
intending to fix? We seem to agree that the caret height should be
SimpleFontData->getFontMetrics() height at each glyph position. Does this CL aim
for solving that, or is it only fixing font at position 0, where - as yosin@
explains - it would be the first available font, i.e.
Font->primaryFont()->getFontMetrics().height()
> 
> How about a case like a Latin primary font, e.g. Arial, and then typing tall
Burmese characters, like Myanmar Letter O ဩ, which span and exceed the usual
Latin ascenders - is the caret height adjusting for this situation and is the
intention to cover this in this CL or in separate ones?

We should commit this patch when caret height is calculated from the font of a
character *before* caret for making users not to be surprised when inserting
taller character than a character before caret.

We'll use Font->primaryFont()->getFontMetrics() in
LayoutBlockModel::localCaretRectForEmptyElement()

[joone, 2016-12-13 02:17:11]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 02:18:04]

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, 2016-12-13 03:42:49]

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

[commit-bot: I haz the power, 2016-12-13 03:42:50]

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

[joone, 2016-12-13 23:32:56]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 23:33:42]

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

[joone, 2016-12-14 00:23:25]

On 2016/12/08 09:30:36, Yosi_UTC9 wrote:
> On 2016/12/08 at 08:21:41, drott wrote:
> > Thanks for the details explaining the intended behavior and implications,
that
> helped.
> > 
> > > Regarding the caret height, the caret height should be the
> SimpleFontData->getFontMetrics() height at each glyph
> > position.
> > 
> > Joone, what's the status of this CL now and which case or cases is it
> intending to fix? We seem to agree that the caret height should be
> SimpleFontData->getFontMetrics() height at each glyph position. Does this CL
aim
> for solving that, or is it only fixing font at position 0, where - as yosin@
> explains - it would be the first available font, i.e.
> Font->primaryFont()->getFontMetrics().height()

I applied yosin's suggestion to the CL so it gets the caret height from an
InlineBox before caret position.

> > How about a case like a Latin primary font, e.g. Arial, and then typing tall
> Burmese characters, like Myanmar Letter O ဩ, which span and exceed the usual
> Latin ascenders - is the caret height adjusting for this situation and is the
> intention to cover this in this CL or in separate ones?

I will cover this case in separate CL.

> We should commit this patch when caret height is calculated from the font of a
> character *before* caret for making users not to be surprised when inserting
> taller character than a character before caret.

I did so please take a look at the updated CL.
 
> We'll use Font->primaryFont()->getFontMetrics() in
> LayoutBlockModel::localCaretRectForEmptyElement()

It is already used.


Currently, the following tests failed:

webkit_unit_tests (with patch) webkit_unit_tests (with patch)

failures:
GranularityStrategyTest.DirectionShrinkFontSizes
GranularityStrategyTest.DirectionSwitchSideFontSizes
GranularityStrategyTest.DirectionSwitchSideVerticalAlign
GranularityStrategyTest.DirectionExpandFontSizes
GranularityStrategyTest.DirectionShrinkVerticalAlign

If I remove the style in span tag in GranularityStrategyTest::setupFontSize
, the test works fine. So, it looks like the font height is not updated in this
test when the span tag is appended.

[joone, 2016-12-14 00:24:26]

Patchset #7 (id:160001) has been deleted

[joone, 2016-12-14 00:24:41]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 00:25:03]

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

[joone, 2016-12-14 00:25:43]

Patchset #7 (id:180001) has been deleted

[commit-bot: I haz the power, 2016-12-14 01:41:19]

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

[commit-bot: I haz the power, 2016-12-14 01:41:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[drott, 2016-12-14 16:42:22]

Thanks for the update, Joone.

https://codereview.chromium.org/2517383002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2517383002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutText.cpp:737: if
(box->prevLeafChild() && caretOffset == 0)
In https://bugs.chromium.org/p/chromium/issues/detail?id=588060#c12 Yosin
mentions 
"The actual font for calculating caret rectangle should be taken from a
character around caret in following order:
1. a character before caret; left for dir=LTR, right for dir=RTL"

So, I believe this needs to take RTL into account here, and it would be good to
have an additional test which covers and illustrates this font boundary size
change.

[joone, 2016-12-16 00:26:05]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 00:27: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, 2016-12-16 00:34:38]

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

[commit-bot: I haz the power, 2016-12-16 00:34:39]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[joone, 2016-12-16 01:27:23]

Patchset #8 (id:220001) has been deleted

[joone, 2016-12-16 09:01:38]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 09:02:04]

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, 2016-12-16 10:19:43]

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

[commit-bot: I haz the power, 2016-12-16 10:19:44]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[joone, 2016-12-17 01:06:52]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-17 01:07:46]

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

[joone, 2016-12-17 01:15:29]

On 2016/12/14 16:42:22, drott wrote:
> Thanks for the update, Joone.
> 
>
https://codereview.chromium.org/2517383002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):
> 
>
https://codereview.chromium.org/2517383002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/layout/LayoutText.cpp:737: if
> (box->prevLeafChild() && caretOffset == 0)
> In https://bugs.chromium.org/p/chromium/issues/detail?id=588060#c12 Yosin
> mentions 
> "The actual font for calculating caret rectangle should be taken from a
> character around caret in following order:
> 1. a character before caret; left for dir=LTR, right for dir=RTL"
> 
> So, I believe this needs to take RTL into account here, and it would be good
to
> have an additional test which covers and illustrates this font boundary size
> change.

drott@ The latest CL considered RTL and added additional tests to cover RTL and
font boundary size change.
The only problem is that the following tests failed:

GranularityStrategyTest.DirectionShrinkFontSizes
GranularityStrategyTest.DirectionSwitchSideFontSizes
GranularityStrategyTest.DirectionSwitchSideVerticalAlign
GranularityStrategyTest.DirectionExpandFontSizes
GranularityStrategyTest.DirectionShrinkVerticalAlign

Because visiblePositionToContentsPoint() returns a different value due to the
change of the caret height.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/G...

drott@ yosin@ do you have any idea to fix this?

Thanks!

[commit-bot: I haz the power, 2016-12-17 02:19:40]

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

[commit-bot: I haz the power, 2016-12-17 02:19:42]

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

[drott, 2016-12-19 13:55:35]

On 2016/12/17 at 01:15:29, joone.hur wrote:

> drott@ The latest CL considered RTL and added additional tests to cover RTL
and font boundary size change.
> The only problem is that the following tests failed:

Joone, thanks for reworking this CL multiple times and sorry that the review
took several cycles. Change now LGTM, thanks for adding the good tests.

> GranularityStrategyTest.DirectionShrinkFontSizes
> GranularityStrategyTest.DirectionSwitchSideFontSizes
> GranularityStrategyTest.DirectionSwitchSideVerticalAlign
> GranularityStrategyTest.DirectionExpandFontSizes
> GranularityStrategyTest.DirectionShrinkVerticalAlign
> 
> Because visiblePositionToContentsPoint() returns a different value due to the
change of the caret height.
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/G...
> 
> drott@ yosin@ do you have any idea to fix this?

Unfortunately, I don't know this area very well - yosin@ might have more ideas.

[joone, 2016-12-20 00:02:53]

On 2016/12/19 13:55:35, drott wrote:
> On 2016/12/17 at 01:15:29, joone.hur wrote:
> 
> > drott@ The latest CL considered RTL and added additional tests to cover RTL
> and font boundary size change.
> > The only problem is that the following tests failed:
> 
> Joone, thanks for reworking this CL multiple times and sorry that the review
> took several cycles. Change now LGTM, thanks for adding the good tests.
> 
> > GranularityStrategyTest.DirectionShrinkFontSizes
> > GranularityStrategyTest.DirectionSwitchSideFontSizes
> > GranularityStrategyTest.DirectionSwitchSideVerticalAlign
> > GranularityStrategyTest.DirectionExpandFontSizes
> > GranularityStrategyTest.DirectionShrinkVerticalAlign
> > 
> > Because visiblePositionToContentsPoint() returns a different value due to
the
> change of the caret height.
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/G...
> > 
> > drott@ yosin@ do you have any idea to fix this?
> 
> Unfortunately, I don't know this area very well - yosin@ might have more
ideas.

drott@ thanks for the review!

[joone, 2017-01-04 01:56:36]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 01:56: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-04 03:16:55]

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

[commit-bot: I haz the power, 2017-01-04 03:16:56]

Dry run: This issue passed the CQ dry run.

[joone, 2017-01-05 01:58:46]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

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

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-05 03:43:36]

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

[commit-bot: I haz the power, 2017-01-05 03:43:37]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-01-05 07:31:32]

Happy 2017! Sorry for late response. Now, I'm back in work.

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/editing/caret/caret-height-multi-line.html
(right):

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/caret/caret-height-multi-line.html:27:
var editor = document.getElementById('test1');
%s/var/const/ or using |let|

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/caret/caret-height-multi-line.html:30:
var caretHeight1 = window.internals.absoluteCaretBounds().height;
Please add assertion for availability of window.internals, e.g.

assert_not_equals(window.internal, undefined, 'This test requires
window.internal');
or

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/caret/caret-height-multi-line.html:82:
sel.modify("move", "left", "character");
Please use single-quote since other parts of script use single-quote.

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/VisibleUnits.h (right):

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/VisibleUnits.h:123: CORE_EXPORT IntRect
absoluteSelectionBoundsOf(const VisiblePosition&);
Can we move introducing absoluteSelectionBoundsOf() and updating
GranularityStrategy.cpp and GranularityStrategyTest.cpp in another patch?

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/VisibleUnits.h:313: CORE_EXPORT
LayoutRect localSelectionRectOfPosition(const PositionWithAffinity&,
It seems we don't need to export this function.

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutText.cpp:759: int top =
caretBox->logicalTop().toInt();
Is this baseline.y - font.ascent?

[joone, 2017-01-06 20:40:43]

Happy new year too!

I will update this CL once https://codereview.chromium.org/2616103003/ lands.

Thanks!

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/editing/caret/caret-height-multi-line.html
(right):

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/caret/caret-height-multi-line.html:27:
var editor = document.getElementById('test1');
On 2017/01/05 07:31:31, Yosi_UTC9 wrote:
> %s/var/const/ or using |let|

Acknowledged.

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/caret/caret-height-multi-line.html:30:
var caretHeight1 = window.internals.absoluteCaretBounds().height;
On 2017/01/05 07:31:31, Yosi_UTC9 wrote:
> Please add assertion for availability of window.internals, e.g.
> 
> assert_not_equals(window.internal, undefined, 'This test requires
> window.internal');
> or

Acknowledged.

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/editing/caret/caret-height-multi-line.html:82:
sel.modify("move", "left", "character");
On 2017/01/05 07:31:31, Yosi_UTC9 wrote:
> Please use single-quote since other parts of script use single-quote.

Acknowledged.

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/VisibleUnits.h (right):

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/VisibleUnits.h:123: CORE_EXPORT IntRect
absoluteSelectionBoundsOf(const VisiblePosition&);
On 2017/01/05 07:31:31, Yosi_UTC9 wrote:
> Can we move introducing absoluteSelectionBoundsOf() and updating
> GranularityStrategy.cpp and GranularityStrategyTest.cpp in another patch?

Done: https://codereview.chromium.org/2616103003/

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/VisibleUnits.h:313: CORE_EXPORT
LayoutRect localSelectionRectOfPosition(const PositionWithAffinity&,
On 2017/01/05 07:31:31, Yosi_UTC9 wrote:
> It seems we don't need to export this function.

Removed.

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutText.cpp (right):

https://codereview.chromium.org/2517383002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutText.cpp:759: int top =
caretBox->logicalTop().toInt();
On 2017/01/05 07:31:31, Yosi_UTC9 wrote:
> Is this baseline.y - font.ascent?

Yes

[joone, 2017-01-10 23:19:20]

Patchset #12 (id:320001) has been deleted

[joone, 2017-01-10 23:22:41]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 23:23:46]

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

[joone, 2017-01-10 23:34:06]

Patchset #12 (id:340001) has been deleted

[joone, 2017-01-10 23:34:25]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 23:35:12]

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

[joone, 2017-01-11 00:55:00]

The CQ bit was checked by joone.hur@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 00:55:38]

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

[joone, 2017-01-11 01:00:26]

yosin@ please review the updated CL.

[yosin_UTC9, 2017-01-11 01:21:32]

lgtm

Thanks for doing this!
And sorry for long latency due by my vacation.

[commit-bot: I haz the power, 2017-01-11 05:39:46]

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

[commit-bot: I haz the power, 2017-01-11 05:39:47]

Dry run: This issue passed the CQ dry run.

[joone, 2017-01-11 05:59:00]

The CQ bit was checked by joone.hur@intel.com

[joone, 2017-01-11 05:59:00]

The patchset sent to the CQ was uploaded after l-g-t-m from drott@chromium.org
Link to the patchset: https://codereview.chromium.org/2517383002/#ps380001
(title: "fix test fail")

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

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-11 06:06:14]

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

[commit-bot: I haz the power, 2017-01-11 06:06:16]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[joone, 2017-01-11 06:29:23]

joone.hur@intel.com changed reviewers:
+ tkent@chromium.org

[joone, 2017-01-11 06:29:25]

tkent@ could you review the change in LayoutText.cpp?

[tkent, 2017-01-11 06:42:12]

lgtm

[joone, 2017-01-11 06:55:21]

The CQ bit was checked by joone.hur@intel.com

[commit-bot: I haz the power, 2017-01-11 06:55:44]

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-11 07:00:17]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1484117721458730,
"parent_rev": "0c11ed986f2c7da313eaca87d592b6bd21cee383", "commit_rev":
"c5291a0115a65797dbf7992c8aa47136db95ae6a"}

[commit-bot: I haz the power, 2017-01-11 07:00:47]

Description was changed from

==========
Use the font height as caret height instead of the line height

The caret height should be calculated by using the FontMetrics
instead of the selection height.

BUG=588060
TEST=editing/caret/caret-height-multi-line.html
==========

to

==========
Use the font height as caret height instead of the line height

The caret height should be calculated by using the FontMetrics
instead of the selection height.

BUG=588060
TEST=editing/caret/caret-height-multi-line.html

Review-Url: https://codereview.chromium.org/2517383002
Cr-Commit-Position: refs/heads/master@{#442830}
Committed:
https://chromium.googlesource.com/chromium/src/+/c5291a0115a65797dbf7992c8aa4...
==========

[commit-bot: I haz the power, 2017-01-11 07:00:48]

Committed patchset #13 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/c5291a0115a65797dbf7992c8aa4...