Issue 218863002: [FastTextAutosizer] Fix the inline overlap issue.

Issue 218863002: [FastTextAutosizer] Fix the inline overlap issue. (Closed)

Created:
6 years, 8 months ago by pdr.

Modified:
6 years, 8 months ago

Reviewers:
skobes (do not use), skobes

CC:
blink-reviews, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, rune+blink

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

[FastTextAutosizer] Fix the inline overlap issue. Inline renderers have preferred logical widths that end up depending on the autosizing font scale. This patch forces these preferred widths to be recomputed. This recomputation is not necessary for text in blocks because the block layout algorithm does not depend on the text width. The parent null check (renderer->parent()...) is for list markers which, interestingly, can not be in the render tree. This patch fixes rendering on many sites, including hacker news. BUG=338494 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170638

Patch Set 1 #

Total comments: 6

Patch Set 2 : Cleanup test #

Patch Set 3 : Move the pref width invalidation into inflate, add a test #

Patch Set 4 : Really move setPreferredLogicalWidthsDirty #

Patch Set 5 : Add fixme #

Patch Set 6 : update expectations #

Created: 6 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+153 lines, -0 lines)			Patch
A	LayoutTests/fast/text-autosizing/inline-width.html	View	1 2	1 chunk	+71 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/text-autosizing/inline-width-expected.txt	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	LayoutTests/fast/text-autosizing/table-inline-width.html	View	1 2	1 chunk	+77 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/text-autosizing/table-inline-width-expected.txt	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/rendering/FastTextAutosizer.cpp	View	1 2 3 4	1 chunk	+3 lines, -0 lines	0 comments	Download

Messages

Total messages: 12 (0 generated)

Expand Messages | Collapse Messages

skobes

Very nice! https://codereview.chromium.org/218863002/diff/1/LayoutTests/fast/text-autosizing/inline-width.html File LayoutTests/fast/text-autosizing/inline-width.html (right): https://codereview.chromium.org/218863002/diff/1/LayoutTests/fast/text-autosizing/inline-width.html#newcode8 LayoutTests/fast/text-autosizing/inline-width.html:8: .inline { display: inline; font-weight: normal; font-style: ...

6 years, 8 months ago (2014-03-31 17:19:58 UTC) #2

pdr.

PTAL https://codereview.chromium.org/218863002/diff/1/LayoutTests/fast/text-autosizing/inline-width.html File LayoutTests/fast/text-autosizing/inline-width.html (right): https://codereview.chromium.org/218863002/diff/1/LayoutTests/fast/text-autosizing/inline-width.html#newcode8 LayoutTests/fast/text-autosizing/inline-width.html:8: .inline { display: inline; font-weight: normal; font-style: normal; ...

6 years, 8 months ago (2014-04-01 06:06:42 UTC) #3

PTAL

https://codereview.chromium.org/218863002/diff/1/LayoutTests/fast/text-autosi...
File LayoutTests/fast/text-autosizing/inline-width.html (right):

https://codereview.chromium.org/218863002/diff/1/LayoutTests/fast/text-autosi...
LayoutTests/fast/text-autosizing/inline-width.html:8: .inline { display: inline;
font-weight: normal; font-style: normal; background-color: green; }
On 2014/03/31 17:19:58, skobes wrote:
> Suggest putting each property on its own line for readability.

Done.

https://codereview.chromium.org/218863002/diff/1/LayoutTests/fast/text-autosi...
LayoutTests/fast/text-autosizing/inline-width.html:16: onload = function() {
On 2014/03/31 17:19:58, skobes wrote:
> Nit: you can probably inline this at the bottom of the <body> instead of using
> the onload event.

Done.

https://codereview.chromium.org/218863002/diff/1/Source/core/rendering/FastTe...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/218863002/diff/1/Source/core/rendering/FastTe...
Source/core/rendering/FastTextAutosizer.cpp:808:
renderer->setPreferredLogicalWidthsDirty(MarkOnlyThis);
On 2014/03/31 17:19:58, skobes wrote:
> Do you know why this works?  I would have guessed MarkContainingBlockChain
would
> be needed here.

A good question. In looking into this I found our table layout code is very
broken. Lets look into that tomorrow (crbug.com/358510).

In standard block layout where the block contains inline children, FTA::inflate
gets called after the block's width is computed but before laying out the
children.
In normal layout the FTA flow looks like:
1) At the top of RenderBlock:Layout we do the inflation
2) During inflation we invalidate any pref widths of children
3) Later in RenderBlock::Layout we query the widths of all text nodes
(layoutInlineChildren)
4) Querying the widths with dirty pref widths will recompute them.

Because all children are walked, we can get away with MarkOnlyThis.

You may be wondering... how did my test exercise stale preferred logical widths?
It did, but only by accident. The two-pass layout caused by scrollbars caused
the pref widths to get invalidated. I've hidden the scrollbars so this test now
passes before and after my change, but I think this test is still useful to keep
around.

I've added a new test, table-inline-width, which does properly exercise the new
inline width code. It's not clear how inflateTable should work here so I've
moved the setPreferredLogicalWidthsDirty call into FTA::inflate until the table
code is more sane.

skobes

LGTM On 2014/04/01 06:06:42, pdr wrote: > Because all children are walked, we can get ...

6 years, 8 months ago (2014-04-01 18:49:05 UTC) #4

pdr.

On 2014/04/01 18:49:05, skobes wrote: > LGTM > > On 2014/04/01 06:06:42, pdr wrote: > ...

6 years, 8 months ago (2014-04-01 22:27:12 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pdr@chromium.org/218863002/80001

6 years, 8 months ago (2014-04-01 22:44:10 UTC) #7

commit-bot: I haz the power

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

6 years, 8 months ago (2014-04-01 23:18:16 UTC) #8

commit-bot: I haz the power

Try jobs failed on following builders: tryserver.blink on linux_blink_rel

6 years, 8 months ago (2014-04-01 23:18:16 UTC) #9

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pdr@chromium.org/218863002/100001

6 years, 8 months ago (2014-04-02 04:47:27 UTC) #11

Message was sent while issue was closed.

Change committed as 170638

Expand Messages | Collapse Messages