Bug #889721: LayoutInlines that do not generate fragments will not paint outlines

[Aleks Totic]

https://bugs.chromium.org/p/chromium/issues/detail?id=889721
failing test: fast/inline/continuation-outlines.html

Here is a bug that @kojii and I have been trying to fix, and there is no clean fix. 

cc'ing layout-dev just in case you have some better ideas. I am inspired to email 

you all by seeing @mstensho fix a floating bug that had me busy for a week in a day.

The bug concerns painting of LayoutInline outlines.

LayoutInline outline is a union of fragments generated by LayoutInline, 

and fragments generated by all of LayoutInline's children. 

This includes continuations, and children's continuations. 

Implementing this is awkward. Outline can spans multiple fragments, 

but needs to be painted by one fragment. That fragment will have InkOverflow

that spans all outlines together.

NG solves "multiple fragments painted by one" problem by designating 

first fragment of LayoutInline as "outline painting fragment".

This works well as long as LayoutInline generates at least one fragment.

Simplest failure case is <span .outline><div></div></span>

There are two ways to fix this:

1) In Layout, make sure that LayoutInline always generates at least one fragment.
@kojii tried this, and 43 tests failed.

It would also mean more memory usage, unsure how much.

2) in Paint, detect that LayoutInline has not created any fragments, and 

paint missing LayoutInline directly.

I've been trying to implement this, and it is more complex than 

I thought. Some issues, and my attempts at solving them:

A) Who should paint invisible LayoutInline? 

Fragment generated by LayoutInline's containing block. 

I named this fragment "GhostPainter"

B) How does GhostPainter detect that it has LayoutInlines it should paint?

I do not have a good solution for this. We can't just traverse all element children

all the time, that would be too inefficient.

Even worse, I think we need to traverse not just children, but descendants, 

because of this case:

<span><span .outline><div<</div></span></span>

The list of LayoutInline's with outlines cannot be stored on fragments, 

because we must not relayout when outline status changes.

My current guess is, after looking at fragment trees for a while: 

If GhostPainter has empty LineBox children, inside GhostPainter.GetLayoutObject

search for LayoutInline's with outline, and without fragments. 

I do not think this is strictly correct, I can imagine something like

<span>foo</span><span .outline><div></div></span><span>bar</span>

not generating any empty LineBoxes.

C) GhostPainter also needs to do this search during Layout to

C.1) compute its InkOverflow.

C.2) Tell paint layer that it needs kOutline paint phase.

D) Need a bit of hacking to paint LayoutInline outline when it 

is laid out by NG, but should be painted by Legacy.

I think we need to solve the search problem inside inline_layout_algorithm, 

and store LayoutInlines that did not generate fragments on LineBox. 

Then I can use this list to solve C) and D) efficiently.

@Koji Ishii  wdyt?

Solving this has been tough for me, because my knowledge of 

inline_layout_algorithm is limited, I cannot predict with certainty

what fragments will be generated by a layout tree.

Aleks

[Morten Stenshorne]

"'Aleks Totic' via layout-dev" <layou...@chromium.org> writes:

> https://bugs.chromium.org/p/chromium/issues/detail?id=889721
> failing test: fast/inline/continuation-outlines.html
>
> Here is a bug that @kojii and I have been trying to fix, and there is no clean fix.
> cc'ing layout-dev just in case you have some better ideas. I am inspired to email
> you all by seeing @mstensho fix a floating bug that had me busy for a week in a day.
>
> The bug concerns painting of LayoutInline outlines.
>
> LayoutInline outline is a union of fragments generated by LayoutInline,
> and fragments generated by all of LayoutInline's children.
> This includes continuations, and children's continuations.

This is only a problem when there are continuations, right? But this bug
is a blocker for phase 1, right? Because, if NOT: What if there was no
such thing as continuations? :) I.e. how about getting rid of
continuations for LayoutNG?

I think WebKit implements blocks inside inlines as special inline-blocks
(that don't shrink to fit, don't establish a BFC, and so on) these days,
for instance.

--
Morten Stenshorne, Software developer,
Blink/Layout, Google, Oslo, Norway

[Aleks Totic]

On Sun, Oct 14, 2018 at 12:48 PM Morten Stenshorne <mste...@chromium.org> wrote:

"'Aleks Totic' via layout-dev" <layou...@chromium.org> writes:
> LayoutInline outline is a union of fragments generated by LayoutInline,
> and fragments generated by all of LayoutInline's children.
> This includes continuations, and children's continuations.

This is only a problem when there are continuations, right? But this bug
is a blocker for phase 1, right? Because, if NOT: What if there was no
such thing as continuations? :) I.e. how about getting rid of
continuations for LayoutNG?

Continuations are not the problem for this particular bug.

Eventually, we will need a coherent story on how to handle

continuations for NG. For now, outlines just use Legacy code.

Aleks

[Koji Ishii]

I'm personally in favor to defer this to post-phase 1. As Morten said, we will need to re-write this when we eliminated continuations, and I don't know what are the use cases for this to work.

Eliminating continuation in phase 1 is another possible option, but again, given being enable to imagine use cases, I think working on perf/memory is more valuable use of our time.

Continuations are not the problem for this particular bug.

I think this is a bit misleading. We have some code to handle continuation, but the code is not enough to handle all types of continuations. This particular case is about continuing to an empty line box. We could say either an empty line box is a problem or a continuation is a problem, but either way, the expected result does not seem very useful to me.

[Aleks Totic]

Kojii, so you think it is ok to ship with this being broken? How do we make these decisions? 

My worry is that outline is used in accessibility, and that this change would make pages less a11y friendly. 

I think we might not mean the same thing when discussing continuations. 

The problem I am wrestling with is that I need to paint something that does not explicitly exist in fragment tree: an outline of children of an Element that did not generate any fragments. 

I’d be dealing with the same problem without continuations. They are only in this conversation because test cases for this bug all generate continuations. ( div inside a span)

Aleks

[Koji Ishii]

2018年10月15日(月) 15:05 Aleks Totic <ato...@google.com>:

Kojii, so you think it is ok to ship with this being broken? How do we make these decisions? 

I think we agree it's important when we find reasonable use cases.

My worry is that outline is used in accessibility, and that this change would make pages less a11y friendly.

I can't imagine how including an empty line box can improve a11y. Can you?

From the git history, this test was added for when continuation outline drawing was implemented in 2007, so it's hard to say it was fixed for a specific use case or just for the completeness.