Closed Bug 1726663 Opened 4 months ago Closed 3 months ago

Screenshots do not include some fixed-positioned elements

Categories

(Core :: Graphics, defect)

defect

Tracking

()

RESOLVED FIXED
94 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- fixed
firefox91 --- wontfix
firefox92 --- wontfix
firefox93 --- fixed
firefox94 --- fixed

People

(Reporter: kevin, Assigned: mattwoodrow)

References

(Regression, )

Details

(Keywords: regression)

Attachments

(4 files)

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.

Has Regression Range: --- → yes
Regressed by: 1540737
Component: Screenshots → Graphics
Product: Firefox → Core
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)

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.

Pushed by mwoodrow@mozilla.com:
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
Status: UNCONFIRMED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch

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.

Flags: needinfo?(matt.woodrow)

Would that fix be safe to uplift in 93? Thanks

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
Flags: needinfo?(matt.woodrow)
Attachment #9240179 - Flags: approval-mozilla-beta?

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.

Attachment #9240179 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Please nominate this for ESR91 approval also.

Flags: needinfo?(matt.woodrow)
Flags: in-testsuite+

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:
Flags: needinfo?(matt.woodrow)
Attachment #9240179 - Flags: approval-mozilla-esr91?

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

Flags: needinfo?(matt.woodrow)

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.

Flags: needinfo?(matt.woodrow)
Attachment #9240179 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+

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

Flags: needinfo?(matt.woodrow)
Attached patch esr-diffSplinter Review
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.