Include glyph overflow in SVG text bounding boxes

Include glyph overflow in SVG text bounding boxes

Glyph overflow is the area that extends past the layout bounds of a
glyph. Both the paint invalidation bounding boxes and the returned
value from SVGTextElement.getBBox() did not include the glyph
overflow. This can be seen in repaint bugs around compositor tile
boundaries if text is animated, or by looking at the result of
getBBox().

This patch stores the glyph overflow when computing text metrics and
adds this overflow only to the bounding box so layout values are
unaffected. With this patch we now match the behavior of Gecko.

BUG=566285
Committed: https://crrev.com/50972884330c51464b56f9082d4da49330e9f6b3
Cr-Commit-Position: refs/heads/master@{#366291}

[pdr., 2015-12-07 06:46:52]

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

[pdr., 2015-12-07 06:46:59]

pdr@chromium.org changed reviewers:
+ fs@opera.com

[commit-bot: I haz the power, 2015-12-07 06:48:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505713002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505713002/1

[pdr., 2015-12-07 06:50:34]

@fs, this isn't quite ready to go (running all the bots to get the list of test
failures/updates) but would you like to review this at a high level?

[commit-bot: I haz the power, 2015-12-07 08:07:24]

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

[commit-bot: I haz the power, 2015-12-07 08:07:24]

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

[fs, 2015-12-07 10:00:49]

Description was changed from

==========
Include glyph overflow in SVG text bounding boxes

Glyph overflow is the area that extends past the layout bounds of a
glyph. Both the paint invalidation bounding boxes and the returned
value from SVGTextElement.getBBox() did not include the glyph
overflow. This can be seen in repaint bugs around compositor tile
boundaries if text is animated, or by looking at the result of
getBBox().

This patch stores the glyph overflow when computing text metrics and
adds this overflow only to the bounding box so layout values are
unaffected. With this patch we not match the behavior of Gecko.

BUG=566285
==========

to

==========
Include glyph overflow in SVG text bounding boxes

Glyph overflow is the area that extends past the layout bounds of a
glyph. Both the paint invalidation bounding boxes and the returned
value from SVGTextElement.getBBox() did not include the glyph
overflow. This can be seen in repaint bugs around compositor tile
boundaries if text is animated, or by looking at the result of
getBBox().

This patch stores the glyph overflow when computing text metrics and
adds this overflow only to the bounding box so layout values are
unaffected. With this patch we now match the behavior of Gecko.

BUG=566285
==========

[fs, 2015-12-07 10:32:30]

On 2015/12/07 at 06:50:34, pdr wrote:
> @fs, this isn't quite ready to go (running all the bots to get the list of
test failures/updates) but would you like to review this at a high level?

From a storage PoV it seems a better to try and compute the overflow after
forming the fragments (and store overflow per-fragment.)

https://codereview.chromium.org/1505713002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp
(right):

https://codereview.chromium.org/1505713002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp:247:
fragment.x - textMetrics.glyphOverflowLeft(),
Shouldn't this include the overflow from (potentially) all glyphs? This looks
like it will only include overflow from the first glyph (which might work in the
tests since they only use a single glyph - and here we expand the entire
fragment "enough" because of that.)

Using per-glyph overflow metrics it seems you'd need to walk the glyphs in the
fragment and sort of "push" the overflow through (in the "inline" direction -
the other direction should be more obvious simple).

[pdr., 2015-12-14 05:11:06]

Thanks!

On 2015/12/07 at 10:32:30, fs wrote:
> On 2015/12/07 at 06:50:34, pdr wrote:
> > @fs, this isn't quite ready to go (running all the bots to get the list of
test failures/updates) but would you like to review this at a high level?
> 
> From a storage PoV it seems a better to try and compute the overflow after
forming the fragments (and store overflow per-fragment.)

It's unfortunate that this requires an additional walk over the glyphs, but as
you pointed out below, we need to do that anyway :/

> 
>
https://codereview.chromium.org/1505713002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp
(right):
> 
>
https://codereview.chromium.org/1505713002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp:247:
fragment.x - textMetrics.glyphOverflowLeft(),
> Shouldn't this include the overflow from (potentially) all glyphs? This looks
like it will only include overflow from the first glyph (which might work in the
tests since they only use a single glyph - and here we expand the entire
fragment "enough" because of that.)
> 
> Using per-glyph overflow metrics it seems you'd need to walk the glyphs in the
fragment and sort of "push" the overflow through (in the "inline" direction -
the other direction should be more obvious simple).

Good catch. The latest patch does this properly, and adds a test to prove it.

('faQ' in a cursive font will have the f spill out before the first apostrophe,
and the Q will spill past the last apostrophe.)


If this looks reasonable to you, I'll update the test expectations, figure out
why Harfbuzz gets confused with spaces + glyph offsets, and put this up for
landing.

[pdr., 2015-12-14 05:11:17]

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

