Remove "table-caption" special case from dynamic reflow root

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P3
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

6 months ago
Right now, InitDynamicReflowRoot has a special case with the broad explanation:
>    // We have a display type where 'width' and 'height' don't actually
>    // set the width or height (i.e., the size depends on content).
...which includes a check for table-caption frames:
> [...] || display == StyleDisplay::TableCaption ||
https://searchfox.org/mozilla-central/rev/bcc0fe1081dc4f84fc5fdfebe0d2de8c075a4e2c/layout/generic/ReflowInput.cpp#825,828-829

However, from some quick testing, it appears to me that table-caption frames *do* respect their width and height properties (when they are set to fixed values).  If you have a caption with e.g.
  height: 5px;
  width:  5px;
  background: red;

...then its orange box is 5px by 5px, even if its content happens to be larger, and even if it happens to be in a table that happens to be larger in one dimension or the other.

So I don't think we need this table-caption exemption after all. Or, if we do, I need help figuring out how to trigger the sort of failure that causes us to need this exemption, for the benefit of the automated test that I'm writing in bug 1508420.
Assignee

Comment 1

6 months ago
dbaron, it looks like the "table-caption" check here dates back to your first patches in bug 1159042 comment 7. (search for "caption" in https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/22ba869b38a4/set-dynamic-reflow-root )

Do you agree that we can remove this exemption, or is there something I'm not thinking of here?
Assignee

Comment 3

6 months ago
Note: table-caption does have one kinda-special sizing behavior -- its width[1] (whether specified or content-based) does establish a minimum width for the table itself.

However, that's not really that special; the same is true for *any* element that has an intrinsically-sized ancestor; and that doesn't prevent us from making the inner element a dynamic reflow root (if it satisfies all the conditions, e.g. its own size is fixed).

Assuming the table caption has a specified pixel-valued size which remains fixed (and that it satisfies all the other dynamic reflow root requirements RE being a fixed-position CB etc.), I believe it's valid for us to label it as a dynamic reflow root and to initiate reflows at the caption itself if the only dirty frames are inside of it.


[1] (s/width/height/ depending on the caption side, of course)
Assignee

Comment 4

6 months ago
(meant to set ni=dbaron (post-vacation) for comment 1)
Flags: needinfo?(dbaron)
Assignee

Updated

6 months ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED

This sounds reasonable to me; I don't particularly remember the rationale here.

I think it's probably worth double-checking two things:
(1) that it's true for all six values of caption-side (top, bottom, left, right, top-outside, bottom-outside)
(2) that it's also true in other browser engines.

If so, I think it seems fine to remove this exception.

Flags: needinfo?(dbaron)
Assignee

Comment 6

5 months ago

(In reply to David Baron from comment #5)

Thanks for the reply.

I think it's probably worth double-checking two things:
(1) that it's true for all six values of caption-side (top, bottom, left, right, top-outside, bottom-outside)

It seems to be. I opened the attached testcase (with a fixed-size caption whose contents & are larger than its fixed-size, and whose table is also larger), and I ran through the caption-side values you listed via tweaks in devtools. And in all cases, the caption was only as large as its specified size (with its too-large-content overflowing).0

(2) that it's also true in other browser engines.

I tried Chrome and Safari, and it's true there for the two caption-side values that they support ("top" and "bottom").

If so, I think it seems fine to remove this exception.

Great, thanks!

Comment 8

5 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9969ea8e9a58
Allow table-caption boxes to be dynamic reflow roots. r=dbaron

Comment 9

5 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.