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

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P3
normal
a year ago
9 months ago

People

(Reporter: hiro, Assigned: mantaroh)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 affected)

Details

(Reporter)

Description

a year ago
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
P3 unless this is actually creating blind spots in our test coverage.
Priority: -- → P3
(Reporter)

Comment 2

10 months ago
Note that this is not specific for stylo, this does also affects gecko.
No longer blocks: 1288269
Summary: stylo: reftest-print mode doesn't work with animations → reftest-paged mode doesn't work with animations

Comment 3

9 months ago
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!
status-firefox57: --- → wontfix
status-firefox58: --- → affected
Flags: needinfo?(bbirtles)
(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.
Summary: reftest-paged mode doesn't work with animations → Make print-no-animations.html test the printed output
Mantaroh, would you be able to look at this?
Flags: needinfo?(bbirtles) → needinfo?(mantaroh)
(Assignee)

Comment 6

9 months ago
(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.
Assignee: nobody → mantaroh
Flags: needinfo?(mantaroh)
(Assignee)

Comment 7

9 months ago
(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
You need to log in before you can comment on or make changes to this bug.