I think you're mixing two things; culled inline boxes and empty line boxes.

The former isn't a problem for both of issues you listed, unless there're more bugs than I know of.

Empty line box in these scenario is problematic, but always generating fragments does not solve it. There are several cases we must suppress fragments to render correctly, generating fragments in those cases will break rendering.

So I guess your reasoning and your proposal do not match.

This CL prevents generating fragments within an empty line box:
https://chromium-review.googlesource.com/c/chromium/src/+/1098925/

As in the discussion in the CL, if were to generate such fragments, we need another solution to the problem the CL solves.

+mstensho

To explain the difference by example:

1. <div>a<span position:relative><div position:absolute></div></span></div>
We generate a box fragment for the <span position:relative> today.

2. <div><span position:relative><div position:absolute></div></span></div>
We don't, because this outer <div> is an empty line box. This is the case #3 fixed. Generating box can break rendering in some other cases.

I agree there should be a way to solve it differently, that satisfies both needs, but we haven't come up yet.

Are we in the same page now that the problem you're facing is not about generating fragments for the culled inline boxes?

Phew, this is a complicated issue, and I am glad we have a bug for discussing it.

I do not know inline algorithm details well enough to figure out what the 
solution is. Explanation of differences in #4 was very helpful.

What I do know well is parts of the code that suffer because some
LayoutInlines do not generate fragments. These problem areas are:

#1) Outline painting (#889721)

Ex: <span.outline><div></div></span>
Outlines are painted by the first fragment of outlined Element. 
If Element does not generate fragments, we do not paint the outline, 
which is wrong.

Can this be fixed without generating fragments? 

The only way I can think of is this:
- Paint algorithm cannot depend on finding outlines during fragment tree traversal.
- Paint algorithm can find Elements (not fragments!) with outlines during pre-paint walk.
- These elements can be painted in kOutline phase without doing tree traversal.
- Problem: if we are not doing tree traversal, how do we guarantee correct
  order of outline painting? Also, atomic inlines get painted with kOutline outside
  of PaintLayer's kOutline phase. How to handle that?

#2) Computing abspos inline containing block position.(#903578)

Ex: <span.relative><div.absolute></div></span>

When inline is an abspos containing block, we need inline's location to compute
containing block position.

Can this be fixed without generating fragments?

Currently, NGOutOfFlowPositionedDescendant only stores its static position, 
and inline container. 
Descendant could also store position of inline container.
This position could be computed when we discover inline container inside
inline layout algorithm, before fragment is discarded.
The position would have to be propagated just like static position.
Problem: when inline Element spans multiple lines, its full position is not known
to inline algorithm right away. Because of this, we cannot assign full position
right away.

#3) Hit testing

I am not an expert here, @xiaocheng is. 

According to @xiaocheng, "We handle this, but it is hacky". While traversing
fragment tree, hit testing queries Legacy tree. Something like this happens:

HitTest(paint_fragment) {
  parent_fragment = paint_fragment.parent();
  if (parent_fragment.GetLayoutObject() != paint_fragment.GetLayoutObject().parent())
    // do special traversal of Elements + fragments
  else
    // traverse fragments.
}

"#3) Hit testing" is about "culled inline box", that was solved. The problem we're talking about here is "empty line box", correct?

The conflicts for the "empty line box" problem are:
1. Empty line boxes cannot generate child fragments for web compat.
2. OOF requires a LayoutObject that generates a fragment, even within an empty line box.

Generating children of empty line will break 43 tests as in:
  https://chromium-review.googlesource.com/c/chromium/src/+/1251142
We can go through this list to investigate why, and add special case code to avoid the breakage. This is one option.

Another way is to use NGPhysicalLineBoxFragment as a fallback when the line box is empty, but as stated above, NG OOF algorithm needs a LayoutObject, and NGPhysicalLineBoxFragment doesn't have a LayoutObject.

atotic@, how much easy/hard it is to not to look for the ancestor chain of LayoutObject? I'm not familiar enough with OOF code to know how easy/hard this is, but I hope we could adopt to propagate pattern to the algorithm?

There maybe more options but I haven't come up yet.

Another option: add a new flag to box fragment, or new type of a fragment, which is fully ignored during layout/paint regardless of the style it has, but act only as a container for OOF. If the fragment doesn't affect anything other than OOF, we can probably generate such one without breaking 43 tests.

Created some tests:
https://jsbin.com/hawerux/edit?html,output

* When the line box is empty, inline boxes should not have strut for Edge/Blink.
  Gecko has strut.
* Empty line box should be placed at the bottom of collapsed margins for Edge/Blink.
  Gecko does somewhat differently, can't figure the rule out.
* Blink (legacy) is broken when line box is not empty (?)
* NG positions at left even when the left left of empty line box is not at left of the block.

So probably what is expected, assuming we match to Edge/Blink, is either:
a. Fallback to the linebox.
b. Create a special box that always has 0x0 size, ignoring strut.

outline assumes that the empty line box has available width, so we will need a separate fix for outline.

Responses to #7:
> Hit testing" is about "culled inline box", that was solved. The problem we're 
> talking about here is "empty line box", correct?

Can you explain (or point me to docs) for the difference between 
"culled inline" and "empty line box"?
My knowledge of inline layout algorithm is limited.

> atotic@, how much easy/hard it is to not to look for the ancestor 
> chain of LayoutObject? I'm not familiar enough with OOF code to know
> how easy/hard this is, but I hope we could adopt to propagate pattern to the algorithm?

OOF needs dimensions of container's LayoutObject.
If container is empty, dimensions are not available from Parent(), 
or anywhere in fragment tree/layout tree. Or is there some secret
spot I am not aware of?

Can we store this information anywhere before we decide not to
generate the fragment? Another option is to pass dimensions to
 NGContainerFragmentBuilder& AddInlineOutOfFlowChildCandidate and propagate
it together with static_position. Painful, but doable.

Fixing outline is trickier: 
- PaintLayer will not call Paint(PaintPhase::kOutline) unless
it has at least one outline fragment.
- We paint by traversing the NGPaintFragment tree. If the fragment
with outline is not there, it does not get painted.

> #8: Another option: add a new flag to box fragment, or new type of a fragment, 
> which is fully ignored during layout/paint

This would be simplest for OOF/Outline.

> #11 outline assumes that the empty line box has available width,
> so we will need a separate fix for outline.

I am unclear why this is. Why does outline assume available width
for empty line box? I think I could make NullFragment work for outline
painting.

> Can you explain (or point me to docs) for the difference between "culled inline" and "empty line box"?

"Culled inline" is an optimization by omitting box fragments when it's not necessary. NGInlineItem::ComputeBoxProperties() computes whether we create a box or cull. CanContainAbsolutePositionObjects() is included, so abspos is not affected by this.

"empty line box" is what CSS2 defines as "certain zero-height line boxes"
https://drafts.csswg.org/css2/box.html#collapsing-margins
Normally, an empty span <span></span> has the height of its font, and will never be zero-size except in quirks mode. This "certain zero-height line boxes" is an exception, it must be considered as not exist. It paints nothing, and bottom margin of previous box and top margin of next box must collapse.

Examples:
  <div>a<span></span></div>
This is a normal line box because it has text "a". The <span></span> has a height of its font.

  <div><span></span></div>
This is an empty line box. The size of <span></span> is not well-defined, but the line box must be zero-height, and margins must collapse across this <div>.

  <div><span></span><span style="border-left: 1px solid blue"></span></div>
This is a normal line box, because there's a border in inline-direction, and thus the empty span has height.

  <div><span></span><span style="border-top: 1px solid blue"></span></div>
This is an empty line box, because it only has top border.

