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.

Sign up for GitHub

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

Jump to bottom

[math functions] Change mod() math function subtest #335

Closed
danielsakhapov opened this issue May 16, 2023 · 8 comments
Closed

[math functions] Change mod() math function subtest #335

danielsakhapov opened this issue May 16, 2023 · 8 comments
Labels
focus area: CSS Math Functions test-change-proposal Proposal to add or remove tests for an interop area

Comments

@danielsakhapov
Copy link

Test List

https://wpt.fyi/css/css-values/round-mod-rem-computed.html

Rationale

Change mod(-18px,100% / 15) to mod(-18px,100% / 10) and expected value from to 2px 4.5px. As Chrome doesn't pass due to some rounding of layout units. Probably Firefox would like to change mod(-18%,5%) also, as they fail for the same reason.
@danielsakhapov danielsakhapov added the test-change-proposal Proposal to add or remove tests for an interop area label May 16, 2023
@foolip
Copy link
Member

foolip commented May 16, 2023

@danielsakhapov can you link to a draft CL / PR for this change?

@danielsakhapov
Copy link
Author

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

@foolip
Copy link
Member

foolip commented May 17, 2023

Thanks @danielsakhapov! That's exported at https://github.com/web-platform-tests/wpt/pull/40008/files.

@jgraham can you handle this proposal for Gecko?
@gsnedders @nt1m can you handle this proposal for WebKit?

@nt1m
Copy link
Member

nt1m commented May 17, 2023

This is fine (the math looks correct either way), but a bit surprising diving 75 / 10 produces rounding errors while 75 / 15 doesn't.

@foolip
Copy link
Member

foolip commented May 23, 2023

@dholbert @tiaanl are either of you able to review this for Gecko? (I pinged James and he suggested one of you might be the right contact.)

@dholbert
Copy link

Yeah, this seems fine to me. Change makes sense, and Firefox still passes the test after the change locally for me, so we're not e.g. trading between different implementation float rounding edge cases. :)

@dholbert
Copy link

dholbert commented May 24, 2023 •
edited

Probably Firefox would like to change mod(-18%,5%) also, as they fail for the same reason.

I noticed that one too, yeah. It looks like our float-rounding error goes away if I change -18% to -19% there, and the expected-equivalent value from 2% to 1%. And it looks like other browsers render those consistently, too, based on this testcase (devtools shows the same computed value for margin-left for "Test" vs. "Ref" there, in Firefox/Epiphany/Chrome-with-experimental-web-features-enabled.)

@danielsakhapov would you mind making that edit while you're here, as a ridealong to your existing change, sharing the same justification? i.e. changing line 131 from...

test_math_used('mod(-18%,5%)', '2%');

...to:

test_math_used('mod(-19%,5%)', '1%');

@danielsakhapov
Copy link
Author

@dholbert Sure thing, done! Thanks everyone!

Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 25, 2023
@danielsakhapov @chromium-wpt-export-bot
Change failing WPT test for css function mod()
def3f7d 
Fail is due to the rounding error, as the test checks the mod()
function and not the rounding behaviour, the test is changed to pass.

Resolved here: web-platform-tests/interop#335

Bug: 1407473
Change-Id: Ic7776e3a131348140c56ebe0e79e2dcea3282790
aarongable pushed a commit to chromium/chromium that referenced this issue May 25, 2023
@danielsakhapov
Change failing WPT test for css function mod()
0ce23a7 
Fail is due to the rounding error, as the test checks the mod()
function and not the rounding behaviour, the test is changed to pass.

Resolved here: web-platform-tests/interop#335

Bug: 1407473
Change-Id: Ic7776e3a131348140c56ebe0e79e2dcea3282790
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4529482
Reviewed-by: Steinar H Gunderson <sesse@chromium.org>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1149045}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 25, 2023
@danielsakhapov @chromium-wpt-export-bot
Change failing WPT test for css function mod()
aef3010 
Fail is due to the rounding error, as the test checks the mod()
function and not the rounding behaviour, the test is changed to pass.

Resolved here: web-platform-tests/interop#335

Bug: 1407473
Change-Id: Ic7776e3a131348140c56ebe0e79e2dcea3282790
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4529482
Reviewed-by: Steinar H Gunderson <sesse@chromium.org>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1149045}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 25, 2023
@danielsakhapov @chromium-wpt-export-bot
Change failing WPT test for css function mod()
604996f 
Fail is due to the rounding error, as the test checks the mod()
function and not the rounding behaviour, the test is changed to pass.

Resolved here: web-platform-tests/interop#335

