Remove fail expectations for contain:paint tests

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P3
minor
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: yusuf, Assigned: yusuf)

Tracking

unspecified
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 months ago
Since contain:paint currently passes all web-platform tests, we can remove any remaining fail expectations (meta ini files).
Hmm... If we pass this test, do you know why isn't turning TreeHerder orange right now, for an "unexpected pass" condition?
Flags: needinfo?(ysermet)
(e.g. hypothetically, if treeherder web-platform-tests runs are green both with & without this patch applied, that might suggest that we're not running the test at all, or that the test is nerfed in some other way.)
(Assignee)

Comment 4

10 months ago
I checked it out and it seems like the original test file uses absolute positioning for rtc:after, and the reference does not, which causes it to fail even if it passes ("PASS" is printed, but in the wrong place). Once that's removed, this test shouldn't create any issue. I suspect, using absolute positioning with internal ruby elements to test if we ignore contain:paint may not be the best way, since it causes the internal ruby element to be considered as display:block [1], which is in the scope of contain:paint. And, I don't see a specific reason for this test to use absolute positioning anyway. Should we just update that test, or do we need to comment on this on github[2]?

On a related note, though I am checking if the web-platform tests are working after our updates, I am not necessarily thoroughly checking the integrity of those tests, i.e. if they are actually testing the intended functionality.

[1] "Note that absolute positioning or floating an element causes its display value to compute to a block-level equivalent. (See [CSS-DISPLAY-3] or [CSS2] section 9.7.) For the internal ruby display types, this causes their display value to compute to block. ", source: https://drafts.csswg.org/css-ruby-1/#block-ruby

[2] https://github.com/w3c/csswg-drafts/issues/2512
Flags: needinfo?(ysermet)
Gotcha -- thanks for investigating.

I think the goal of the test is to be sure that "contain:paint" on the <rtc> element doesn't end up clipping the painting of its child.  You're correct that position:absolute on the child ends up changing its "display" value, but that's not really important here -- the <rtc> is still a ruby-text-container, and that's the thing we're testing contain:paint on.

So I think the only problem here is that the test is implicitly assuming that the ::after child will render at the same position regardless of its "position:absolute" styling -- and that's not the case, in Chrome & Firefox. (I haven't investigated precisely why but I'm not entirely surprised.)

To avoid that flaky dependency (and to make the testcase & reference case as similar as possible aside from the thing that's being tested which is 'contain:paint'), we should probably just add position:absolute in the reference case.
Comment hidden (mozreview-request)
(Assignee)

Comment 7

10 months ago
Thanks for the explanation! I updated the reference case and pushed it just now.

Comment 8

10 months ago
mozreview-review
Comment on attachment 8988047 [details]
Bug 1471452 - Fix a contain:paint web platform test by addressing an inconsistency between testcase & reference case.

https://reviewboard.mozilla.org/r/253292/#review260126

::: commit-message-4b498:1
(Diff revision 2)
> +Bug 1471452 - Remove any remaining fail expectations for contain:paint tests'. r=dholbert

The commit message should describe the change you're making to the test, too.  (E.g. "Fix a contain:paint web platform test by addressing an inconsistency between testcase & reference case")

::: testing/web-platform/tests/css/css-contain/reference/contain-paint-007-ref.html:6
(Diff revision 2)
>  <!doctype html>
>  <html lang=en>
>    <meta charset=utf-8>
>    <title>CSS-contain test reference</title>
>    <link rel="author" title="Florian Rivoal" href="https://florian.rivoal.net">
> +  <link rel="author" title="Yusuf Sermet" href="mailto:ysermet@mozilla.com">

Probably better not to bother adding an "author" tag for a tweak like this... It kinda adds clutter to have 2 authors, and this isn't an especially substantial change (just copying some markup from the testcase).

You'll still be credited in the test's change history, via version control. :)

::: testing/web-platform/tests/css/css-contain/reference/contain-paint-007-ref.html:13
(Diff revision 2)
> +rtc::after {
> +  content: "PASS";
> +  position: absolute;
> +}

[not about this line in particular, just noting in general]:

I'm pretty sure that after any changes to WPT test files, you have to run this command:
 ./mach web-platform-tests --manifest-update
...which updates some manifest file with new hashes of the test files.

(That command probably will produce changes to manifest.json, which you should include as part of this patch.  Note that this particular ./mach command requires your build to have completed -- it runs some executables in your obj dir, I think.)

Without this, you'll run into trouble like I did in bug 1450390 comment 15.
Attachment #8988047 - Flags: review?(dholbert) → review-
(Assignee)

Comment 9

10 months ago
(In reply to Daniel Holbert [:dholbert] from comment #8)
> Comment on attachment 8988047 [details]
> Bug 1471452 - Remove any remaining fail expectations for contain:paint
> tests'.
> 
> https://reviewboard.mozilla.org/r/253292/#review260126
> 
> ::: commit-message-4b498:1
> (Diff revision 2)
> > +Bug 1471452 - Remove any remaining fail expectations for contain:paint tests'. r=dholbert
> 
> The commit message should describe the change you're making to the test,
> too.  (E.g. "Fix a contain:paint web platform test by addressing an
> inconsistency between testcase & reference case")

Done.

> Probably better not to bother adding an "author" tag for a tweak like
> this... It kinda adds clutter to have 2 authors, and this isn't an
> especially substantial change (just copying some markup from the testcase).
> 
> You'll still be credited in the test's change history, via version control.
> :)

I honestly don't care about the credit. It's just a copy paste, like you said :). I thought it was a rule to add the name when making any changes, since if there happens an error on the changes I make, responsibility may go to the original writer even though it wasn't their mistake. I removed the author tag, and good to know this for future!

> I'm pretty sure that after any changes to WPT test files, you have to run
> this command:
>  ./mach web-platform-tests --manifest-update
> ...which updates some manifest file with new hashes of the test files.

Doing this right now.
(In reply to Yusuf Sermet [:yusuf] from comment #9)
> I honestly don't care about the credit. It's just a copy paste, like you
> said :). I thought it was a rule to add the name when making any changes,
> since if there happens an error on the changes I make, responsibility may go
> to the original writer even though it wasn't their mistake. I removed the
> author tag, and good to know this for future!

Gotcha, yeah. I'm not aware of any such rule, at least (though it's possible such a rule exists & I'm not aware of it; let me know if you ran across something suggesting that you should do this).  But my gut feeling is that such a rule wouldn't scale well, for tests that get multiple edits over time.

Thanks!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

10 months ago
Tests are still running on my machine, but as you suggested, the manifest file was written at the start. So, I went ahead and committed it. I'll keep the tests running though, just in case any complication comes up.

Thanks!

Comment 14

10 months ago
mozreview-review
Comment on attachment 8988047 [details]
Bug 1471452 - Fix a contain:paint web platform test by addressing an inconsistency between testcase & reference case.

https://reviewboard.mozilla.org/r/253292/#review260204
Attachment #8988047 - Flags: review?(dholbert) → review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed
(Try run looks busted, but I think the bustage is from unrelated work that this is based on top of.  Optimistically triggering autoland, in any case, on the assumption that you've tested locally that the reftest passes with your changes. :))

Comment 16

10 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b2c8eebbb03
Fix a contain:paint web platform test by addressing an inconsistency between testcase & reference case. r=dholbert
Keywords: checkin-needed
(Assignee)

Comment 17

10 months ago
(In reply to Daniel Holbert [:dholbert] from comment #15)
> (Try run looks busted, but I think the bustage is from unrelated work that
> this is based on top of.  Optimistically triggering autoland, in any case,
> on the assumption that you've tested locally that the reftest passes with
> your changes. :))

Exactly, it's because of the contain:style patches which are still a work in progress. Tests are still continuing in my machine, but hopefully there won't be any issue :)
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11701 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 20

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5b2c8eebbb03
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.