Bug 246060 - threejs.org renders mostly blank (lazy image loading inside 0-height inline is broken?)
Summary: threejs.org renders mostly blank (lazy image loading inside 0-height inline i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari Technology Preview
Hardware: All All
: P1 Critical
Assignee: Rob Buis
URL: https://threejs.org
Keywords: BrowserCompat, InRadar, NeedsReduction
Depends on:
Blocks:
 
Reported: 2022-10-04 19:03 PDT by Karl Dubost
Modified: 2022-12-08 07:37 PST (History)
13 users (show)

See Also:


Attachments
Patch (1.52 KB, patch)
2022-10-11 07:26 PDT, Rob Buis
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Reduced test case (328 bytes, text/html)
2022-11-09 03:02 PST, Rob Buis
no flags Details
Patch (4.33 KB, patch)
2022-12-07 21:33 PST, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Karl Dubost 2022-10-04 19:03:42 PDT
Steps to reproduce:
1. With Safari 16.1 (18614.2.6.1.1) on macOS
2. Go to https://threejs.org

Expected:
The right side of the screen (`#projects`) is full of square images

Actual:
The area is empty. 
`#projects` has a 0px height. Images have not been loaded. 


Each images are 

```
<a href="https://eyes.nasa.gov/apps/mars2020/" target="_blank" rel="noopener"><img src="files/projects/mars2020.png" loading="lazy"></a>
```

if I toggle off and on:

```
#projects a {
overflow: hidden;
}
```

or 

```
#projects {
line-height: 0;
}
```

The images are being downloaded and the grid appears.

This is working as expected in Firefox Nightly 107 and Chrome Canary 108.

also on rdar://99905638
Comment 1 Rob Buis 2022-10-05 00:12:22 PDT
I am not sure if this is a regression, I see the same behaviour on 15.4.
(I am back next week so can take a look then).
Comment 2 Alexey Proskuryakov 2022-10-06 13:05:53 PDT
I can also reproduce with older Safari, 15.6.1 on macOS Monterey.
Comment 3 Rob Buis 2022-10-11 07:26:01 PDT
Created attachment 462924 [details]
Patch
Comment 4 Rob Buis 2022-10-11 11:58:05 PDT
The problem seems to be that intersection observer is not triggering the images as intersecting the root, so no lazy loads are triggered. This seems to depend on the line-height in combination with overflow: hidden; on my test line-height <= 0.5 causes images to register as non intersecting, bigger values do register as intersecting and thus trigger the lazy loads.
Comment 5 Rob Buis 2022-10-11 12:02:51 PDT
One possible cause, take this snippet:

<div style="display:grid; grid-template-columns: repeat(7, 1fr);line-height:0">
  <div style="overflow:hidden"><img/></div>
</div>

In WebKit the inner div as well as the grid div get assigned height of 0, whereas in Firefox/Chrome they get 4/5 pixels instead. The larger area obviously has more of a chance of causing an intersection.
Comment 6 Simon Fraser (smfr) 2022-10-11 13:07:23 PDT
Ignoring grid layout behavior, how does lazy-load of an image inside a height:0 container ever work?
Comment 7 Rob Buis 2022-10-11 14:04:19 PDT
(In reply to Simon Fraser (smfr) from comment #6)
> Ignoring grid layout behavior, how does lazy-load of an image inside a
> height:0 container ever work?

For example:
  <div style="width:0px;height:0px">
    <img loading="lazy" src="https://threejs.org/files/projects/junni.png"/>
  </div>

Here the 0x0 image inside the 0x0 container still intersects.
Comment 8 Simon Fraser (smfr) 2022-10-11 14:17:18 PDT
Does this work because of edge-inclusive intersection?
Comment 9 Rob Buis 2022-10-11 14:30:48 PDT
(In reply to Simon Fraser (smfr) from comment #8)
> Does this work because of edge-inclusive intersection?

Yes, VisibleRectContextOption::UseEdgeInclusiveIntersection is used in Document.cpp:8169 as well as in computeClippedRectInRootContentsSpace and Document.cpp:8177 uses edgeInclusiveIntersect.
Comment 10 Rob Buis 2022-10-26 14:49:01 PDT
The lazy loading starts to work when IFC internal feature is disabled.
Comment 11 Rob Buis 2022-11-09 03:02:22 PST
Created attachment 463466 [details]
Reduced test case

Reduced test case.
Comment 12 Rob Buis 2022-11-09 03:05:49 PST
(In reply to Rob Buis from comment #10)
> The lazy loading starts to work when IFC internal feature is disabled.

The first grid layout for the reduced test case with IFC enabled is:

(B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, hasLayer(S)crollableArea, (C)omposited, (+)Dirty style, (+)Dirty layout
B---YGLSC -+  RenderView at (0,0) size 1728x0 renderer (0x1440008d0) layout box (0x0) layout->[normal child]
B-----LS- -+    HTML RenderBlock at (0,0) size 1728x0 renderer (0x144001920) layout box (0x0) node (0x144001240) layout->[self][normal child]
B-------- -+      BODY RenderBody at (0,8) size 1712x0 renderer (0x144001ba0) layout box (0x0) node (0x144001480) layout->[self][normal child]
B-------- --*       DIV RenderGrid at (0,0) size 1712x0 renderer (0x144004760) layout box (0x0) node (0x144004200)
B--O--LS- --          A RenderBlock at (0,0) size 1712x0 renderer (0x144004ab0) layout box (0x144004e00) node (0x144004400)
-------- --            line at (0.00,0.00) size (1712.00x0.00) baseline (5.00) enclosing top (5.00) bottom (5.00)
-------- --              Root inline box at (0.00,-9.00) size (0.00x18.00)
-------- --              Run(s):
-------- --                Atomic box at (0.00,5.00) size 0.00x0.00 renderer->(0x144004c00)
I-------- --            IMG RenderImage at (0,5) size 0x0 renderer (0x144004c00) layout box (0x144004ed0) node (0x144004540)

With IFC disabled:
(B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y, (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer, hasLayer(S)crollableArea, (C)omposited, (+)Dirty style, (+)Dirty layout
B---YGLSC -+  RenderView at (0,0) size 1728x0 renderer (0x1440008d0) layout box (0x0) layout->[normal child]
B-----LS- -+    HTML RenderBlock at (0,0) size 1728x0 renderer (0x144001920) layout box (0x0) node (0x144001240) layout->[self][normal child]
B-------- -+      BODY RenderBody at (0,8) size 1712x0 renderer (0x144001ba0) layout box (0x0) node (0x144001480) layout->[self][normal child]
B-------- --*       DIV RenderGrid at (0,0) size 1712x5 renderer (0x144004760) layout box (0x0) node (0x144004200)
B--O--LS- --          A RenderBlock at (0,0) size 1712x5 renderer (0x144004ab0) layout box (0x0) node (0x144004400)
-------- --            Line: (top: -9 bottom: 9) with leading (top: 0 bottom: 5)
-------- --            RootInlineBox at (0,-9) size 0x18 (0x144007f20) renderer->(0x144004ab0)
-------- --              InlineBox at (0,5) size 0x0 (0x144007e80) renderer->(0x144004c00)
I-------- --            IMG RenderImage at (0,5) size 0x0 renderer (0x144004c00) layout box (0x0) node (0x144004540)

Only in the second situation is the lazy load processed. I think we have to find out first if the changed render tree with IFC is correct.

Note that FF and Chrome show the image when loading the reduced test case.
Comment 13 zalan 2022-12-06 16:07:04 PST
(In reply to Rob Buis from comment #12)
> (In reply to Rob Buis from comment #10)
> > The lazy loading starts to work when IFC internal feature is disabled.
> 
> The first grid layout for the reduced test case with IFC enabled is:
> 
> (B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y,
> (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer,
> hasLayer(S)crollableArea, (C)omposited, (+)Dirty style, (+)Dirty layout
> B---YGLSC -+  RenderView at (0,0) size 1728x0 renderer (0x1440008d0) layout
> box (0x0) layout->[normal child]
> B-----LS- -+    HTML RenderBlock at (0,0) size 1728x0 renderer (0x144001920)
> layout box (0x0) node (0x144001240) layout->[self][normal child]
> B-------- -+      BODY RenderBody at (0,8) size 1712x0 renderer
> (0x144001ba0) layout box (0x0) node (0x144001480) layout->[self][normal
> child]
> B-------- --*       DIV RenderGrid at (0,0) size 1712x0 renderer
> (0x144004760) layout box (0x0) node (0x144004200)
> B--O--LS- --          A RenderBlock at (0,0) size 1712x0 renderer
> (0x144004ab0) layout box (0x144004e00) node (0x144004400)
> -------- --            line at (0.00,0.00) size (1712.00x0.00) baseline
> (5.00) enclosing top (5.00) bottom (5.00)
> -------- --              Root inline box at (0.00,-9.00) size (0.00x18.00)
> -------- --              Run(s):
> -------- --                Atomic box at (0.00,5.00) size 0.00x0.00
> renderer->(0x144004c00)
> I-------- --            IMG RenderImage at (0,5) size 0x0 renderer
> (0x144004c00) layout box (0x144004ed0) node (0x144004540)
> 
> With IFC disabled:
> (B)lock/(I)nline/I(N)line-block, (A)bsolute/Fi(X)ed/(R)elative/Stic(K)y,
> (F)loating, (O)verflow clip, Anon(Y)mous, (G)enerated, has(L)ayer,
> hasLayer(S)crollableArea, (C)omposited, (+)Dirty style, (+)Dirty layout
> B---YGLSC -+  RenderView at (0,0) size 1728x0 renderer (0x1440008d0) layout
> box (0x0) layout->[normal child]
> B-----LS- -+    HTML RenderBlock at (0,0) size 1728x0 renderer (0x144001920)
> layout box (0x0) node (0x144001240) layout->[self][normal child]
> B-------- -+      BODY RenderBody at (0,8) size 1712x0 renderer
> (0x144001ba0) layout box (0x0) node (0x144001480) layout->[self][normal
> child]
> B-------- --*       DIV RenderGrid at (0,0) size 1712x5 renderer
> (0x144004760) layout box (0x0) node (0x144004200)
> B--O--LS- --          A RenderBlock at (0,0) size 1712x5 renderer
> (0x144004ab0) layout box (0x0) node (0x144004400)
> -------- --            Line: (top: -9 bottom: 9) with leading (top: 0
> bottom: 5)
> -------- --            RootInlineBox at (0,-9) size 0x18 (0x144007f20)
> renderer->(0x144004ab0)
> -------- --              InlineBox at (0,5) size 0x0 (0x144007e80)
> renderer->(0x144004c00)
> I-------- --            IMG RenderImage at (0,5) size 0x0 renderer
> (0x144004c00) layout box (0x0) node (0x144004540)
> 
> Only in the second situation is the lazy load processed. I think we have to
> find out first if the changed render tree with IFC is correct.
> 
> Note that FF and Chrome show the image when loading the reduced test case.
It looks like legacy constructs a 5px tall line for the _empty_ content.
Comment 14 Rob Buis 2022-12-07 07:30:44 PST
(In reply to zalan from comment #13)
> > Only in the second situation is the lazy load processed. I think we have to
> > find out first if the changed render tree with IFC is correct.
> > 
> > Note that FF and Chrome show the image when loading the reduced test case.
> It looks like legacy constructs a 5px tall line for the _empty_ content.

Good point. I tried this test to mimic the situation:
<!DOCTYPE html>
<meta name="viewport" content="width=device-width,initial-scale=1">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="./resources/intersection-observer-test-utils.js"></script>

<style>
pre, #log {
  position: absolute;
  top: 0;
  left: 200px;
}
#container {
  display: grid;
  height: 0px;
}
</style>

<div id="container">
  <div src="about:blank" style="overflow:hidden">
    <img src="/css/support/60x60-red.png" id='target'></img>
  </div>
</div>

<script>
var entries = [];

runTestCycle(function() {
  var target = document.getElementById('target');
  assert_true(!!target, "target exists");
  var observer = new IntersectionObserver(function(changes) {
    entries = entries.concat(changes)
  });
  observer.observe(target);
  entries = entries.concat(observer.takeRecords());
  assert_equals(entries.length, 0, "No initial notifications.");
  runTestCycle(step0, "First rAF should generate a notification.");
}, "Ensure that a target intersecting a zero-height container generates a notification with intersectionRatio == 0");

function step0() {
  assert_equals(entries.length, 1, "One notification.");
  assert_equals(entries[0].intersectionRatio, 0, "intersectionRatio == 0");
}
</script>

So, given a zero-height container and parent with overflow:hidden, the inner image is expected to not intersect (regardless whether it is lazy loading) by all implementations (all PASS).

With that in mind, with the IFC behaviour the intersection is not supposed to happen so lazy loading will not be triggered (see test case above), with legacy behaviour the intersection is supposed to happen so lazy loading will be triggered.

If IFC behaviour here is what we want/what is specced then the behaviour in this reported bug is as expected.

A workaround I suspect may work in this case is removing the overflow: hidden.
Comment 15 zalan 2022-12-07 21:10:24 PST
Actually the line box is not empty as we are in standards mode here (DOCTYPE). Standards mode puts an invisible strut in every root inline box making the line box contentful. (I should have taken a look at the content and not just the tree dump)
Comment 16 zalan 2022-12-07 21:33:39 PST
Created attachment 463934 [details]
Patch
Comment 17 Rob Buis 2022-12-08 06:45:16 PST
(In reply to zalan from comment #15)
> Actually the line box is not empty as we are in standards mode here
> (DOCTYPE). Standards mode puts an invisible strut in every root inline box
> making the line box contentful. (I should have taken a look at the content
> and not just the tree dump)

I did not expect that, I was thinking maybe some minimum height was being used internally. Maybe a link to the relevant spec can be put in the commit log?
Comment 18 EWS 2022-12-08 07:30:41 PST
Committed 257565@main (7e8fe9be9b22): <https://commits.webkit.org/257565@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 463934 [details].
Comment 19 zalan 2022-12-08 07:37:56 PST
(In reply to Rob Buis from comment #17)
> (In reply to zalan from comment #15)
> > Actually the line box is not empty as we are in standards mode here
> > (DOCTYPE). Standards mode puts an invisible strut in every root inline box
> > making the line box contentful. (I should have taken a look at the content
> > and not just the tree dump)
> 
> I did not expect that, I was thinking maybe some minimum height was being
> used internally. Maybe a link to the relevant spec can be put in the commit
> log?
You are right. I assumed it was clear from the commit message that this was about inline layout (and there's only one inline layout spec).
Thanks for looking into it btw.