WICG / layout-instability

A proposal for a Layout Instability specification
https://wicg.github.io/layout-instability/
Other
159 stars 27 forks source link

layout instability spec doesn't account for paint-only effects that don't influence css box size (e.g. box-shadow) #65

Closed dholbert closed 4 years ago

dholbert commented 4 years ago

tl;dr: Chrome seems to be considering box-shadow as something that contributes to the Visual Representation of a node, which seems superficially reasonable but disagrees with the spec's definition of Visual Representation. So this is a case where Chrome & the spec disagree. Not sure whether it's a spec bug vs. Chrome bug. --> Starting out as a spec bug.

STR:

  1. Load this testcase in Chrome, with the devtools console open (F12): https://jsfiddle.net/dholbert/6rw8oqsf/
  2. Make a note of the logged layout-shift value.
  3. Edit the testcase to remove the box-shadow declaration, and re-run the testcase, and look at the logged layout-shift value.

ACTUAL RESULTS: The layout shift value changes. EXPECTED RESULTS: Per the current spec, the layout shift value should not change.

NOTES: box-shadow is a paint effect which is defined as not influencing layout. Quoting the spec that defines box-shadow:

Shadows do not influence layout [...] Shadows do not trigger scrolling or increase the size of the scrollable area.

https://drafts.csswg.org/css-backgrounds-3/#shadow-layers

...while layout-shift is defined in terms of an element's "boxes" (from the box tree): https://wicg.github.io/layout-instability/#visual-representation

Shadows attach themselves to boxes, but they do not increase the size of the boxes. Shadows do increase the size of the "ink overflow" region -- one possible resolution here would be to redefine the Visual Representation in terms of the Ink Overflow. That would be a somewhat substantial change, though.

dholbert commented 4 years ago

CC @skobes @mattwoodrow @npm1

dholbert commented 4 years ago

Here's a related slightly-different scenario:

Testcase: https://jsfiddle.net/dholbert/5u0depzk/ (This^ testcase triggers a console.log() for a layout shift. If you remove the box-shadow styling, then it does not trigger a console.log for a layout shift anymore.)

npm1 commented 4 years ago

Thanks for the detailed feedback! At first glance, this looks to me like it's a Chrome implementation bug. It's not clear to me if this would be easy to fix. I do wonder: do you think this falls under the category of things where the user agent may be slightly inaccurate if it would be expensive performance-wise to fix?

dholbert commented 4 years ago

I think it falls under the category of "things that fall naturally out of the engine-specific paint pipeline, which are difficult to avoid, and also probably difficult/undesirable to specify precisely". :)

If this is difficult to fix, that suggests to me that Chrome's implementation is in terms of paint primitives (like ink overflow region) rather than layout primitives (like the css box tree). Do you know if that's the case? (And/or: would you be able to sum up Chrome's implementation approach somewhere, and/or is that documented somewhere that I could look at?)

This sort of distinction (box-shadow included/excluded) seems superficially trivial but matters a great deal for how we might approach a Gecko implementation -- where we plug into our relayout/render pipeline, which box tree / primitives we're dealing with, etc.

npm1 commented 4 years ago

BTW I was just voicing my thoughts here :) I will let @skobes comment on difficulty of the fix (or if this is WAI and we need to fix the spec).

skobes-chromium commented 4 years ago

I think Chrome may have fixed this recently (crbug.com/1108622).

npm1 commented 4 years ago

Oh yes that's right, both testcases now seem to behave properly (tested on Chrome Canary). Hence I think we can close this issue, but let me know if there's anything pending!