Closed Bug 1394302 Opened 2 years ago Closed 2 years ago
stylo: using calc() in font-size in animation produces inconsistent and wrong results
760 bytes, text/html
59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
41 bytes, text/x-github-pull-request
|Details | Review|
See attached test case. Using 'font-size: calc(110% + 0.1em)' with a base font-size of 10px initially gives the correct font-size of 12px. After updating the base font-size to 20px, however, 23.2px is returned (for me anyway, running on Linux). However, if you comment-out the first call to getComputedStyle(), 24px is returned. With other combinations of content you can get 23.2167px. I came across this when trying get a similar SMIL test case to work where I get 23.2167px the first time I call getComputedStyle and then 24.3167px the next time.
Reduced test case to remove use of SVG and mapped attributes.
Attachment #8901664 - Attachment is obsolete: true
This *may* be related to the fact that we don't deal with calcs applied to keyword-derived font sizes, but I doubt it since kw/default font sizes aren't involved here.
Oh, I think I know what happens. We don't construct the correct compute context. By default the context's font-size keyword info is set to medium, a keyword font size, and this inherits and is cleared when you do an explicit size. We probably construct the wrong context when animated, because we've lost the info as to where the font size comes from. It might be worth moving the stored base-size factor/keyword info in the style structs themselves. It's something I've wanted to do for a while, though I planned to do it by simultaneously removing SetGenericFont so that Gecko can use the framework too. This would probably simplify the Servo code dealing with this.
Thanks for looking into this Manish! You are very very welcome to take this bug :)
Ok, this has nothing to do with the stored base size, that in fact is persistent since it ends up on the nsStyleContext (so it's ugly, but not a correctness issue). We still have the calc bug there but it's unrelated to animations. The problem is that we unconditionally calculate calcs with em units against the current style, with no option to use the inherited style in the case of font-size. We should just run a special calc computation routine for font-size.
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Do you have a reliable testcase that can be turned into a reftest? Or should we just mochitest this?
Comment on attachment 8902415 [details] Bug 1394302 - stylo: Compute font-size calcs against appropriate base size; https://reviewboard.mozilla.org/r/173996/#review179328 Great, thanks for doing this. I've checked the test case where this first showed up and it appears to be fixed with this (and some other local patches I'm working on).
Attachment #8902415 - Flags: review?(bbirtles) → review+
(In reply to Manish Goregaokar [:manishearth] from comment #7) > Do you have a reliable testcase that can be turned into a reftest? Or should > we just mochitest this? No reftest, I'm afraid. Only mochitests. I tried making one, but the font-size updates on the next animation frame and I'm not confident we could get the reftest snapshot to occur before the next animation frame.
Manish, were you planning on landing this? Or do you need help preparing a test?
I am. Just haven't gotten around to doing the test. Will tomorrow or friday. (But if you want to do that feel free)
Ok, great. I'm heading home now but if you haven't got to it by tomorrow and I have some time, I'll have a look at it.
Attachment #8903391 - Flags: review?(bbirtles) → review+
It'd be nice if this was a WPT test fwiw.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #15) > It'd be nice if this was a WPT test fwiw. NVM me, I see why that's not as trivial as I thought.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/820cb0437789 stylo: Add mochitest; r=birtles
Backed out for failing new test, at least on Linux x64: https://hg.mozilla.org/integration/autoland/rev/7a5f1092920acfd6bf70e055e40c96084185f258 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=127964546&repo=autoland [task 2017-09-02T07:32:45.055502Z] 07:32:45 INFO - TEST-START | layout/style/test/test_bug1394302.html [task 2017-09-02T07:32:45.198219Z] 07:32:45 INFO - TEST-INFO | started process screentopng [task 2017-09-02T07:32:47.095667Z] 07:32:47 INFO - TEST-INFO | screentopng: exit 0 [task 2017-09-02T07:32:47.097691Z] 07:32:47 INFO - TEST-UNEXPECTED-FAIL | layout/style/test/test_bug1394302.html | undefined assertion name - got "19.2px", expected "12px" [task 2017-09-02T07:32:47.098833Z] 07:32:47 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:312:5 [task 2017-09-02T07:32:47.103546Z] 07:32:47 INFO - @layout/style/test/test_bug1394302.html:26:1
Is it possible that this test is fuzzy? It seemed to have passed when I landed it.
Flags: needinfo?(manishearth) → needinfo?(bbirtles)
It didn't run for the push which landed this, at least on Linux x64 opt and debug. See opt as example: https://treeherder.mozilla.org/#/jobs?repo=autoland&tochange=7a5f1092920acfd6bf70e055e40c96084185f258&fromchange=2047237ce098815c66e3c59f39b1a9af6221da1d&selectedJob=127964546&filter-searchStr=5c04f21b0ccc5c12adbc5561dd3f044335315d8c
The failure reason on gecko is that we don't recompute keyframe values when the parent style is changed (bug 1254424). I think we can skip this test on gecko with annotating the bug number. Also, IIUC if we wrote a test case using SMIL, it passes on gecko, but it will not pass on stylo until bug 1358955 is fixed.
Yes, I agree we can just skip this for Gecko.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/e36fd3453c28 stylo: Add mochitest; r=birtles
Adding link to the PR for posterity
Also, the code changes have landed in m-c so this is fixed: https://hg.mozilla.org/mozilla-central/rev/3aada22a678f9826ccc1952b927340266f26c178
You need to log in before you can comment on or make changes to this bug.