Closed
Bug 1394302
Opened 7 years ago
Closed 7 years ago
stylo: using calc() in font-size in animation produces inconsistent and wrong results
Categories
(Core :: CSS Parsing and Computation, enhancement, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: birtles, Assigned: manishearth)
Details
Attachments
(4 files, 1 obsolete file)
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.
Reporter | ||
Comment 1•7 years ago
|
||
Reduced test case to remove use of SVG and mapped attributes.
Attachment #8901664 -
Attachment is obsolete: true
Assignee | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
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.
Reporter | ||
Comment 4•7 years ago
|
||
Thanks for looking into this Manish! You are very very welcome to take this bug :)
Assignee | ||
Comment 5•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → manishearth
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•7 years ago
|
||
Do you have a reliable testcase that can be turned into a reftest? Or should we just mochitest this?
Reporter | ||
Comment 8•7 years ago
|
||
mozreview-review |
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+
Reporter | ||
Comment 9•7 years ago
|
||
(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.
Reporter | ||
Comment 10•7 years ago
|
||
Manish, were you planning on landing this? Or do you need help preparing a test?
Flags: needinfo?(manishearth)
Assignee | ||
Comment 11•7 years ago
|
||
I am. Just haven't gotten around to doing the test. Will tomorrow or friday. (But if you want to do that feel free)
Flags: needinfo?(manishearth)
Reporter | ||
Comment 12•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Reporter | ||
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8903391 [details] Bug 1394302 - stylo: Add mochitest; https://reviewboard.mozilla.org/r/175228/#review180222 r=me assuming you've verified this test fails without the code changes from this bug. ::: layout/style/test/test_bug1394302.html:8 (Diff revision 1) > +<!-- > +https://bugzilla.mozilla.org/show_bug.cgi?id=1394302 > +--> > +<head> > + <title>Test for Bug 1394302</title> > + <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> Nit: trailing whitespace here
Attachment #8903391 -
Flags: review?(bbirtles) → review+
Comment 15•7 years ago
|
||
It'd be nice if this was a WPT test fwiw.
Comment 16•7 years ago
|
||
(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.
Comment 17•7 years ago
|
||
Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/820cb0437789 stylo: Add mochitest; r=birtles
Comment 18•7 years ago
|
||
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
Flags: needinfo?(manishearth)
Assignee | ||
Comment 19•7 years ago
|
||
Is it possible that this test is fuzzy? It seemed to have passed when I landed it.
Flags: needinfo?(manishearth) → needinfo?(bbirtles)
Comment 20•7 years ago
|
||
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
Comment 21•7 years ago
|
||
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.
Reporter | ||
Comment 22•7 years ago
|
||
Yes, I agree we can just skip this for Gecko.
Flags: needinfo?(bbirtles)
Comment 23•7 years ago
|
||
Pushed by manishearth@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e36fd3453c28 stylo: Add mochitest; r=birtles
Reporter | ||
Comment 24•7 years ago
|
||
Adding link to the PR for posterity
Reporter | ||
Comment 25•7 years ago
|
||
Also, the code changes have landed in m-c so this is fixed: https://hg.mozilla.org/mozilla-central/rev/3aada22a678f9826ccc1952b927340266f26c178
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 26•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e36fd3453c28
You need to log in
before you can comment on or make changes to this bug.
Description
•