1726663 - Screenshots do not include some fixed-positioned elements

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Kevin Locke]

Attachment: Full page screenshot taken with Firefox 89 (good)

On Firefox 90 and later (including Nightly 93.0a1 (2021-08-19)), screenshots do not include descendants of elements with position: fixed on some pages. For example:

1. Navigate to https://mobile.twitter.com/i/spaces/1mnxeaAyywbxX
2. Right-click anywhere on the page and click "Take Screenshot"
3. Click "Save full page" (or "Save visible", or select an area)
4. Observe that the screenshot appears very different from the page.

The linked page contains a fullscreen modal with position: fixed showing an error message. The screenshot only includes the content behind the modal (the mostly-blank Twitter UI).

Mozregression:
Last good revision: d7a58cff3e920dfbfdfe29c76a2822f7b916c0d3
First bad revision: 411d3031b1b4c0f8b9ff3009ceb21ba4cfdc601e
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d7a58cff3e920dfbfdfe29c76a2822f7b916c0d3&tochange=411d3031b1b4c0f8b9ff3009ceb21ba4cfdc601e

Suggesting it was regressed by Bug 1540737.

Note: This issue is similar to Bug 1646063 and Bug 1708403. However, both of those involve the scroll position and were present in earlier Firefox versions. In this example, the linked page can not be scrolled.

Comment 1

[Kevin Locke]

Attachment: Full page screenshot taken with Firefox 90 (bad)

Comment 2

[Matt Woodrow (:mattwoodrow)]

The bug occurs when we have nested position:fixed elements, and the outer one has an empty size (apart from the inner position:fixed, which doesn't contribute to the size/overflow areas of the outer).

Normally, we fixup this after display list building by nsDisplayWrapList::UpdateBounds running a fixup to find the descendants which didn't contribute overflow area and add them to the building rect.

This fails if there's an intermediate container item, which doesn't implement UpdateBounds. nsDisplayContainer is the problem in this case.

It looks like this bug with invalid building rects has existed for a long time, but the changes to screenshot painting meant we started using the invalid value to clip our draw area.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1726663 - Don't restrict fallback painting to the building rect, since it can be incorrect and is going to be removed soon. r?miko

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0bedadc1ee1b
Don't restrict fallback painting to the building rect, since it can be incorrect and is going to be removed soon. r=miko

Comment 5

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/0bedadc1ee1b

Comment 6

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

The patch landed in nightly and beta is affected.
:mattwoodrow, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

Would that fix be safe to uplift in 93? Thanks

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9240179 [details]
Bug 1726663 - Don't restrict fallback painting to the building rect, since it can be incorrect and is going to be removed soon. r?miko

Beta/Release Uplift Approval Request

• User impact if declined: Incorrect screenshots in some rare cases
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very low risk, change only affects screenshot painting
• String changes made/needed: None

Comment 9

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9240179 [details]
Bug 1726663 - Don't restrict fallback painting to the building rect, since it can be incorrect and is going to be removed soon. r?miko

Low risk fix with tests for a rather recent functional regression, approved for 93 beta 6, thanks.

Comment 10

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b14d4cddde76

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR91 approval also.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9240179 [details]
Bug 1726663 - Don't restrict fallback painting to the building rect, since it can be incorrect and is going to be removed soon. r?miko

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Can affect screenshots of some twitter pages
• User impact if declined: Incorrect screenshot rendering for some pages
• Fix Landed on Version: 94
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very simple change, low risk.
• String or UUID changes made by this patch:

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

The patch does not graft cleanly to the ESR branch, could you provide an updated patch? Thanks

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9240179 [details]
Bug 1726663 - Don't restrict fallback painting to the building rect, since it can be incorrect and is going to be removed soon. r?miko

Approved for 91.2esr.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/7243bddf915e

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Or not. Looks like the conflicts are due to bug 1728498 and the trivial attempt at rebasing caused build bustage. Backed out.
https://hg.mozilla.org/releases/mozilla-esr91/rev/5d4ad120b8d41af6d9452a0572e774865f110e0e

Comment 17

[Matt Woodrow (:mattwoodrow)]

Attachment: esr-diff

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/8d12483380d1