1334680 - Box shadows missing on block edges of pages in print preview (except first/last page)

Status: VERIFIED FIXED

(Core :: Layout, defect)

Comment 1

[Neerja Pancholi[:neerja]]

Attachment: Bug 1334680 - Use the same box shadow on every page in print preview.

Review commit: https://reviewboard.mozilla.org/r/107894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107894/

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8831331 [details]
Bug 1334680 - Use the same box shadow on every page in print preview.

https://reviewboard.mozilla.org/r/107894/#review109080

r=me, though one thing to keep an eye out for:

::: layout/style/res/ua.css:258
(Diff revision 1)
> +  box-decoration-break: clone;
>    margin: 0.125in 0.25in;
>  }

Hmm, so technically this will make us reapply this "margin" on every pair of pages, too (.125in)

This might not make a difference in practice, because maybe we do Print Preview's between-page-spacing using some other mechanism (not this "margin" property) anyway.  (If we only depended on this margin to separate the print-preview pages, then the pages would be smashed into each other in current builds due to the issue that you're fixing here.  And they're not smashed into each other, so there must be something else handling the separation/placement.)

But do keep an eye out for unexpected extra space between pages in your reftests, after this change. If there is a noticeable increase from this and it's awkwardly large, we might want to reduce this margin a bit.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8831331 [details]
Bug 1334680 - Use the same box shadow on every page in print preview.

https://reviewboard.mozilla.org/r/107894/#review109080

> Hmm, so technically this will make us reapply this "margin" on every pair of pages, too (.125in)
> 
> This might not make a difference in practice, because maybe we do Print Preview's between-page-spacing using some other mechanism (not this "margin" property) anyway.  (If we only depended on this margin to separate the print-preview pages, then the pages would be smashed into each other in current builds due to the issue that you're fixing here.  And they're not smashed into each other, so there must be something else handling the separation/placement.)
> 
> But do keep an eye out for unexpected extra space between pages in your reftests, after this change. If there is a noticeable increase from this and it's awkwardly large, we might want to reduce this margin a bit.

Update: I just tested stock Nightly against a build with this patch applied, and I'm not seeing any new space being introduced between pages -- just nicer shadows.  So that's good. (And I guess we're not actually relying on this "margin" property for page-separation right now. *shrug*.)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2492fd15497c
Use the same box shadow on every page in print preview. r=dholbert

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: screenshot of buggy inter-page shadows

For reference / posterity, here's a screenshot of the buggy print-preview issue that's being fixed in this bug.

The start/end box-shadows between pages were being awkwardly & abruptly clipped, in a way that disrupts the "page floating above a gray background" illusion.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: screenshot of expected rendering (taken from final page)

...and here's how the shadow's expected to look. Before the fix here, only the final page would have a bottom-shadow like this.  Now all of the pages will look like this. (Yay!)

Comment 7

[Daniel Holbert [:dholbert]]

And for the record, this blocks bug 1166147 because it makes us render reftest-print shadows reliably regardless of their writing-mode, which we need for bug 1166147's reftests.  (Without this fix, we skip block-start/block-end shadows, and those skipped shadows are on different sides depending on the writing-mode, as of bug 1166147.)

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2492fd15497c

Comment 9

[Ciprian Georgiu, Desktop QA]

Reproduced this issue using info from comment 6 and comment 7 on an old Nightly build.

The box shadow is properly displayed on pages in print preview with 54 beta 2. Tested on Windows 7 x64 and Ubuntu 16.04 x64 LTS.