1374154 - Make print-no-animations.html test the printed output

Status: NEW

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

In Gecko_UpdateAnimations, we are skipping updating animations if the nsPresContext::IsDynamic() returns false [1]. But even in reftest-print mode IsDynamic() returns true,  so we are trying to update animations there.  In a genuine print-preview mode, IsDynamic() returns false though.

[1] https://hg.mozilla.org/mozilla-central/file/95543bdc59bd/layout/style/ServoBindings.cpp#l599

Comment 1

[Brian Birtles (:birtles)]

P3 unless this is actually creating blind spots in our test coverage.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Note that this is not specific for stylo, this does also affects gecko.

Comment 3

[Jet Villegas (inactive)]

Per bug 1382327, these tests test fragmentation/pagination, not printing. It's not clear to me what nsPresContext::IsDynamic() should return in these cases where we fragment across "pages" for screen viewing and not paper printing. 

Brian: can you review these tests and recommend next steps for figuring that out? Thx!

Comment 4

[Brian Birtles (:birtles)]

(In reply to Jet Villegas (:jet) from comment #3)
> Per bug 1382327, these tests test fragmentation/pagination, not printing.
> It's not clear to me what nsPresContext::IsDynamic() should return in these
> cases where we fragment across "pages" for screen viewing and not paper
> printing. 

If it's not for printing, then IsDynamic should return true in this case. That would suggest that the bug described in comment 0 is no longer a bug.

However, I think the underlying bug here is that we are not testing printing with animations. Specifically this test:

  layout/reftests/css-animations/print-no-animations.html

doesn't actually test what it was intended to (as indicated in reftest.list)

Now, that bug 1299848 has landed I believe we are able to test actual print output so we should repurpose this bug to make print-no-animations.html test print output using the reftest 'print' annotation.

Comment 5

[Brian Birtles (:birtles)]

Mantaroh, would you be able to look at this?

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Brian Birtles (:birtles, busy with MDN roadshow) from comment #5)
> Mantaroh, would you be able to look at this?

Ok, I will take a look at this bug.

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Brian Birtles (:birtles) from comment #4)
> (In reply to Jet Villegas (:jet) from comment #3)
> > Per bug 1382327, these tests test fragmentation/pagination, not printing.
> > It's not clear to me what nsPresContext::IsDynamic() should return in these
> > cases where we fragment across "pages" for screen viewing and not paper
> > printing. 
> 
> If it's not for printing, then IsDynamic should return true in this case.
> That would suggest that the bug described in comment 0 is no longer a bug.
> 
> However, I think the underlying bug here is that we are not testing printing
> with animations. Specifically this test:
> 
>   layout/reftests/css-animations/print-no-animations.html
> 
> doesn't actually test what it was intended to (as indicated in reftest.list)
> 
> Now, that bug 1299848 has landed I believe we are able to test actual print
> output so we should repurpose this bug to make print-no-animations.html test
> print output using the reftest 'print' annotation.

This test was introduced in bug 1299848. According to this commit message[1], reftest harness doesn't currently make the pres context non-dynamic. So we should change the 'print type' for purpose of it.
However, reftests can't compare this test as 'print type' since reftests doesn't check pixel, reftests of print only check the printed page number and output string using by pdf.js.[2]
We should change this test to comparable test like using to display:none animation.

[1] https://hg.mozilla.org/mozilla-central/rev/14b9894272cb
[2] http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/layout/tools/reftest/reftest.jsm#2411-2469

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.