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

[css-values] Reftest for percentage root font size and rem units #18941

Conversation

@dotnetCarpenter
Copy link
Contributor

@dotnetCarpenter dotnetCarpenter commented Sep 9, 2019

rem unit
Equal to the computed value of font-size on the root element.
If used in the font-size property of the root element, or in a document with no root element, 1rem is equal to the initial value of the font-size property.

This test sets the font-size of the :root element to 20%, which according to spec means that 31.25rem = 100px. This test assumes that the initial value of the font-size property is 16px, as observed in desktop Chrome, Opera, Safari, Firefox, IE and Edge.

This test fails in current Chrome (78.0.3904.7) as explained here: https://lists.w3.org/Archives/Public/public-test-infra/2019JulSep/0002.html and https://bugs.chromium.org/p/chromium/issues/detail?id=308862.
And probably also Webkit Safari - see https://bugs.webkit.org/show_bug.cgi?id=117680
Test passes in Edge, IE10, IE11 and Firefox

Spec: https://www.w3.org/TR/css-values-3/#rem

Note about current browser differences: AFAIK Internet Explorer and Firefox has always passed this test. Webkit was the only engine to fail. Since blink was forked from Webkit, this test now also fails in Chrome, Opera etc. The percentage scale bug is pre blink.

@dotnetCarpenter dotnetCarpenter changed the title Reftest for percentage root font size and rem units [css-values] Reftest for percentage root font size and rem units Sep 9, 2019
@FremyCompany FremyCompany removed their request for review Sep 9, 2019
css/css-values/percentage-rem-low.html Show resolved Hide resolved
css/css-values/percentage-rem-low-ref.html Outdated Show resolved Hide resolved
css/css-values/percentage-rem-low.html Outdated Show resolved Hide resolved
css/css-values/percentage-rem-low.html Outdated Show resolved Hide resolved
<meta name="assert" content="When :root font-size is 20%, a 31.25rem length must be 100px, if the browser default for :root font-size is 16px.">
<style>
:root {
font-size: 20%; /* default is 16px in all desktop browsers */

This comment has been minimized.

@foolip

foolip Sep 10, 2019
Member

Since this be set to a fixed value in px, like 3.2px or maybe some other very small value that makes the other numbers nice and round? This way the test doesn't depend on the default font size.

This comment has been minimized.

@gsnedders

gsnedders Sep 10, 2019
Contributor

The Blink/WebKit bug is specifically about percentages, no?

That said, I'd rather we go for for 25% if we can (because then we get a nice round 4px and avoid any potential subpixel rendering differences).

Also we already assume 16px as default all over the place. We could write a new reference (maybe a 8rem square to keep everything integral) for this if we have any interest in changing that, but I don't think we do?

This comment has been minimized.

@lilles

lilles Sep 10, 2019
Member

The Blink issue is not related to percentages. It's generally about minimum font-size being applied to font-size before it's used to resolve em and rem units.

I'd pick a really small font-size (< 1px ?) to make sure any minimum font-size will affect the issue. In danger of introducing too much churn here, this could have been a testharness.js test.

This comment has been minimized.

@dotnetCarpenter

dotnetCarpenter Sep 10, 2019
Author Contributor

@lilles In my tests, I can always reproduce the bug if I use low percentage but not with small px values. But Trevor Burnham's (https://bugs.chromium.org/p/chromium/issues/detail?id=308862#c21) demo does highlight that it is not the only case. https://codepen.io/TrevorBurnham/pen/VXqoEW
I'm not sure why.

I don't know what a testharness.js test is.

This comment has been minimized.

@dotnetCarpenter

dotnetCarpenter Sep 10, 2019
Author Contributor

@gsnedders I'll change it to 25%

This comment has been minimized.

@gsnedders

gsnedders Sep 10, 2019
Contributor

See https://web-platform-tests.org/writing-tests/testharness.html for testharness.js tests. I think writing a test based on Trevor Burnham's asserting that window.getComputedStyle(x).fontSize is the same for each pair and x.offsetWidth is the same for each pair.

This comment has been minimized.

@gsnedders

gsnedders Sep 10, 2019
Contributor

@lilles Also the testsuite assumptions include "there is no minimum font-size" so we might not be able to test this. I wonder how much breaks (probably in the CSS2 testsuite?) if we remove that assumption…

@dotnetCarpenter
Copy link
Contributor Author

@dotnetCarpenter dotnetCarpenter commented Sep 10, 2019

Reopen to get screen shot in Chrome - failed last time due to an Ubuntu mirror issue.

@foolip
Copy link
Member

@foolip foolip commented Sep 10, 2019

Results are at https://wpt.fyi/results/css/css-values/percentage-rem-low.html?label=pr_head&max-count=1&pr=18941 (same link will work after updates, just check that the commit sha is the same to be sure you're seeing the right results)

@foolip
Copy link
Member

@foolip foolip commented Sep 10, 2019

The expected and actual results in Chrome look very similar to me, I can't spot what the difference is...

I think this is actually a known issue that @Hexcles has told me about, that when there are multiple references we'll just pick the first or the last to show. If it's possible to express this test with just a single ref that should match, I think that'd be the most robust.

@dotnetCarpenter
Copy link
Contributor Author

@dotnetCarpenter dotnetCarpenter commented Sep 10, 2019

@gsnedders @foolip FYI, my latest commit results shows that the test pass in firefox, as expected. And fails in Chrome, as expected. In Safari the test fails but not only is the square too big but the ref text, which is styled with font-size:initial;, is also too small. The wrong text font-size is not really part of the test.

https://wpt.fyi/results/css/css-values/percentage-rem-low.html?label=pr_head&max-count=1&pr=18941

Should I change the ref text styling to font-size: 16px;, so that does not impact if the test fails or succeed?

I'm not sure if there is a test for font-size: initial; but there probably should be since Safari differs from Chrome and Firefox.

@foolip
Copy link
Member

@foolip foolip commented Sep 10, 2019

Should I change the ref text styling to font-size: 16px;, so that does not impact if the test fails or succeed?

Yes, locking down variations in the ref file is a good idea, even if those variations are because of a bug in some browser or a lack of interoperability. Those things should then be tested separately, but you're not on the hook to add those tests yourself just because you noticed the problem. It would be appreciated though, of course. In a separate PR if so.

Copy link
Member

@foolip foolip left a comment

Getting very close to greatness :)

