1665476 - LW-theme background images aren't shown

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Richard Marti (:Paenglab)]

LW-theme background images do not show any more.

Back-out of bug 1665064 fixes it for me.

Emilio, do you know why this happens? I CCed Magnus if you have questions about deeper functions.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

What are LW-theme background images? Can you attach an screenshot?

(No idea why off-hand, we should only skip painting backgrounds during print).

Comment 2

[Richard Marti (:Paenglab)]

Attachment: LW_theme.png

This screenshot shows the LW-theme "Dark Fox" applied. In front the smaller window how it should look and on background how it looks now.

Maybe this happens on TB because TB doesn't use electrolysis.

Comment 3

[Richard Marti (:Paenglab)]

I'm a complete noob in Cpp so I don't really know what I'm doing. With a back-out of only the changes in nsDisplayList.cpp the LW-theme image is shown again. A back-out of only the changes in nsCanvasFrame.cpp shows still the issue.

Emilio, not sure if that helps you.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Sorry for the lag in looking into this. So if I back out my patch, on a debug build I get:

Assertion failure: mFrame->IsCanvasFrame() || mFrame->IsFrameOfType(nsIFrame::eTablePart), at /home/emilio/src/moz/gecko-5/layout/painting/nsDisplayList.cpp:3355

So something is a bit off...

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1665476 - Remove nsRootBoxFrame to unify background propagation between XUL and non-XUL documents. r=mats

It's unused on mozilla-central, and Thunderbird can just use the canvas
frame as regular (X)HTML documents, so just use a canvas frame instead
of an nsRootBoxFrame for XUL as well.

nsRootBoxFrame was needed because of various XUL-specific things like
tooltips and so on lived there. But with the move away from XUL, that
functionality has been added to nsCanvasFrame already, behind a
principal check instead.

This also allows simplifying our background propagation setup, which was
only half-working for XUL documents (this bug is a consequence of that).

With this, most of the callers of nsCSSRendering::IsCanvasFrame can go.
They're only two of the frames that would return true for that that
actually paint backgrounds (nsCanvasFrame and nsRootBoxFrame), so the
codepaths in display list building and painting can just check
frame->IsCanvasFrame() instead.

The remaining caller to that function is
nsContainerFrame::SyncWindowProperties, and the change is also legit, in
the sense that the only thing SyncWindowProperties() really cares about
is propagating the max/min-width constraints from the root element's
style to the view/widget, and the only frame that would return true from
IsCanvasFrame and have a view is the viewport frame which is the root of
the frame tree.

Comment 6

[Richard Marti (:Paenglab)]

I tried your patch on Linux and Windows on TB.

• On both platforms the Add-on manager fills only the top fifth of the window.
• On both platforms the DevTools Toolbox (Browser Toolbox on FX) fills also only the top fifth of the window.
• On Linux the theme background image is displayed but with the dark or light theme the titlebar and tabbar are transparent.
• On Windows first the theme background looked good but after a restart of TB the whole window is transparent. Also in safe mode no change. The safe mode dialog is also transparent but the text is still visible.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Alright, I'll look into that, there's also a bit of orange in my try push.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

The updated patch should be much closer if you want to give it a run. I still need to fix one or two existing xul tests, but won't get to that today.

Comment 9

[Richard Marti (:Paenglab)]

On Linux all is looking good but Windows still shows a completely transparent window. Absolute nothing is visible. When I start in safe mode, the safe mode dialog is shown with the content but no window decoration (no titlebar, no window border).

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Can you check on a debug build to see if there's any assertion going on?

Comment 11

[Richard Marti (:Paenglab)]

Attachment: console.log

I attached the log when starting with mach run. Before I built without the patch and correlated both outputs. No additional assertions are in the log.
But with your patch I see this line:
[GFX2-]: ASurface Init failed with Cairo status 1 on 00007FFDB7D0A660
and this:
WARNING: GetImageSurface on an invalid (null) surface; who's calling this without checking for surface errors?: file z:/Mozilla/comm-central/gfx/thebes/gfxWindowsSurface.cpp, line 149

Comment 14

[Richard Marti (:Paenglab)]

Emilio, can my log and comment 11 help you find the issue?

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(In reply to Richard Marti (:Paenglab) from comment #14)

Emilio, can my log and comment 11 help you find the issue?

Yes, I know what the issue is already, it was just harder to fix than it seemed, I just got all the dependencies sorted out and can try to land this again.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1665476 - Handle XUL root frame backgrounds explicitly in AppendBackgroundItemsToTop. r=mats

My patch somehow causes WR to crash and fall back to d2d on Windows, see
bug 1671591.

While I figure that out let's fix TB by restoring the previous behavior
for XUL.

Rename the Root frame type because it's super-confusing that IsRootFrame
is not equivalent to !GetParent().

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Can you confirm comment 16 fixes it for you too? The proper fix will take a bit more because of bug 1671591 and me being sick this week :(

Comment 18

[Richard Marti (:Paenglab)]

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

Can you confirm comment 16 fixes it for you too? The proper fix will take a bit more because of bug 1671591 and me being sick this week :(

Yes, this fixes the background image issue. Many thanks.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cafeb3cc35d9
Handle XUL root frame backgrounds explicitly in AppendBackgroundItemsToTop. r=mats

Comment 20

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/cafeb3cc35d9

Comment 23

[Emilio Cobos Álvarez (:emilio)]

It seems like nowadays the WR failure is much noisier, and I should be able to debug it better... https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=E-0RYZy_SR6LwPIc6gjgGA.0&revision=92edd731e64cc448719e8dac6ae573efda121463

Comment 24

[Wayne Mery (:wsmwk)]

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

It seems like nowadays the WR failure is much noisier, and I should be able to debug it better... https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=E-0RYZy_SR6LwPIc6gjgGA.0&revision=92edd731e64cc448719e8dac6ae573efda121463

Bug 1671591 is P3, so perhaps won't get attention soon.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

I checked and bug 1671591 doesn't seem to keep happening, so I'll rebase and reland.

Comment 26

[Alexandru Michis [:malexandru]]

Backed out changeset e41bfdf79fb8 (Bug 1665476) for causing windows mochitest failures in test_windowminmaxsize.xhtml
Backout link: https://hg.mozilla.org/integration/autoland/rev/54f245b73e54063142fb0d7e1a493401f6389cfd
Push with failures, failure log.
(Update): Looks to have also caused bc failures.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/503c84054f68
Remove nsRootBoxFrame to unify background propagation between XUL and non-XUL documents. r=layout-reviewers,mats

Comment 28

[Atila Butkovits]

Backed out for causing mochitest mass failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/8280e8cef3ae5f2028521ace665f63c8879d1084

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=Q9tPVscCRXGwn7CuaAoKwA.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=503c84054f683f305d4b02ffce4c9ea96ace860c

Failure logs:
https://treeherder.mozilla.org/logviewer?job_id=391550429&repo=autoland&lineNumber=8719
https://treeherder.mozilla.org/logviewer?job_id=391548502&repo=autoland&lineNumber=7757

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37180b0bf14a
Remove nsRootBoxFrame to unify background propagation between XUL and non-XUL documents. r=layout-reviewers,mats

Comment 30

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/37180b0bf14a