[commit-bot: I haz the power, 2015-12-14 05:11:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505713002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505713002/20001

[commit-bot: I haz the power, 2015-12-14 06:33:56]

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

[commit-bot: I haz the power, 2015-12-14 06:33:57]

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

[fs, 2015-12-14 09:59:31]

On 2015/12/14 at 05:11:06, pdr wrote:
> Thanks!
> 
> On 2015/12/07 at 10:32:30, fs wrote:
> > On 2015/12/07 at 06:50:34, pdr wrote:
> > > @fs, this isn't quite ready to go (running all the bots to get the list of
test failures/updates) but would you like to review this at a high level?
> > 
> > From a storage PoV it seems a better to try and compute the overflow after
forming the fragments (and store overflow per-fragment.)
> 
> It's unfortunate that this requires an additional walk over the glyphs, but as
you pointed out below, we need to do that anyway :/

Yeah, if that turns out to be a problem, maybe we could store a flag in
SVGTextMetrics which indicates if the "glyph" overflows or not and then use that
to try and avoid computeGlyphOverflow when we can (assuming the overflow will be
available anyway during SVGTextMetrics computations.)

https://codereview.chromium.org/1505713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/SVGTextFragment.h (right):

https://codereview.chromium.org/1505713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextFragment.h:74: float
glyphOverflowTop;
I guess the time has come for me to respin and land the CL that shrinks
SVGTextFragment by roughly this amount of floats... =)

https://codereview.chromium.org/1505713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp (right):

https://codereview.chromium.org/1505713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp:115: void
computeGlyphOverflow(SVGInlineTextBox* textBox, SVGTextFragment& textFragment)
Nit: static?

https://codereview.chromium.org/1505713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/SVGTextMetrics.h (left):

https://codereview.chromium.org/1505713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextMetrics.h:56: 
Nit: I like the new blank line, but I think I'd like this one back too =)

[fs, 2015-12-14 09:59:53]

On 2015/12/14 at 05:11:06, pdr wrote:
> Thanks!
> 
> On 2015/12/07 at 10:32:30, fs wrote:
> > On 2015/12/07 at 06:50:34, pdr wrote:
> > > @fs, this isn't quite ready to go (running all the bots to get the list of
test failures/updates) but would you like to review this at a high level?
> > 
> > From a storage PoV it seems a better to try and compute the overflow after
forming the fragments (and store overflow per-fragment.)
> 
> It's unfortunate that this requires an additional walk over the glyphs, but as
you pointed out below, we need to do that anyway :/
> 
> > 
> >
https://codereview.chromium.org/1505713002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp
(right):
> > 
> >
https://codereview.chromium.org/1505713002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/layout/svg/line/SVGInlineTextBox.cpp:247:
fragment.x - textMetrics.glyphOverflowLeft(),
> > Shouldn't this include the overflow from (potentially) all glyphs? This
looks like it will only include overflow from the first glyph (which might work
in the tests since they only use a single glyph - and here we expand the entire
fragment "enough" because of that.)
> > 
> > Using per-glyph overflow metrics it seems you'd need to walk the glyphs in
the fragment and sort of "push" the overflow through (in the "inline" direction
- the other direction should be more obvious simple).
> 
> Good catch. The latest patch does this properly, and adds a test to prove it.
> 
> ('faQ' in a cursive font will have the f spill out before the first
apostrophe, and the Q will spill past the last apostrophe.)
> 
> 
> If this looks reasonable to you, I'll update the test expectations, figure out
why Harfbuzz gets confused with spaces + glyph offsets, and put this up for
landing.

LRTM

[pdr., 2015-12-15 22:18:33]

Description was changed from

==========
Include glyph overflow in SVG text bounding boxes

Glyph overflow is the area that extends past the layout bounds of a
glyph. Both the paint invalidation bounding boxes and the returned
value from SVGTextElement.getBBox() did not include the glyph
overflow. This can be seen in repaint bugs around compositor tile
boundaries if text is animated, or by looking at the result of
getBBox().

This patch stores the glyph overflow when computing text metrics and
adds this overflow only to the bounding box so layout values are
unaffected. With this patch we now match the behavior of Gecko.

BUG=566285
==========

to

==========
Include glyph overflow in SVG text bounding boxes

Glyph overflow is the area that extends past the layout bounds of a
glyph. Both the paint invalidation bounding boxes and the returned
value from SVGTextElement.getBBox() did not include the glyph
overflow. This can be seen in repaint bugs around compositor tile
boundaries if text is animated, or by looking at the result of
getBBox().

This patch stores the glyph overflow when computing text metrics and
adds this overflow only to the bounding box so layout values are
unaffected. With this patch we now match the behavior of Gecko.

BUG=566285,569833
==========

[pdr., 2015-12-15 22:18:45]

Description was changed from

==========
Include glyph overflow in SVG text bounding boxes

Glyph overflow is the area that extends past the layout bounds of a
glyph. Both the paint invalidation bounding boxes and the returned
value from SVGTextElement.getBBox() did not include the glyph
overflow. This can be seen in repaint bugs around compositor tile
boundaries if text is animated, or by looking at the result of
getBBox().

This patch stores the glyph overflow when computing text metrics and
adds this overflow only to the bounding box so layout values are
unaffected. With this patch we now match the behavior of Gecko.

BUG=566285,569833
==========