css/css-values/percentage-rem-low.html Show resolved Hide resolved
css/css-values/percentage-rem-low.html Outdated Show resolved Hide resolved
css/css-values/percentage-rem-low.html Show resolved Hide resolved
@Hexcles
Copy link
Member

@Hexcles Hexcles commented Sep 10, 2019

@dotnetCarpenter I saw your IRC messages when you were already offline, so I'm asking here. Re:

dotnetCarpenter Should I change the ref text styling to font-size: 16px;, so that does not impact if the test fails or succeed? I'm not sure if there is a test for font-size: initial; but there probably should be since Sadari differs from Chrome and Firefox.
dotnetCarpenter Sadari Safari
dotnetCarpenter Right, I'm off for now but will check back with you tomorrow. Thank you for all the feedback. It has been a very nice experience indeed.
dotnetCarpenter I take it back about Safari - perhaps I was seeing a cahed version of the screenshot - it look the exact same as in chrome now (still not if I visit the same page in firefox, hence I believe it to about caching)

I'm not sure if you are talking about the test page or the reftest viewer on wpt.fyi. If you are seeing different rendering of the same wpt.fyi URL on different browsers, please let me know. It's definitely a bug. Browser caching shouldn't cause this, either.

@dotnetCarpenter
Copy link
Contributor Author

@dotnetCarpenter dotnetCarpenter commented Oct 16, 2019

Sorry for the delay. I moved country and didn't have time to update until now. The positively frustrating part is that this particular instance of the bug has been fixed in Chrome 77, so I can not trigger it anymore. I don't know how to downgrade. But I guess the test is still valuable to detect regression.

Edit I take that back. It's fixed in 79.0.3942 not in 77.0.3865. I'm gonna go get some sleep. 💤 I'll check back again tomorrow afternoon.

The Trevor Burnham test still fails in Chrome though. https://codepen.io/TrevorBurnham/pen/VXqoEW

I currently only have Windows and Linux, so I can not test Safari at the moment.

@dotnetCarpenter
Copy link
Contributor Author

@dotnetCarpenter dotnetCarpenter commented Oct 16, 2019

The Blink issue is not related to percentages. It's generally about minimum font-size being applied to font-size before it's used to resolve em and rem units.

I'd pick a really small font-size (< 1px ?) to make sure any minimum font-size will affect the issue. In danger of introducing too much churn here, this could have been a testharness.js test.

@lilles I tried to set root font-size to .0625px; and width/height to 1500rem; but it doesn't trigger the bug in Chrome 77. A different kind of test need to be written to show what you see in https://codepen.io/TrevorBurnham/pen/VXqoEW in blink.

@dotnetCarpenter dotnetCarpenter requested review from foolip, gsnedders and lilles Oct 16, 2019
@dotnetCarpenter
Copy link
Contributor Author

@dotnetCarpenter dotnetCarpenter commented Oct 19, 2019

@foolip I think this can be merged in now?

@foolip
Copy link
Member

@foolip foolip commented Oct 19, 2019

@dotnetCarpenter I saw you requested review from @lilles and was waiting for that, but OK, merge merge away!

@foolip
foolip approved these changes Oct 19, 2019
@foolip foolip merged commit 076ec51 into web-platform-tests:master Oct 19, 2019
10 checks passed
10 checks passed
Azure Pipelines Build #20191016.5 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@dotnetCarpenter
Copy link
Contributor Author

@dotnetCarpenter dotnetCarpenter commented Oct 19, 2019

@dotnetCarpenter I saw you requested review from @lilles and was waiting for that, but OK, merge merge away!

Sorry, my bad. I was requesting review from any of you guys to approve. Thanks for merging!

@dotnetCarpenter dotnetCarpenter deleted the dotnetCarpenter:reftest-for-percentage-root-font-size-and-rem-units branch Feb 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.