Bug: 1407473
Change-Id: Ic7776e3a131348140c56ebe0e79e2dcea3282790
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4529482
Reviewed-by: Steinar H Gunderson <sesse@chromium.org>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1149045}
@nairnandu nairnandu mentioned this issue May 25, 2023
Closed
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jun 13, 2023
@danielsakhapov @moz-wptsync-bot
Bug 1833087 [wpt PR 40008] - Change failing WPT test for css function…
ec78576 
… mod(), a=testonly

Automatic update from web-platform-tests
Change failing WPT test for css function mod()

Fail is due to the rounding error, as the test checks the mod()
function and not the rounding behaviour, the test is changed to pass.

Resolved here: web-platform-tests/interop#335

Bug: 1407473
Change-Id: Ic7776e3a131348140c56ebe0e79e2dcea3282790
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4529482
Reviewed-by: Steinar H Gunderson <sesse@chromium.org>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1149045}

--

wpt-commits: 604996f06850cec8178d60c47b273c03fbf0aec2
wpt-pr: 40008
ErichDonGubler pushed a commit to ErichDonGubler/firefox that referenced this issue Jun 14, 2023
@danielsakhapov @moz-wptsync-bot
Bug 1833087 [wpt PR 40008] - Change failing WPT test for css function…
03db754 
… mod(), a=testonly

Automatic update from web-platform-tests
Change failing WPT test for css function mod()

Fail is due to the rounding error, as the test checks the mod()
function and not the rounding behaviour, the test is changed to pass.

Resolved here: web-platform-tests/interop#335

Bug: 1407473
Change-Id: Ic7776e3a131348140c56ebe0e79e2dcea3282790
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4529482
Reviewed-by: Steinar H Gunderson <sesse@chromium.org>
Commit-Queue: Daniil Sakhapov <sakhapov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1149045}

--

wpt-commits: 604996f06850cec8178d60c47b273c03fbf0aec2
wpt-pr: 40008
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Jun 16, 2023
@marco-c
Bug 1833087 [wpt PR 40008] - Change failing WPT test for css function…
408f1dc 
… mod(), a=testonly

Automatic update from web-platform-tests
Change failing WPT test for css function mod()

Fail is due to the rounding error, as the test checks the mod()
function and not the rounding behaviour, the test is changed to pass.

Resolved here: web-platform-tests/interop#335

Bug: 1407473
Change-Id: Ic7776e3a131348140c56ebe0e79e2dcea3282790
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4529482
Reviewed-by: Steinar H Gunderson <sessechromium.org>
Commit-Queue: Daniil Sakhapov <sakhapovchromium.org>
Cr-Commit-Position: refs/heads/main{#1149045}

--

wpt-commits: 604996f06850cec8178d60c47b273c03fbf0aec2
wpt-pr: 40008

UltraBlame original commit: ce095f1fd991d8732a33f06685ba017ca4576f51
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Jun 16, 2023
@marco-c
Bug 1833087 [wpt PR 40008] - Change failing WPT test for css function…
84c5994 
… mod(), a=testonly

Automatic update from web-platform-tests
Change failing WPT test for css function mod()

Fail is due to the rounding error, as the test checks the mod()
function and not the rounding behaviour, the test is changed to pass.

Resolved here: web-platform-tests/interop#335

Bug: 1407473
Change-Id: Ic7776e3a131348140c56ebe0e79e2dcea3282790
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4529482
Reviewed-by: Steinar H Gunderson <sessechromium.org>
Commit-Queue: Daniil Sakhapov <sakhapovchromium.org>
Cr-Commit-Position: refs/heads/main{#1149045}

--

wpt-commits: 604996f06850cec8178d60c47b273c03fbf0aec2
wpt-pr: 40008

UltraBlame original commit: ce095f1fd991d8732a33f06685ba017ca4576f51
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jun 16, 2023
@marco-c
Bug 1833087 [wpt PR 40008] - Change failing WPT test for css function…
c94f13a 
… mod(), a=testonly

Automatic update from web-platform-tests
Change failing WPT test for css function mod()

Fail is due to the rounding error, as the test checks the mod()
function and not the rounding behaviour, the test is changed to pass.

Resolved here: web-platform-tests/interop#335

Bug: 1407473
Change-Id: Ic7776e3a131348140c56ebe0e79e2dcea3282790
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4529482
Reviewed-by: Steinar H Gunderson <sessechromium.org>
Commit-Queue: Daniil Sakhapov <sakhapovchromium.org>
Cr-Commit-Position: refs/heads/main{#1149045}

--

wpt-commits: 604996f06850cec8178d60c47b273c03fbf0aec2
wpt-pr: 40008

UltraBlame original commit: ce095f1fd991d8732a33f06685ba017ca4576f51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
focus area: CSS Math Functions test-change-proposal Proposal to add or remove tests for an interop area
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@zcorpan @dholbert @foolip @nt1m @danielsakhapov