Closed Bug 1478550 Opened 6 years ago Closed 6 years ago

'contain:size' should apply to table-caption, but doesn't

Categories

(Core :: Layout, enhancement, P3)

Product:
Core
Component:
Layout
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: dbaron, Assigned: morgan)

References

Details

(Keywords: css3)

Attachments

(1 file)

Bug 1478550 - Adjust IsInnerTableStyle to fail for table captions.
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
While reviewing bug 1472919 I noticed that the IsContainSize function:
https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/layout/style/nsStyleStruct.h#2343-2355
incorrectly returns false for an element with 'display:table-caption' that has 'contain:size'.

This confusion may result from the fact that in spec terminology:
https://drafts.csswg.org/css-display-3/#internal-table-element
table-caption is *not* an internal table element (whereas table-cell is), but in Gecko code they are both considered to be so.

It's possible we should refactor the IsInnerTableStyle function into two functions:
 - IsInternalTableStyle (excluding caption)
 - IsInternalTableStyleOrCaption
where the latter corresponds to the current function and the former to the spec's "internal table style" definition.  (When doing that refactoring, it may be worth looking closely at the callers to see which path they should take.)

It's also possible that the conclusion is that instead, the spec should change so that size containment does not apply to table-caption just like it doesn't apply to table-cell, given that table-caption and table-cell are rather similar.  So filing a spec bug instead of changing the behavior might be a sensible course of action.  (In fact, if we're not sure what makes sense, it might be worth filing a spec bug anyway to ask for the issue to be considered, since it may not have been.)
Flags: needinfo?(mreschenberg)
I think a spec change makes the most sense to me, given we've already said size-containment shouldn't apply to tables or internal table elements, but we'll see what csswg things. Thanks for filing that issue, I'll keep an eye on it and take this bug + make any changes if they become necessary.
Flags: needinfo?(mreschenberg)
Speaking of what they think* (not things, oops), looks like they're leaning towards keeping it so I'll refactor that function and push a temp patch.
Assignee: nobody → mreschenberg
I have 1 test on contain: size and table-caption:

http://www.gtalbot.org/BrowserBugsSection/CSS3Contain/contain-size-056.html
Importing this patch on a clean copy of central + building passes Gérard's test above.
Comment on attachment 8995332 [details]
Bug 1478550 - Adjust IsInnerTableStyle to fail for table captions.

https://reviewboard.mozilla.org/r/259788/#review267418

::: layout/base/nsLayoutUtils.cpp:9445
(Diff revision 1)
>    bool isTableElement = disp->IsInnerTableStyle() &&
> -    disp->mDisplay != StyleDisplay::TableCell &&
> +    disp->mDisplay != StyleDisplay::TableCell;
> -    disp->mDisplay != StyleDisplay::TableCaption;

As long as you're modifying this chunk, it looks like this *really* wants to be using "IsInternalTableStyleExceptCell". (It's kind of implementing that by hand, backwards. :))

::: layout/style/nsStyleStruct.h:2278
(Diff revision 1)
> +  bool IsInnerTableStyleOrCaption() const {
>      return mozilla::StyleDisplay::TableCaption == mDisplay ||
>             mozilla::StyleDisplay::TableCell == mDisplay ||
>             IsInternalTableStyleExceptCell();

This new "IsInnerTableStyleOrCaption" function has no callers.  So there's probably no need for it to exist, right?

So: unless I'm missing something, probably just get rid of this new function (and update the commit message to remove the bit about adding it).
Priority: -- → P3
Comment on attachment 8995332 [details]
Bug 1478550 - Adjust IsInnerTableStyle to fail for table captions.

https://reviewboard.mozilla.org/r/259788/#review268336

(Restoring r? which it seems I cleared)

This looks good, though per IRC, let's include at least one simple test to verify that the code change is actually having an effect.

At this point, I think it would be a 'contain:paint' test (verifying that overflowing content gets clipped), since that's the only place this will have an effect in current Nightly until other contain:layout stuff lands.
Attachment #8995332 - Flags: review?(dholbert)
Sorry, I meant "contain:size" in comment 9 (not contain:paint); I got mixed up about which pieces of containment this mattered for.  (It looks like this does also matter for contain:paint, but it seems to already work correctly there for some reason, which confuses me slightly but I'm not going to worry about too much.)

Here's a simple testcase for this with contain:size, which I used to convince myself that I'm understanding how this applies there at least:
   https://jsfiddle.net/Lv6whmp2/

(In Chrome, that renders as a tiny collapsed blue square, whereas in Firefox it's huge.  We could have a reftest like that, where the reference case is the same but with just an auto-sized <caption> without any child (& without the specified red background, which won't render anyway).
Comment on attachment 8995332 [details]
Bug 1478550 - Adjust IsInnerTableStyle to fail for table captions.

https://reviewboard.mozilla.org/r/259788/#review268380

r=me with one small CSS tweak:

::: layout/reftests/w3c-css/submitted/contain/contain-size-table-caption-001.html:13
(Diff revision 4)
> +  .innerContents {
> +    height: 100px;
> +    width: 100px;
> +    border: none;
> +    color: transparent;

No need for this "border: none" styling.

The element that this rule applies to will already have no border, by default.
Attachment #8995332 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1510b4502e36
Adjust IsInnerTableStyle to fail for table captions. r=dholbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1510b4502e36
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Firefox 63.0a1 buildID=20180808100114 fails

http://test.csswg.org/harness/test/css-contain-1_dev/single/contain-size-011/

but this may be related only to the way the test is designed. Other tests on 'contain: size' and table-caption elements suggests that Firefox 63 implements 'contain: size' on table-caption elements. The 'overflow: hiddden' part in contain-size-011 test is what triggers, causes the failure of the test.

Firefox 63 passes

http://test.csswg.org/harness/test/css-contain-1_dev/single/contain-size-056/
(In reply to Gérard Talbot from comment #17)
> Firefox 63.0a1 buildID=20180808100114 fails
> 
> http://test.csswg.org/harness/test/css-contain-1_dev/single/contain-size-011/
> 
> but this may be related only to the way the test is designed. Other tests on
> 'contain: size' and table-caption elements suggests that Firefox 63
> implements 'contain: size' on table-caption elements. The 'overflow:
> hiddden' part in contain-size-011 test is what triggers, causes the failure
> of the test.
> 
> Firefox 63 passes
> 
> http://test.csswg.org/harness/test/css-contain-1_dev/single/contain-size-056/

Looks like this involves overflow:hidden which is causing problems with contain:size per bug 1482173. I'll repost this there. Thanks!
(whoops, looks like its already referenced there, nevermind!)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: