Open Bug 1592739 Opened 7 months ago Updated 6 days ago

macOS vibrancy doesn't work in WebRender + OS compositor configuration

Categories

(Core :: Web Painting, enhancement, P2)

All
macOS
enhancement

Tracking

()

ASSIGNED
mozilla72
Tracking Status
firefox72 --- disabled
firefox73 --- disabled
firefox74 --- wontfix
firefox75 --- affected

People

(Reporter: mstange, Assigned: mstange)

References

(Depends on 2 open bugs, Blocks 2 open bugs, Regressed 1 open bug)

Details

Attachments

(11 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
40.26 KB, text/plain
Details
40.25 KB, patch
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review

With the current default Webrender configuration (i.e. no OS compositor integration), vibrancy works as follows:

  • The window contains an opaque CALayer for the window background color.
  • Then there's the CALayer from a vibrant NSVisualEffectsView. This knocks out the window background color.
  • Then there's a full-window CALayer with WebRender's rendering. Within this layer, OpenGL painting does the following:
    • First, it draws a full-window sized opaque window background color.
    • Then it draws the web content.
    • Then it draws a "clear rect" primitive, which erases the window background color drawing in the tab bar again.
    • Then it draws the content of the tab bar and the rest of the chrome on top.

The "clear rect" approach is hard to make work with WR OS compositor surfaces. The semantics of the "clear rect" primitive ask for the pixels to be cleared across the entire stack of WebRender rendering, which can consist of multiple overlapping OS compositor surfaces.
It would be much simpler for WebRender if there was no "erasing". Instead of drawing a background color and then clearing it, we should just not draw that background color.
And the easiest way to achieve that is probably to change the front-end CSS so that there's no CSS background-color definition on the root element of the window document.

So I think what I'll do is the following:

  • Remove the CSS background-color from the stylesheet and replace it with -moz-appearance: dialog. (Reverting bug 461366, which is now 11 years old.)
  • On macOS, treat -moz-appearance: dialog similarly to how -moz-appearance: -moz-win-glass is treated on Windows 7: Make Gecko leave the background transparent, but make sure the OS window doesn't actually turn transparent.
  • At this point there will be nothing to erase anymore, and we can get rid of the "clear rect" display items.
Depends on: 1593150

This might actually be a little more complicated when a lightweight theme is selected. At the moment, lightweight themes cause us to set -moz-appearance: none; background-color: var(--lwt-accent-color); on the root element. With those properties, we can't have any vibrancy in the window under the new model. If we could set those properties on the toolbox instead, and leave the window with -moz-appearance: dialog, then we can still have vibrant sidebars. But maybe we don't want vibrant sidebars if a theme is selected anyway.

On macOS, the OS window always comes with an opaque background for top level
windows. This is the case even if Gecko determines the root element of the
window to be transparent: Ever since bug 1162649, nsChildView/nsCocoaWindow
ignore calls to SetTransparencyMode for top level windows and always stay opaque.

Returning true from nsChildView::WidgetPaintsBackground() lets us indicate that
we do not need an opaque backstop color to be added at the bottom of the display
list. This backstop color would interfere with vibrant -moz-appearance rendering
under the new vibrancy model.
WidgetPaintsBackground() is only called in one place, in ComputeBackstopColor():

nscolor PresShell::ComputeBackstopColor(nsView* aDisplayRoot) {
  nsIWidget* widget = aDisplayRoot->GetWidget();
  if (widget && (widget->GetTransparencyMode() != eTransparencyOpaque ||
                 widget->WidgetPaintsBackground())) {
    // Within a transparent widget, so the backstop color must be
    // totally transparent.
    return NS_RGBA(0, 0, 0, 0);
  }
  // Within an opaque widget (or no widget at all), so the backstop
  // color must be totally opaque. The user's default background
  // as reported by the prescontext is guaranteed to be opaque.
  return GetDefaultBackgroundColorToDraw();
}

On Windows 7, the widget returns eTransparencyBorderlessGlass from
GetTransparencyMode(), which also avoids the backstop color.

Depends on D51457

For regular elements, whenever -moz-appearance is used, the CSS background is
ignored. Root elements were behaving specially, and the background color also
needed to be adjusted.
For example, for Windows 7, we have the following CSS rule;

    :root {
      background-color: transparent;
      -moz-appearance: -moz-win-borderless-glass;
    }

This change makes the root element more consistent with other elements, so the
extra background-color: transparent declaration is no longer necessary.

This change does not let content documents opt out of forced opaqueness:
Root content documents still get an opaque background color from an existing
check further down in this method.

Depends on D51458

This is preferable over a hardcoded color because it lets Gecko choose where the
window background should come from. We would like the background to be handled
by the OS widget, and prevent Gecko from painting a CSS background color.

Depends on D51459

The window background will be contributed by the widget itself, which renders
them underneath Gecko's rendering.
As a result, -moz-appearance: dialog is now equivalent to the combination
-moz-appearance: none; background-color: transparent.

This change does not turn the widget itself transparent because nsCocoaWindow
does not allow top level windows to become transparent (ever since bug 1162649).
If we ever add support for top level widgets with transparent backgrounds again,
we will probably want to treat -moz-appearance: dialog differently from
-moz-appearance: none; background-color: transparent, but for now this is fine.

This change means that Gecko's rendering will go into transparent buffers. This
may result in some loss of subpixel AA in various cases.
In the main browser window, there are CSS backgound colors that cover all the
non-vibrant areas of the window, so in that window we still render mostly onto
opaque pixels.

Depends on D51460

With -moz-appearance: dialog now always being transparent, setting up our own
vibrant views for sheet windows is no longer necessary. We now let the regular
sheet window background show through, and that background is already vibrant.

Depends on D51461

Now that there is no Gecko-contributed background color in the window any more,
there's nothing that needs to be cleared away. This simplifies things.

Depends on D51462

This is needed because under the new vibrancy model, vibrant -moz-appearance
values only work on elements that have nothing rendered behind them. The elements
with the vibrant appearance become truly transparent.

Depends on D51464

This code was assuming that the only non-opaque parts of compositor rendering would be the
parts of the window that had vibrancy. But now that the default window background is transparent,
we can have non-vibrant parts where we render into transparency. Dialog windows such as sheet
windows are an example of this.
So instead of using the non-vibrant region of the window as its opaque region, we now use
the region that is covered by opaque Gecko layers. This region is a lot more conservative:
For example, the main browser chrome is now entirely transparent, because the chrome's opaque
parts share a layer with its transparent parts.
As a result, this change slightly affects the CALayer partitioning in the main browser window:
The entire browser chrome is now transparent, not just the tab bar.
The web content area is still opaque.

I think this will be fine. The thing I'm most concerned about is that scrolling inside web
content might cause invalidations of pixels from the chrome, because then we'd recomposite
the CALayers that cover the vibrant tab bar. This doesn't seem to happen most of the time
though, from what I can tell.

Depends on D51465

Priority: -- → P2
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/d0cf63ab4616
Make nsChildView::WidgetPaintsBackground() return true. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/fdbf0586295d
Ignore the background-color CSS value on the window document's root element if that element has a -moz-appearance. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/8c02fa3fcc22
Style the browser window's root element with -moz-appearance: dialog on macOS (which is the default). r=ntim
https://hg.mozilla.org/integration/autoland/rev/fbdd3e1c6155
Make -moz-appearance: dialog render nothing. r=spohl
https://hg.mozilla.org/integration/autoland/rev/d5f51e8bbbcb
Remove vibrancy code for sheet windows. r=spohl
https://hg.mozilla.org/integration/autoland/rev/f4b92056ed05
Stop clearing the background behind vibrant -moz-appearance items. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/1664ebaed4a6
Make the sidebar non-vibrant when any lwtheme is in use. r=ntim
https://hg.mozilla.org/integration/autoland/rev/4d9f24928383
Stop using the vibrant region as the transparent region. r=mattwoodrow
Regressions: 1595408
Regressions: 1599366
Regressions: 1602193
Regressions: 1601183

I need more time to fix the regression that this caused. So I'm going to prepare a backout-patch for 72.

Attached file backout patch

Approval Request Comment
[Feature/Bug causing the regression]: This bug
[User impact if declined]: Regressions: bug 1596826, bug 1599366, bug 1601183
[Is this code covered by automated tests?]: some of it is
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: See the other bugs
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: it's a backout, should be low-risk
[String changes made/needed]: none

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7346baab256a7666a86e1a538545561698f3d5d3

Attachment #9116569 - Flags: approval-mozilla-beta?
Whiteboard: [checkin-needed-beta]
Attachment #9116569 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Regressions: 1594132
Comment on attachment 9116569 [details]
backout patch

(removing approval flag to get this out of uplift queries)
Attachment #9116569 - Flags: approval-mozilla-beta+

Does this need to be pushed to beta (gecko 73) again after the central-to-beta merge on Monday?

Flags: needinfo?(mstange)

This is an update for bug 1596826 that is a regression from this one.

There are 29 total failures in the last 7 days on macosx1014-64 debug and macosx1014-64-shippable opt.

Recent failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=283556900&repo=autoland&lineNumber=1878

[task 2020-01-06T02:55:07.063Z] 02:55:07 INFO - TEST-START | browser/base/content/test/general/browser_tab_detach_restore.js
[task 2020-01-06T02:55:08.012Z] 02:55:08 INFO - GECKO(2234) | Exiting due to channel error.
[task 2020-01-06T02:55:08.013Z] 02:55:08 INFO - GECKO(2234) | Exiting due to channel error.
[task 2020-01-06T02:55:08.013Z] 02:55:08 INFO - GECKO(2234) | [GFX1-]: Receive IPC close with reason=AbnormalShutdown
[task 2020-01-06T02:55:08.013Z] 02:55:08 INFO - GECKO(2234) | Exiting due to channel error.
[task 2020-01-06T02:55:08.013Z] 02:55:08 INFO - GECKO(2234) | Exiting due to channel error.
[task 2020-01-06T02:55:08.013Z] 02:55:08 INFO - GECKO(2234) | Exiting due to channel error.
[task 2020-01-06T02:55:08.013Z] 02:55:08 INFO - GECKO(2234) | Exiting due to channel error.
[task 2020-01-06T02:55:08.014Z] 02:55:08 INFO - GECKO(2234) | Exiting due to channel error.
[task 2020-01-06T02:55:08.014Z] 02:55:08 INFO - GECKO(2234) | Exiting due to channel error.
[task 2020-01-06T02:55:08.014Z] 02:55:08 INFO - GECKO(2234) | Exiting due to channel error.
[task 2020-01-06T02:55:08.199Z] 02:55:08 INFO - TEST-INFO | Main app process: exit 1
[task 2020-01-06T02:55:08.200Z] 02:55:08 INFO - Buffered messages logged at 02:55:07
[task 2020-01-06T02:55:08.200Z] 02:55:08 INFO - Entering test bound
[task 2020-01-06T02:55:08.200Z] 02:55:08 INFO - Console message: OpenGL compositor Initialized Succesfully.
[task 2020-01-06T02:55:08.200Z] 02:55:08 INFO - Version: 2.1 INTEL-12.9.22
[task 2020-01-06T02:55:08.200Z] 02:55:08 INFO - Vendor: Intel Inc.
[task 2020-01-06T02:55:08.200Z] 02:55:08 INFO - Renderer: Intel Iris OpenGL Engine
[task 2020-01-06T02:55:08.200Z] 02:55:08 INFO - FBO Texture Target: TEXTURE_2D
[task 2020-01-06T02:55:08.200Z] 02:55:08 INFO - TEST-PASS | browser/base/content/test/general/browser_tab_detach_restore.js | Should have properly copied the permanentKey -
[task 2020-01-06T02:55:08.210Z] 02:55:08 INFO - TEST-PASS | browser/base/content/test/general/browser_tab_detach_restore.js | Should have restore data for the closed window -
[task 2020-01-06T02:55:08.210Z] 02:55:08 INFO - Console message: OpenGL compositor Initialized Succesfully.
[task 2020-01-06T02:55:08.210Z] 02:55:08 INFO - Version: 2.1 INTEL-12.9.22
[task 2020-01-06T02:55:08.210Z] 02:55:08 INFO - Vendor: Intel Inc.
[task 2020-01-06T02:55:08.210Z] 02:55:08 INFO - Renderer: Intel Iris OpenGL Engine
[task 2020-01-06T02:55:08.210Z] 02:55:08 INFO - FBO Texture Target: TEXTURE_2D
[task 2020-01-06T02:55:08.210Z] 02:55:08 INFO - Buffered messages finished
[task 2020-01-06T02:55:08.210Z] 02:55:08 ERROR - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_tab_detach_restore.js | application terminated with exit code 1
[task 2020-01-06T02:55:08.210Z] 02:55:08 INFO - runtests.py | Application ran for: 0:02:35.883497
[task 2020-01-06T02:55:08.211Z] 02:55:08 INFO - zombiecheck | Reading PID log: /var/folders/gg/h30_s40x70b507f3mjqtg4_m000017/T/tmpur84y6pidlog

Since that is a restricted bug, i cannot update there. Markus please take a look.

Yes, let's back this out on 73 again.

I have compiled the Beta branch with this patch applied locally and everything seemed to be fine. I didn't do a try push.

Flags: needinfo?(mstange)

Backed out from Beta for 73.0b5. This remains fixed on m-c for 74+.
https://hg.mozilla.org/releases/mozilla-beta/rev/ff911052beb1

Backing this out on trunk now since the regressions haven't yet been fixed.

https://hg.mozilla.org/integration/autoland/rev/ecc5c853bb77

beta 74 is next.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Thanks for taking care of this, Julien.

Status: REOPENED → ASSIGNED
Duplicate of this bug: 1620060
Duplicate of this bug: 1620738
Depends on: 1624599
Depends on: 1617088
You need to log in before you can comment on or make changes to this bug.