1803094 - A web article's image is initially displayed in bad quality in print preview

Status: VERIFIED FIXED

(Core :: Print Preview, defect)

Description

[Daniel Bodea [:danibodea]]

Attachment: images are printed with bad quality.png

Note

• When the user loads some web articles and print previews them, he will notice that the image is displayed in severely bad quality. When changing a print option and forcing the print preview to reload, the image quality is corrected.

Found in

• 108.0b7

Affected versions

• Beta 108.0b7
• Release v107.0

Tested platforms

• Affected platforms: Windows 7
• Unaffected platforms: ?

Steps to reproduce

1. Load https://www.vogue.co.uk/arts-and-lifestyle/article/portia-white-lotus-style
2. Print preview

Expected result

• The image is displayed in original quality

Actual result

• The image is displayed in bad quality

Regression range

• will be provided

Additional notes

• The PDF does not print the article with bad-quality images, only print preview is affected.

Comment 1

[Jim Mathies [:jimm]]

Can reproduce on Windows. jwatt, this feels polishy, but also pretty annoying. Might want to take a look. Clearing Severity to get it into triage lists.

Comment 2

[Daniel Bodea [:danibodea]]

Mozregression investigation logs:
Bug 1787671 - Trigger Document::NotifyMediaFeatureValuesChanged() synchronously. r=emilio

We generate the nsPresContext for the cloned document after cloning the
elements, for printing. This makes the updating of the responsive source
failed because we don't have nsPresContext when binding it to the cloned
doucment tree. Therefore, we hit the assertion in the microtask which
trying to load the image for the cloned HTMLImageElement, after dropping
nsAutoScriptBlocker.

This can be avoided by notifying the media feature values changed for
HTMLImageElement synchronously, and so we update the responsive source
once its nsPresContext is created, rather than waiting for flushing the
pending media feature values changed, which may happen after the microtasks.

Differential Revision: https://phabricator.services.mozilla.com/D156020

Comment 3

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

Set release status flags based on info from the regressing bug 1787671

Comment 4

[Boris Chiou [:boris]]

Attachment: Simplified test case

Comment 5

[Boris Chiou [:boris]]

Well, after checking the review comments in my previous patch: https://phabricator.services.mozilla.com/D156020?vs=on&id=620863#toc (note: no idea to get a link to this inline comment. :( ), a simple way to fix this is just to call mDocument->NotifyMediaFeatureValuesChanged(); again in nsPresContext::FlushPendingMediaFeatureValuesChanged(). This only reselects responsive image source so it shouldn't be expensive.

Therefore, we notify the media feature values changed of this document twice. The 1st one is a synced call to make sure we choose the correct source when creating the preshell. However, at this moment, the new-created presentation context doesn't have the correct resolution we want. So it is still necessary to update the responsive image source again in FlushPendingMediaFeatureValuesChanged(), to re-select the image with correct resolution.

Comment 6

[Boris Chiou [:boris]]

Or we just take the 1st suggestion from this comment (because the 2nd one causes this bug):

1. It might be that we need to relax the assertion to allow for having a pres context with pending media feature values changed, since those are processed async.
2. Alternatively we should move the NotifyMediaFeatureValuesChanged to nsPresContext::MediaFeatureValuesChanged rather than nsPresContext::FlushPendingMediaFeatureValuesChanged.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Shouldn't changing the resolution trigger the media feature change as well?

Comment 8

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

Shouldn't changing the resolution trigger the media feature change as well?

I may be wrong. It may not be caused by a different resolution. I tried to debug more:

I just realize the image select algorithm use ResponsiveImageSelector::ComputeFinalWidthForCurrentViewport() to get the computed width from the styleset and use this value to select the image.

So in the current implementation, our sync call of mDocument->NotifyMediaFeatureValuesChanged() in nsPresContext::MediaFeatureValuesChanged() happens just when creating a new preshell. And I notice its computedWidth is 0 at this moment, per my attached test case. So the image select algorithm uses an incorrect candidate density.

If we call mDocument->NotifyMediaFeatureValuesChanged() in nsPresContext::FlushPendingMediaFeatureValuesChanged() (just like what our previous implementation did), its computedWidth is a valid value (e.g. 720 in my attached test case), so the candidate density is correct and we choose the correct source.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

We should make sure that the pres context has the viewport size set up before calling Document::CreatePresShell instead.

That is, this line needs to probably go before this line.

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1803094 - Create PreShell later than the setup of viewport size when reflowing the print job.

Basically, we notify the media feature values changed when creating a
new preshell. For the print job, we have to set the page size, the
visible area, and set mIsRootPaginatedDocument before doing this
notification, so we can get the correct viewport size and use it to select
the correct responsive image source, in print preview.

Note: This shouldn't have impact on print result, so we don't have a
test for this, and need a manual verification.

Comment 11

[Boris Chiou [:boris]]

Attachment: Bug 1803094 - Add print tests to wpt.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77d18a1860b7
Create PreShell later than the setup of viewport size when reflowing the print job. r=emilio
https://hg.mozilla.org/integration/autoland/rev/3673d2e25da1
Add print tests to wpt. r=emilio

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/37635 for changes under testing/web-platform/tests

Comment 14

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/77d18a1860b7
https://hg.mozilla.org/mozilla-central/rev/3673d2e25da1

Comment 15

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

The patch landed in nightly and beta is affected.
:boris, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox109 to wontfix.

For more information, please visit auto_nag documentation.

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 17

[Boris Chiou [:boris]]

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #15)

The patch landed in nightly and beta is affected.
:boris, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox109 to wontfix.

For more information, please visit auto_nag documentation.

This doesn't affect the real print result, so should be ok without uplift.

Comment 18

[Ardelean Oana, Desktop QA [:oardelean]]

Reproducible with a 2022-11-29 Nightly build on macOS 12. Verified as fixed on Nightly 110.0a1(build ID: 20230110214526) on macOS 12, Windows 10, Ubuntu 22.