1051522 - [10.10] Implement support for NSVisualEffectView for Yosemite UI effects

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Nomis101]

It seems to get the translucent backgrounds and all other UI effects introduced in Yosemite, we need some support for NSVisualEffectView. It says [1]:
"The new class NSVisualEffectView enables you to create translucent backgrounds and support vibrancy."

There is also sample code for [2]:
"This sample shows how to use various options of NSVisualEffectView to do blurs, translucency, and vibrancy on OS X. It demonstrates in window blending, behind window blending, creating masks, using state, and custom vibrant controls."

[1]: https://developer.apple.com/library/prerelease/mac/releasenotes/MacOSX/WhatsNewInOSX/Articles/MacOSX10_10.html
[2]: https://developer.apple.com/library/prerelease/mac/samplecode/VisualEffectPlayground/Introduction/Intro.html

Comment 1

[Markus Stange [:mstange]]

Here's how to create an instance of NSVisualEffectView, and an instance of a class that inherits from NSVisualEffectView, when not building against the 10.10 SDK:

static BOOL
AllowsVibrancyYes(id self, SEL _cmd)
{
  return YES;
}

// [...]

  Class NSVisualEffectViewClass = NSClassFromString(@"NSVisualEffectView");
  if (NSVisualEffectViewClass) {
    NSView* effectView1 = [[NSVisualEffectViewClass alloc] initWithFrame:NSMakeRect(0, 100, 1000, 100)];
    [[window contentView] addSubview:effectView1];

    Class EffectViewClass = objc_allocateClassPair(NSVisualEffectViewClass, "EffectView", 0);
    class_addMethod(EffectViewClass, @selector(allowsVibrancy), (IMP)AllowsVibrancyYes, "I@:");
    NSView* effectView2 = [[EffectViewClass alloc] initWithFrame:NSMakeRect(0, 200, 1000, 100)];
    [[window contentView] addSubview:effectView2];
  }

Comment 2

[Markus Stange [:mstange]]

I have this mostly working locally and will attach patches once I've extracted and cleaned them up a little.

Comment 3

[Markus Stange [:mstange]]

Attachment: part 1: Add -moz-appearance: -moz-mac-vibrancy-light and -moz-mac-vibrancy-dark

Comment 4

[Markus Stange [:mstange]]

Attachment: part 2: Mark nsChildView coordinate conversion functions const

I'm going to add a new class that needs access to an nsChildView solely for the coordinate conversion functions, so I'd like to pass a "const nsChildView&" to make the limited impact clear. I need the conversion functions to be const in order for that to work.

Comment 5

[Markus Stange [:mstange]]

Attachment: part 3: Hook everything together

This patch depends on the patches in bug 1055585 in order to work.

It takes care of creating the NSVisualEffectViews that are required for letting the window server know the vibrant areas of a window, and it does that during nsChildView::UpdateThemeGeometries for the new vibrancy -moz-appearance values.

Depending on the development in bug 1055585, some small parts of this patch may be subject to changes, but this can be reviewed already anyway.

