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)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: birtles, Assigned: manishearth)

Details

Attachments

(4 files, 1 obsolete file)

Attached file Test case (obsolete) —
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?
Flags: needinfo?(manishearth)
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)
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 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+
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.
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)
Is it possible that this test is fuzzy? It seemed to have passed when I landed it.
Flags: needinfo?(manishearth) → needinfo?(bbirtles)
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.
Flags: needinfo?(bbirtles)
Attached file Servo PR #18335
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.