During the layout, we create a box fragment for <span></span>, but we need to change it to zero-size if the line box is "empty". Morten's patch removes them instead to make the line box zero-size. If we need fragments for abspos within "empty line boxes", we'll need to create a special fragment that has zero-size for this purpose, and ensure the line box is still considered as "empty line box" in other parts of the layout algorithm. IIRC we have a few places to check "if height is zero and no children" then it's an empty line box, so we probably need to change that too.

Hit test needs to handle culled inline boxes, but has nothing to do with empty line box, because empty line box never needs to hit.

> Why does outline assume available width for empty line box? I think I could make NullFragment work for outline painting.

When I checked test cases in the other issue, the outline included the width of the containing block when the line box is empty. Well, we need to try, but I believe we'll need separate fix because expected behavior is different.

I can take a look for both options, but currently I have a few big ones, probably in 3-4 weeks.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24615339dcd4d5bf4ffad3ceb1a5b88c526c8b0f

commit 24615339dcd4d5bf4ffad3ceb1a5b88c526c8b0f
Author: Koji Ishii <kojii@chromium.org>
Date: Thu Nov 15 17:44:05 2018

[LayoutNG] Add NGPhysicalLineBoxFragment::IsEmptyLineBox()

This patch adds IsEmptyLineBox() function that returns
whethe the line box is "empty", or "certain zero-height
line box" as defined in CSS2[1] or not, and change where
we used to determine it by checking if the number of children
is 0.

[1] https://drafts.csswg.org/css2/visuren.html#phantom-line-box

Bug: 636993,  903578 
Change-Id: I2b9961e1ecb743063238b39a3a5be53f15ffce8c
Reviewed-on: https://chromium-review.googlesource.com/c/1332972
Reviewed-by: Aleks Totic <atotic@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608419}
[modify] https://crrev.com/24615339dcd4d5bf4ffad3ceb1a5b88c526c8b0f/third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm.cc
[modify] https://crrev.com/24615339dcd4d5bf4ffad3ceb1a5b88c526c8b0f/third_party/blink/renderer/core/layout/ng/inline/ng_line_box_fragment_builder.cc
[modify] https://crrev.com/24615339dcd4d5bf4ffad3ceb1a5b88c526c8b0f/third_party/blink/renderer/core/layout/ng/inline/ng_line_box_fragment_builder.h
[modify] https://crrev.com/24615339dcd4d5bf4ffad3ceb1a5b88c526c8b0f/third_party/blink/renderer/core/layout/ng/inline/ng_physical_line_box_fragment.cc
[modify] https://crrev.com/24615339dcd4d5bf4ffad3ceb1a5b88c526c8b0f/third_party/blink/renderer/core/layout/ng/inline/ng_physical_line_box_fragment.h
[modify] https://crrev.com/24615339dcd4d5bf4ffad3ceb1a5b88c526c8b0f/third_party/blink/renderer/core/layout/ng/list/ng_unpositioned_list_marker.cc
[modify] https://crrev.com/24615339dcd4d5bf4ffad3ceb1a5b88c526c8b0f/third_party/blink/renderer/core/layout/ng/ng_block_layout_algorithm.cc
[modify] https://crrev.com/24615339dcd4d5bf4ffad3ceb1a5b88c526c8b0f/third_party/blink/renderer/core/layout/ng/ng_physical_fragment.cc
[modify] https://crrev.com/24615339dcd4d5bf4ffad3ceb1a5b88c526c8b0f/third_party/blink/renderer/core/layout/ng/ng_physical_fragment.h

The WIP https://chromium-review.googlesource.com/c/chromium/src/+/1251142/8
is causing 8 failures. We probably need to adjust outline and abspos code.

Found one problem in abspos code, not related with this but one of tests hit it by the change. Probably similar problems in outline code too, not sure how many more I need to fix to pass tests atm.

Thanks for looking into this. This looks much better than the last attempt. Will take a look on Monday.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed2f02d8634e5059d5494429ae976fca08295a43

commit ed2f02d8634e5059d5494429ae976fca08295a43
Author: Koji Ishii <kojii@chromium.org>
Date: Tue Nov 27 02:08:13 2018