Note: This changes the NSView hierarchy. I know you won't like that part, Steven, but adding NSVisualEffectViews is really the only documented way to achieve the desired effect. And from what I've seen of the internal workings, all of the communication of vibrant regions with the window server happens inside methods of NSVisualEffectView, so we really do have to create them.
(We've had a similar conversation before around 944836 comment 15, and my opinion is still that we should just try it and see whether your concerns there were warranted.)

Comment 6

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #5)
> (We've had a similar conversation before around 944836 comment 15

That's bug 944836 comment 15.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

> Note: This changes the NSView hierarchy. I know you won't like that
> part, Steven, but adding NSVisualEffectViews is really the only
> documented way to achieve the desired effect.

I'm not dead set against changing the NSView hierarchy.  I'm just
reluctant to do so if there's any reasonable alternative.

Before reviewing your patches here, I want to spend a few hours
learning what I can about how Apple apps use NSVisualEffectView on
Yosemite.  I should have time for that tomorrow.  So I should be able
to review your patches tomorrow or the day after.

Comment 8

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #7)
> Before reviewing your patches here, I want to spend a few hours
> learning what I can about how Apple apps use NSVisualEffectView on
> Yosemite.

Thanks. Concerning documentation from Apple, the most detailed stuff about vibrancy is probably on https://developer.apple.com/videos/wwdc/2014/ , in "Adapting Your App to the New UI of OS X Yosemite" starting on page 165, and in "Adopting Advanced Features of the New UI of OS X Yosemite".

Here are a few notes from my experiments about things that weren't clear to me from the presentations:
 - There's a difference between the vibrancy you get when you override allowsVibrancy and have it return YES, vs when you return NO. Returning NO does not mean that there's no vibrancy at all! It only means that the blurred vibrancy effect is only visible in those pixels in your window that you leave transparent. I'd call this "background vibrancy". If you return YES from allowsVibrancy, the vibrancy blending also applies to opaque content in your window; I'd call that "foreground vibrancy". We probably don't need foreground vibrancy in Firefox. It looks like it's mostly used for vibrant text labels.
 - Whether you get light or dark vibrancy depends on the appearance you set on the NSVisualEffectView.
 - The appearance value on subviews of the NSVisualEffectView doesn't let you toggle between dark and light, but instead between foreground and background vibrancy, IIRC. Setting the appearance to aqua gives the view only background vibrancy, regardless of the return value of allowsVibrancy. Setting the appearance to light (or dark?) and also returning YES from allowsVibrancy means that you get foreground vibrancy.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8475242 [details] [diff] [review]
part 3: Hook everything together

Thanks for the info, Markus.  What I was most wanted to do was to check out what you say here:

+  // The super implementation would clear the background.
+  // That's fine for views that are placed below their content, but our
+  // setup is different: Our drawn content is drawn to mContainerView, which
+  // sits below this EffectView. So we must not clear the background here,
+  // because we'd erase that drawn content.
+  // Of course the regular content drawing still needs to clear the background
+  // behind vibrant areas. This is taken care of by having nsNativeThemeCocoa
+  // return true from NeedToClearBackgroundBehindWidget for vibrant widgets.

Playing around a bit with an interpose library on Yosemite DP6 (the current DP), I confirmed what you say.  If Safari is any guide, Apple normally (always?) uses NSVisualEffectViews (or subclasses thereof) to make "vibrant" the content of that view (or its subviews).  This patch uses them to make vibrant the content the content of their superview(s) -- or rather of that part of their superviews which they overlap.

I haven't tested this.  But I'll take your word for it that it works.

Could you post one or more testcases here?

Your patch only changes the NSView structure for pages where we want to use vibrancy effects.  I don't imagine there will be too many cases of that.  And especially, I don't imagine we'll ever want to make a plugin "vibrant".  It's plugins that are most likely to have problems with changes to the NSView structure.  So I think it's likely we'll be able to get away with this change without causing trouble.

Comment 10

[Nomis101]

I don't know why, but part 3 doesn't compile for me. I've tried now several times, but I everytime get:

In file included from /Volumes/Developer/mozilla-central/widget/cocoa/VibrancyManager.mm:7:
/Volumes/Developer/mozilla-central/widget/cocoa/VibrancyManager.h:85:17: error: 
      field has incomplete type 'nsIntRegion'
    nsIntRegion region;
                ^
../../dist/include/nsTArray.h:32:7: note: forward declaration of 'nsIntRegion'
class nsIntRegion;
      ^
SkQuarticRoot.o
In file included from /Volumes/Developer/mozilla-central/widget/cocoa/VibrancyManager.mm:7:
/Volumes/Developer/mozilla-central/widget/cocoa/VibrancyManager.h:91:48: error: 
      unknown type name 'NSRect'
  NSView* CreateEffectView(VibrancyType aType, NSRect aRect);

Is there a patch from another bug needed to make this working?

Comment 11

[Markus Stange [:mstange]]

Attachment: part 3: Hook everything together

Fixed the build errors by adding the missing #includes, and added a check for VibrancyManager::SystemSupportsVibrancy() to nsChildView::ClearVibrantAreas() so that we don't crash on pre-10.10.

Comment 12

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #9)
> Could you post one or more testcases here?

I've attached a patch to bug 1059278 which you can use to test this. You can open the sidebar using View -> Sidebar -> Bookmarks (or Cmd+B).

> Your patch only changes the NSView structure for pages where we want to use
> vibrancy effects.  I don't imagine there will be too many cases of that.

Actually, web content can't use vibrancy at all. It can only be used by the browser chrome.
(This is ensured by this line: http://hg.mozilla.org/mozilla-central/annotate/0753f7b93ab7/layout/base/nsDisplayList.cpp#l1766 which causes us not to call nsChildView::UpdateThemeGeometries for "-moz-appearance"-styled web content elements.)

Comment 13

[Markus Stange [:mstange]]

https://tbpl.mozilla.org/?tree=Try&rev=547b0104629a

https://hg.mozilla.org/integration/mozilla-inbound/rev/965564003cda
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4bf1bdbf7ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad26257c3e0d

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/965564003cda
https://hg.mozilla.org/mozilla-central/rev/d4bf1bdbf7ec
https://hg.mozilla.org/mozilla-central/rev/ad26257c3e0d