1687557 - Quirks-mode rendering of some reddit galleries ends up with images much too tall

Status: NEW

(Core :: Layout, defect, P3)

Description

[Daniel Holbert [:dholbert]]

Filing this for:
https://github.com/webcompat/web-bugs/issues/63279 / https://webcompat.com/issues/63279

Reddit's mobile website has a quirks-mode page there, where Firefox handles percent heights differently than Chrome does (and differently from how we handle them in standards mode). And this produces absurdly large images.

Probably some aspect of the percent-height block quirk.

Comment 1

[Jonathan Kew [:jfkthame]]

It would be good to have a simplified testcase here.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Here's another testcase that I'm using locally, with a bit more color & borders to help visualize the various boxes involved.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: reference case 2 (in standards mode)

...and here's a reference case for testcase 2 (identical markup except it's got a doctype to put it in standards-mode).

Comment 5

[Daniel Holbert [:dholbert]]

Here's the DOM we're dealing with in testcase 2:

    <div class="colflex">
      <div class="colitem">
        <div class="tallSibling"></div>
        <div class="slideshowContainer">
          <div class="imageLink">
            <img class="slideImageMainDiv" ...>

• colflex is a column-oriented flex container; imageLink is a row-oriented flex container.
• imageLink and its parent slideshowContainer each have height:100%

So what happens here is:

• colflex uses the flex layout algorithm to resolve a height for its child (colitem, which is a flex item), and calls ReflowFlexItem on it with that height.
• Inside that reflow, we hit the 100% height on slideshowContainer, and I'm pretty sure we don't treat it as quirky since its containing block is a flex item. (per bug 1578586).
  -...but then we hit the 100% height that is one level deeper, on imageLink, and we DO treat that height as quirky, since its containing block is just a regular block.
• So we call CalcQuirkContainingBlockHeight, which digs up and discovers colitem which has a resolved height on its flex item (imposed by its flex container), and we use that to resolve the percentage.

This is a little bit bogus because, strictly speaking, CalcQuirkContainingBlockHeight is only supposed to terminate when it finds a non-auto height; and here, it terminates on a height that happens to have a value that's been filled in but is still indefinite.

Two possible ways to get out of this:
(A) Make CalcQuirkContainingBlockHeight simply refuse to terminate on a flex item (I think that matches oriol's proposed solution in https://github.com/w3c/csswg-drafts/issues/5545 )
(B) Make CalcQuirkContainingBlockHeight check the mTreatBSizeAsIndefinite flag (which will be set in this case), and reject flex item heights if they have that flag set.

(B) is slightly less severe, but we should really probably do (A). I need to test some related scenarios to see how they behave first, though.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (no red should be visible)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1687557: Add check to be sure we recognize indefinite-height flex items, in percent-height quirk's search for definite-height ancestor. r?mats

Some background: the flexbox algorithm always ends up determining an exact
size for flex items for their final reflow (if we don't optimize away that
reflow); and it imposes this size them by setting their ComputedHeight() and
ComputedWidth() in their ReflowInput. However, there are cases where this size
is still required to be treated as indefinite. We track this using a special
flag, "mTreatBSizeAsIndefinite".

This patch makes our Percent-Height Quirk ancestor-traversal code aware of this
flag, so that it doesn't accidentally resolve against a definite-looking height
on some ancestor which is actually supposed to be treated as indefinite-height
(and tagged as such with this flag).

Note: for robustness, this patch also checks that we have a horizontal writing
mode when reasoning about this flag, in order to be sure that this
"BSize"-flavored flag does really correspond to a height (since this quirk only
applies to heights, not to block-sizes in general).

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)