to

==========
Include glyph overflow in SVG text bounding boxes

Glyph overflow is the area that extends past the layout bounds of a
glyph. Both the paint invalidation bounding boxes and the returned
value from SVGTextElement.getBBox() did not include the glyph
overflow. This can be seen in repaint bugs around compositor tile
boundaries if text is animated, or by looking at the result of
getBBox().

This patch stores the glyph overflow when computing text metrics and
adds this overflow only to the bounding box so layout values are
unaffected. With this patch we now match the behavior of Gecko.

BUG=566285
==========

[pdr., 2015-12-16 04:27:45]

Just a minor update. Once https://codereview.chromium.org/1524913003 lands, I'll
update the tests and put this up for a big kid review.

https://codereview.chromium.org/1505713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp (right):

https://codereview.chromium.org/1505713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp:115: void
computeGlyphOverflow(SVGInlineTextBox* textBox, SVGTextFragment& textFragment)
On 2015/12/14 at 09:59:31, fs wrote:
> Nit: static?

Done

https://codereview.chromium.org/1505713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp:125: //
TODO(pdr): The glyph overflow bounds can be wrong for runs with spaces.
I have a fix for this in https://codereview.chromium.org/1524913003. I'd like to
land that patch before this one so I don't have to rebaseline all of SVG twice.

https://codereview.chromium.org/1505713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/SVGTextMetrics.h (left):

https://codereview.chromium.org/1505713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextMetrics.h:56: 
On 2015/12/14 at 09:59:31, fs wrote:
> Nit: I like the new blank line, but I think I'd like this one back too =)

Sigh, I was going to use this one in a followup patch. I guess I can put it back
for now.

[fs, 2015-12-16 10:12:21]

lgtm

https://codereview.chromium.org/1505713002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp (right):

https://codereview.chromium.org/1505713002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextLayoutEngine.cpp:125: //
TODO(pdr): The glyph overflow bounds can be wrong for runs with spaces.
On 2015/12/16 at 04:27:45, pdr wrote:
> I have a fix for this in https://codereview.chromium.org/1524913003. I'd like
to land that patch before this one so I don't have to rebaseline all of SVG
twice.

Roger Rogers.

[pdr., 2015-12-16 22:27:07]

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

[commit-bot: I haz the power, 2015-12-16 22:28:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505713002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505713002/60001

[commit-bot: I haz the power, 2015-12-16 23:42:21]

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

[commit-bot: I haz the power, 2015-12-16 23:42:22]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[pdr., 2015-12-17 04:06:07]

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

[commit-bot: I haz the power, 2015-12-17 04:06:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505713002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505713002/140001

[pdr., 2015-12-17 04:09:49]

This is ready for a real review!

There is one change in the c++ side of things: previous patches ceil'd the
result in GlyphOverflow::*Overflow(...) but I moved that to just happen for the
int version.

Most of the expectation updates are simple 1-px differences due to adding a
minute amount of overflow. The trybot results from patch set 4 have all the
failures if you'd like to take a look.

[commit-bot: I haz the power, 2015-12-17 04:45:20]

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

[commit-bot: I haz the power, 2015-12-17 04:45:21]

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

[fs, 2015-12-17 09:28:49]

lgtm

Given the look of the TE file, I'll let you land this yourself.

[pdr., 2015-12-20 00:30:13]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-12-20 00:30:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1505713002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1505713002/140001

[commit-bot: I haz the power, 2015-12-20 02:14:02]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-20 02:15:08]

Description was changed from

==========
Include glyph overflow in SVG text bounding boxes

Glyph overflow is the area that extends past the layout bounds of a
glyph. Both the paint invalidation bounding boxes and the returned
value from SVGTextElement.getBBox() did not include the glyph
overflow. This can be seen in repaint bugs around compositor tile
boundaries if text is animated, or by looking at the result of
getBBox().

This patch stores the glyph overflow when computing text metrics and
adds this overflow only to the bounding box so layout values are
unaffected. With this patch we now match the behavior of Gecko.

BUG=566285
==========

to

==========
Include glyph overflow in SVG text bounding boxes

Glyph overflow is the area that extends past the layout bounds of a
glyph. Both the paint invalidation bounding boxes and the returned
value from SVGTextElement.getBBox() did not include the glyph
overflow. This can be seen in repaint bugs around compositor tile
boundaries if text is animated, or by looking at the result of
getBBox().

This patch stores the glyph overflow when computing text metrics and
adds this overflow only to the bounding box so layout values are
unaffected. With this patch we now match the behavior of Gecko.

BUG=566285

Committed: https://crrev.com/50972884330c51464b56f9082d4da49330e9f6b3
Cr-Commit-Position: refs/heads/master@{#366291}
==========

[commit-bot: I haz the power, 2015-12-20 02:15:09]

Patchset 8 (id:??) landed as
https://crrev.com/50972884330c51464b56f9082d4da49330e9f6b3
Cr-Commit-Position: refs/heads/master@{#366291}

[manumedinam, 2016-01-30 08:17:58]

manumedinam@gmail.com changed reviewers:
+ manumedinam@gmail.com