1516454 - Computed value of border-radius with calc(percentage) is incorrect

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Gérard Talbot]

Reduced test
- - - - - - 

http://www.gtalbot.org/BrowserBugsSection/CSS3Values/Bugzilla-getComputedStyle-border-radius.html



Actual result: 8 sub-test failures
eg: the reported computed value of 'border-top-left-radius: calc(50%)' is '0px + 50%'


Expected result: 8 sub-test successes
eg: the reported computed value of 'border-top-left-radius: calc(50%)' should be '50%'


Notes
-----
- The original test is:
https://chromium.googlesource.com/chromium/src/+/c825d655f6aaf73484f9d56e9012793f5b9668cc/third_party/WebKit/LayoutTests/css3/calc/getComputedStyle-border-radius.html
- I get actual results with Firefox 60.4.0 ESR and with Firefox 66.0a1 buildID=20181226093642
- Chromium 71.0.3578.80 and Epiphany 3.22.7 (WebKit 2.18.6) pass this test
- I searched for a duplicate and did not find any.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Always a good excuse to do some cleanup.

Do you plan to submit that test to WPT?

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1516454 - Serialize border-radius and outline-radius with Servo. r?#style

I'll add a test if Gerald is not planning to submit it to WPT soon-ish.

Comment 3

[Gérard Talbot]

Emilio,

Yes, my initial plan was to submit the original  
(...) third_party/WebKit/LayoutTests/css3/calc/getComputedStyle-border-radius.html
test to WPT. 

I may also submit that Bugzilla-getComputedStyle-border-radius.html test to WPT; I did not consider doing so. I was mainly thinking, focusing about filing a bug report when I created such test. But now that you have brought up this question, it makes sense to submit it too.

Comment 4

[Gérard Talbot]

Emilio,

I hope to submit that Bugzilla-getComputedStyle-border-radius.html test under a different and more suitable filename to WPT this week.

As for
.../third_party/WebKit/LayoutTests/css3/calc/getComputedStyle-border-radius.html
it is not clear right now if and if so, when and how it will be submitted to WPT. But the initial desire is to submit it too.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Yup, thanks Gérard! I got somewhat driven away from this trying to clean up all the things, because the patch as-is was getting a few failures. Getting almost there :)

Comment 6

[Gérard Talbot]

The filename is

getComputedStyle-border-radius-002.html

and I have created a pull request to submit it to the web platform tests repository:

https://github.com/web-platform-tests/wpt/pull/14987

Integration tests have passed, so now, it is only a matter of getting a reviewer to merge the pull request.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Reworked patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=32ec210be10ed6975b026db097500e20b6a0320e

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1516454 - Simplify border-radius serialization. r=#style,boris

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1516454 - Make the generic size not use euclid under the hood. r=#style,boris

The euclid size is not really used for anything. Also rename it to Size2D to
avoid cbindgen conflicts with values::length::Size.

Depends on D20958

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1516454 - Use rust lengths for border corners. r=#style,boris

The test in https://github.com/web-platform-tests/wpt/pull/15423 hasn't been
synced over yet, but it passes with this patch of course.

Depends on D20959

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96e8b4cb4561
Simplify border-radius serialization. r=firefox-style-system-reviewers,boris
https://hg.mozilla.org/integration/autoland/rev/ec3876470b16
Make the generic size not use euclid under the hood. r=firefox-style-system-reviewers,boris

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43862dc87e0b
Use rust lengths for border corners. r=boris

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a460a4d0a66c
Add some braces on a CLOSED TREE. r=me

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/180c7672c57c
Remove an extra brace on a CLOSED TREE, whoops. r=me

Comment 15

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=c6ab8d3dd19a82d6202a3ef30b2aee3693ef0689&searchStr=linux%2Cx64%2Casan&selectedJob=230410238

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=230410238&repo=autoland&lineNumber=2738
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=230410447&repo=autoland&lineNumber=16807

Backout link: https://hg.mozilla.org/integration/autoland/rev/987a62667969a5f88f8c3f4693299e097db60daf

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Oh, it was a bit tricky to find the actual error from the log:

/builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ServoStyleConsts.h:1456:12: runtime error: load of value 228, which is not a valid value for type 'bool'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ServoStyleConsts.h:1456:12 in

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Well that's interesting. I guess I can do an ASAN build.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85f7a0580ce9
Use rust lengths for border corners. r=boris

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Oh blerg, of course, StyleBasicShape::mRadius gets compared unconditionally, but only initialized in the Inset case. I didn't realize that nsStyleCorners was PodZero'd

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/85f7a0580ce9

Comment 21

[Gérard Talbot]

https://wpt.fyi/results/css/css-values/getComputedStyle-border-radius-002.html?label=master

Thank you Emilio :)

Marking as VERIFIED