Two possible ways to get out of this:
(A) Make CalcQuirkContainingBlockHeight simply refuse to terminate on a flex item (I think that matches oriol's proposed solution in https://github.com/w3c/csswg-drafts/issues/5545 )
(B) Make CalcQuirkContainingBlockHeight check the mTreatBSizeAsIndefinite flag (which will be set in this case), and reject flex item heights if they have that flag set.

(B) is slightly less severe, but we should really probably do (A). I need to test some related scenarios to see how they behave first, though.

I opted for (B) since it seems (A) has some webcompat risk for cases with definite-height flex items, and feels a bit over-broad in any case.

I included a WPT test (numbered -004 in the attached patch) that passes in Firefox and Chromium currently, which approach (A) would cause to fail if we were to take approach (A).

Comment 9

[Mats Palmgren (inactive)]

Attachment: testcase 4

This is percentage-height-quirk-excludes-flex-grid-003.html but with a vertical-lr flex container (row layout) with a horizontal-tb item. Why do we display this differently than Chrome?

Comment 10

[Mats Palmgren (inactive)]

Attachment: testcase 5

Similar, but with flex-direction: column and vertical-lr on all elements - again we differ from Chrome.

Comment 11

[Mats Palmgren (inactive)]

Attachment: testcase 6

Same as testcase 5, but with row layout - again we differ from Chrome.

Comment 12

[Mats Palmgren (inactive)]

I wonder if the differences are rooted from a misreading of the spec (on our part, or their, idk).
https://quirks.spec.whatwg.org/#the-percentage-height-calculation-quirk
The spec describes an algorithm using the word "height" which is typically read as a physical axis. But then the last sentence says:

This quirk needs to take writing modes into account.

That can be read in two ways:

1. it qualifies the "height" in the preceding to mean "logical height", or
2. it only clarifies that the algorithm should work (and use the physical height) also when various intermediary boxes have different writing-modes

Judging from our code (and your changes here), it seems we read it as 2. Looking at the Chrome results for testcase 4 - 6, I wonder if they read it as 1.

Comment 13

[Mats Palmgren (inactive)]

Attachment: testcase 7

same as 6, but with an indefinite block-size on the flex container

Comment 14

[Daniel Holbert [:dholbert]]

For testcases 5-7, it looks like Chrome is perhaps applying this "percent-height resolution quirk" in the block-axis rather than in the height axis. That's a bit silly of them, IMO, since the quirk predates writing-modes and is traditionally height-specific. Though, maybe it's simpler on their end to implement it in a block-axis-specific way -- not sure.

In testcase 4, I'm not 100% sure what's going on, but I can take closer look. In the meantime, though, I feel like the attached patch is a fairly targeted & self-contained fix to what is clearly an oversight (a case where we're misinterpreting a particular flavor of auto height), with real in-the-wild implications & compat requirements -- so I'd prefer to keep this bug/patch scoped to that specific change, if that's all right with you as well, and I can spin off additional bugs/patches as-needed for other testcases.

What do you think?

Comment 15

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #14)

For testcases 5-7, it looks like Chrome is perhaps applying this "percent-height resolution quirk" in the block-axis rather than in the height axis.

Right, so my understanding of the intention of this quirk is that it's a way to make percentages do something in the block-axis (when its CB size is indefinite). I think it's rooted in the fact that the CB inline-size is always definite at reflow time, whereas the CB block-size isn't. So it's not physical height, but rather logical height. I suspect the reason the spec still says "height" is simply that no one has updated it to account for writing-modes. (WHATWG specs in general aren't very precise when they talk about CSS properties like these. I've seen width/height being used to mean inline-size/block-size before).

I always thought that the reason we still use the physical height in our code is that it's a left-over from our incomplete transition from physical to logical coordinates in layout code. IOW, it's just a bug when vertical writing-modes are involved.

In any case, could you open a spec issue on this to ask for clarification? (if the spec really intends "physical height" then it should probably say so explicitly to avoid confusion). It might be worth CC-ing some CSS old-timers that were around in CSS2 times that might know the origin/history of this quirk, like fantasai / TabAtkins / dbaron maybe?

Comment 16

[Mats Palmgren (inactive)]

An alternative interpretation is that the quirk should not apply when vertical writing-modes are involved. But that leaves us in a worse place IMO since then we need to define what should happen when that occurs, which leads to a more complicated spec/implementation.

Comment 17

[Karl Dubost💡 :karlcow]

Probably we could contact reddit on the partner mailing-list.

Comment 18

[Karl Dubost💡 :karlcow]

Email sent to reddit ML.

Comment 19

[Simon Pieters [:zcorpan]]

The quirks spec should indeed use block-size rather than height, it says (though the "issue" styling is no longer present):

This quirk needs to take writing modes into account.

For flex and grid items, I see that was discussed in https://github.com/w3c/csswg-drafts/issues/5545#issuecomment-741938953

Comment 20

[Karl Dubost💡 :karlcow]

It seems reddit has fixed it.
The issue still exists in Gecko.

Comment 21

[Ksenia Berezina [:ksenia]]

Unsetting webcompat priority for now as the issue with reddit galleries is no longer present (based on comment 20).