[LayoutNG] Fix inline out-of-flow container computation

The out-of-flow code assumed that all fragments for a
LayoutInline has the same height, but the change for
 crbug.com/903578  (WIP CL:1251142) breaks the assumption.

This patch fixes by computing the rect and uniting them,
instead of taking the max of heights.

No behavior changes until the WIP CL lands.

Change-Id: I9fe6b35d25836fde10dd88dd90ca2a810f5c9863
Bug: 636993,  903578 
Reviewed-on: https://chromium-review.googlesource.com/c/1351189
Reviewed-by: Aleks Totic <atotic@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610994}
[modify] https://crrev.com/ed2f02d8634e5059d5494429ae976fca08295a43/third_party/blink/renderer/core/layout/ng/ng_box_fragment_builder.cc
[modify] https://crrev.com/ed2f02d8634e5059d5494429ae976fca08295a43/third_party/blink/renderer/core/layout/ng/ng_box_fragment_builder.h
[modify] https://crrev.com/ed2f02d8634e5059d5494429ae976fca08295a43/third_party/blink/renderer/core/layout/ng/ng_out_of_flow_layout_part.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/744ec324aaecc33cdb0090a259aa2d0cafc1d302

commit 744ec324aaecc33cdb0090a259aa2d0cafc1d302
Author: Koji Ishii <kojii@chromium.org>
Date: Thu Nov 29 08:57:10 2018

[LayoutNG] Create fragments in "empty" line boxes

The "empty" line box is what CSS defines as "certain zero-
height line box", that suffices certain conditions as defined
in the spec and is ignored for margin collapsing.

It has some special behaviors, such as to ignore strut, that
LayoutNG suppressed to generate their child fragments.
However, we need them under certain conditions such as when
they have out-of-flow container as descendants.

This change breaks some normal flow, out-of-flow, and outline
tests.

Normal flow failures were fixed by adding an explicit
|IsEmptyLineBox()| function.

Out-of-flow failures are fixed as a separate CL in r610994
(CL:1351189).

Outline failures are fixed in this CL and in r611954
(CL:13543199) thanks to atotic@.

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I4d393af016439cdbe085d91fdd1e8550d320d1f1
Bug: 636993,  903578 
Reviewed-on: https://chromium-review.googlesource.com/c/1251142
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Aleks Totic <atotic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612108}
[modify] https://crrev.com/744ec324aaecc33cdb0090a259aa2d0cafc1d302/third_party/blink/renderer/core/layout/ng/inline/ng_inline_box_state.cc
[modify] https://crrev.com/744ec324aaecc33cdb0090a259aa2d0cafc1d302/third_party/blink/renderer/core/layout/ng/inline/ng_inline_box_state.h
[modify] https://crrev.com/744ec324aaecc33cdb0090a259aa2d0cafc1d302/third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm.cc
[modify] https://crrev.com/744ec324aaecc33cdb0090a259aa2d0cafc1d302/third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm_test.cc
[modify] https://crrev.com/744ec324aaecc33cdb0090a259aa2d0cafc1d302/third_party/blink/renderer/core/layout/ng/inline/ng_line_box_fragment_builder.cc
[modify] https://crrev.com/744ec324aaecc33cdb0090a259aa2d0cafc1d302/third_party/blink/renderer/core/layout/ng/ng_physical_box_fragment.cc
[modify] https://crrev.com/744ec324aaecc33cdb0090a259aa2d0cafc1d302/third_party/blink/web_tests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/744ec324aaecc33cdb0090a259aa2d0cafc1d302/third_party/blink/web_tests/flag-specific/enable-blink-features=LayoutNG/fast/css/ignore-empty-focus-ring-rects-expected.png
[modify] https://crrev.com/744ec324aaecc33cdb0090a259aa2d0cafc1d302/third_party/blink/web_tests/flag-specific/enable-blink-features=LayoutNG/paint/invalidation/outline/focus-ring-on-inline-continuation-move-expected.txt