Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print reftest documentation doesn't match reality? #40788

Closed
mstensho opened this issue Jun 28, 2023 · 5 comments
Closed

Print reftest documentation doesn't match reality? #40788

mstensho opened this issue Jun 28, 2023 · 5 comments
Labels

Comments

@mstensho
Copy link
Contributor

I was looking into changing Chromium to behave as described in https://web-platform-tests.org/writing-tests/print-reftests.html when it comes to the page size - which means a default page size of 5 by 3 inches.

Chromium currently uses a page area size of 4 by 2 inches for WPT print reftests, assuming that there's a 0.5 inch margin on every side of a page box with the size of 5 by 3 inches [1]. The WPT print reftest documentation says nothing about page margins, so I would think that we should simply use a 5 by 3 inch page area size. That said, there are quite a few existing WPT tests that go @page { size: 5in 3in; margin: 0.5in; }, so the hardcoded defaults in Chromium seemed like a reasonable quirk, as long as Chromium doesn't honor @page rules when running WPT print reftests. But that's precisely what I'm working on fixing, so I thought it would be a good time to start using 5x3.

However, then infrastructure/reftest/reftest_match-print.html would start failing, since that test is assuming a page area height 2 inches (192 CSS pixels). Since this test is also passing in Gecko, it has to mean that Gecko is also operating with a page area height of 2 inches, and not 3 inches, which is what the WPT documentation says.

What to do?

A: Fix Gecko and Chromium, and update the test
B: Fix https://web-platform-tests.org/writing-tests/print-reftests.html

[1] https://source.chromium.org/chromium/chromium/src/+/main:content/web_test/renderer/test_runner.cc;l=107-108;drc=dcee6b77444693128fa284cb3b33a8fbbbbbd6b2

@gsnedders
Copy link
Member

cc @jgraham

aarongable pushed a commit to chromium/chromium that referenced this issue Oct 23, 2023
Note that some of the ones now marked with crbug.com/1494847 fail for
additional reasons as well. For one, it looks like some of them also
fail because Blink and Gecko still don't agree on the default page size
or margins. See web-platform-tests/wpt#40788

Bug: 1494838, 1494847, 1494858, 1494859, 1494863
Change-Id: I452def0e90e23bf4f0ddc0c35ca53abc2ce5a7bf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4965604
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1213710}
@jwatt
Copy link
Contributor

jwatt commented Nov 28, 2023

@mstensho

However, then infrastructure/reftest/reftest_match-print.html would start failing, since that test is assuming a page area height 2 inches (192 CSS pixels). Since this test is also passing in Gecko, it has to mean that Gecko is also operating with a page area height of 2 inches, and not 3 inches, which is what the WPT documentation says.

We're acting with a default of @page { size: 5in 3in; margin: 0.5in; } - so a page box of 5x3in and a page area of 4x2in. We defaulted to the 0.5in margin since that was in the RFC I believe. (I'm going to move more from that RFC to the actual docs.)

So maybe best to just keep using the 0.5in margin in your test runner and close this issue?

@mstensho
Copy link
Contributor Author

Thank you for confirming that Firefox uses 0.5in margins for WPT reftests. We'll now do this in Chromium as well:

https://chromium-review.googlesource.com/c/chromium/src/+/5067421

This issue should remain open as long as the documentation doesn't mention margins though, shouldn't it?

moz-wptsync-bot pushed a commit that referenced this issue Dec 1, 2023
The default 0.5 inch margin comes from the original RFC:
https://github.com/web-platform-tests/rfcs/blob/master/rfcs/print_test.md

Firefox already defaults to 0.5 inch margins:
https://searchfox.org/mozilla-central/rev/d84469b005106c5ab0f65e283f71c1415ce76c54/remote/marionette/reftest.sys.mjs#46-48

Chromium will shortly default to 0.5 inch margins:
#40788

Differential Revision: https://phabricator.services.mozilla.com/D195184

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1867684
gecko-commit: acb46883beacbf5b9c50dd2c4dd785af7b7fd7d3
gecko-reviewers: jgraham
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Dec 2, 2023
vinnydiehl pushed a commit to vinnydiehl/mozilla-unified that referenced this issue Dec 3, 2023
moz-wptsync-bot pushed a commit that referenced this issue Dec 3, 2023
The default 0.5 inch margin comes from the original RFC:
https://github.com/web-platform-tests/rfcs/blob/master/rfcs/print_test.md

Firefox already defaults to 0.5 inch margins:
https://searchfox.org/mozilla-central/rev/d84469b005106c5ab0f65e283f71c1415ce76c54/remote/marionette/reftest.sys.mjs#46-48

Chromium will shortly default to 0.5 inch margins:
#40788

Differential Revision: https://phabricator.services.mozilla.com/D195184

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1867684
gecko-commit: acb46883beacbf5b9c50dd2c4dd785af7b7fd7d3
gecko-reviewers: jgraham
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Dec 7, 2023
… page margin. r=jgraham

The default 0.5 inch margin comes from the original RFC:
https://github.com/web-platform-tests/rfcs/blob/master/rfcs/print_test.md

Firefox already defaults to 0.5 inch margins:
https://searchfox.org/mozilla-central/rev/d84469b005106c5ab0f65e283f71c1415ce76c54/remote/marionette/reftest.sys.mjs#46-48

Chromium will shortly default to 0.5 inch margins:
web-platform-tests/wpt#40788

Differential Revision: https://phabricator.services.mozilla.com/D195184

UltraBlame original commit: acb46883beacbf5b9c50dd2c4dd785af7b7fd7d3
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Dec 7, 2023
… page margin. r=jgraham

The default 0.5 inch margin comes from the original RFC:
https://github.com/web-platform-tests/rfcs/blob/master/rfcs/print_test.md

Firefox already defaults to 0.5 inch margins:
https://searchfox.org/mozilla-central/rev/d84469b005106c5ab0f65e283f71c1415ce76c54/remote/marionette/reftest.sys.mjs#46-48

Chromium will shortly default to 0.5 inch margins:
web-platform-tests/wpt#40788

Differential Revision: https://phabricator.services.mozilla.com/D195184

UltraBlame original commit: acb46883beacbf5b9c50dd2c4dd785af7b7fd7d3
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Dec 7, 2023
… page margin. r=jgraham

The default 0.5 inch margin comes from the original RFC:
https://github.com/web-platform-tests/rfcs/blob/master/rfcs/print_test.md

Firefox already defaults to 0.5 inch margins:
https://searchfox.org/mozilla-central/rev/d84469b005106c5ab0f65e283f71c1415ce76c54/remote/marionette/reftest.sys.mjs#46-48

Chromium will shortly default to 0.5 inch margins:
web-platform-tests/wpt#40788

Differential Revision: https://phabricator.services.mozilla.com/D195184

UltraBlame original commit: acb46883beacbf5b9c50dd2c4dd785af7b7fd7d3
@jwatt
Copy link
Contributor

jwatt commented Dec 9, 2023

This issue should remain open as long as the documentation doesn't mention margins though, shouldn't it?

Yes. 89468ad updated the docs, and the live docs now reflect this change, so I think this can be closed now.

@mstensho
Copy link
Contributor Author

Excellent! Yes, let's close this. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants