New issue
Advanced search Search tips

Issue 869486 link

Starred by 2 users

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 865287



Sign in to add a comment

Window levels don't work in MacViews

Project Member Reported by a...@chromium.org, Jul 31

Issue description

Working on bug 865287, I tracked down a window level issue I was facing to NativeWidgetMac::InitNativeWidget(). If the |keep_on_top| param is set, then SetNSWindowAlwaysOnTop is called.

https://cs.chromium.org/search/?q=SetNSWindowAlwaysOnTop

That sets the window level to NSFloatingWindowLevel (3). OK.

But if we look at BridgedNativeWidget::SetVisibilityState(), and add logging to the ui::SHOW_STATE_INACTIVE case:

    NSLog(@"showing inactive window %@ with level %ld", window_.get(), [window_ level]);
    [window_ orderWindow:NSWindowAbove
              relativeTo:parent_window_number];
    NSLog(@"did show inactive window %@ with level %ld", window_.get(), [window_ level]);

we see as log output:

2018-07-31 13:21:28.842 Chromium[56148:11388635] showing inactive window <NativeWidgetMacNSWindow: 0x7fbac59911b0> with level 3
2018-07-31 13:21:28.842 Chromium[56148:11388635] did show inactive window <NativeWidgetMacNSWindow: 0x7fbac59911b0> with level 0

If we drop a breakpoint in lldb on AppKit`-[NSWindow setLevel:] we get:

  * frame #0: 0x00007fff87540fb6 AppKit`-[NSWindow setLevel:]
    frame #1: 0x00007fff87540d44 AppKit`-[NSWindow addChildWindow:ordered:] + 649
    frame #2: 0x000000013c27f3be libviews.dylib`views::BridgedNativeWidget::OnVisibilityChanged(this=0x0000000171a02860) + 270 at bridged_native_widget.mm:800

This seems to hit a fundamental conflict between how Views manages windows (oops, widgets) and Cocoa. Views uses window parent relationships to handle ownership, but in Cocoa, child windows must have the same window level as their parent window.

I'm not sure how to reconcile this.
 
Labels: Proj-MacViews
To clarify that last backtrace:

  * frame #0: 0x00007fff87540fb6 AppKit`-[NSWindow setLevel:]
    frame #1: 0x00007fff87540d44 AppKit`-[NSWindow addChildWindow:ordered:] + 649
    frame #2: 0x000000013c27f3be libviews.dylib`views::BridgedNativeWidget::OnVisibilityChanged(this=0x0000000171a02860) + 270 at bridged_native_widget.mm:800
    frame #3: 0x000000013c297b98 libviews.dylib`::-[ViewsNSWindowDelegate onWindowOrderChanged:](self=0x000000016260bfc0, _cmd="onWindowOrderChanged:", notification=0x0000000000000000) + 40 at views_nswindow_delegate.mm:74
    frame #4: 0x000000013c29652b libviews.dylib`::-[NativeWidgetMacNSWindow orderWindow:relativeTo:](self=0x0000000171a02ab0, _cmd="orderWindow:relativeTo:", orderingMode=NSWindowAbove, otherWindowNumber=5518) + 123 at native_widget_mac_nswindow.mm:229
    frame #5: 0x000000013c27c424 libviews.dylib`views::BridgedNativeWidget::SetVisibilityState(this=0x0000000171a02860, new_state=SHOW_INACTIVE) + 1364 at bridged_native_widget.mm:547
    frame #6: 0x000000013c4109d6 libviews.dylib`views::NativeWidgetMac::ShowWithWindowState(this=0x0000000162668a20, state=SHOW_STATE_INACTIVE) + 790 at native_widget_mac.mm:470

BridgedNativeWidget::SetVisibilityState() → -[NativeWidgetMacNSWindow orderWindow:relativeTo:] → -[ViewsNSWindowDelegate onWindowOrderChanged:] → BridgedNativeWidget::OnVisibilityChanged() → -[NSWindow addChildWindow:ordered:] → -[NSWindow setLevel:]

The same issue with child windows vs layers remains.
Yeah.. I encountered this in https://chromium-review.googlesource.com/c/chromium/src/+/923227 too..

But note that MacViews doesn't use the native parent/child window properties for lifetime management (it can't coz it breaks when stuff is ordered out). So simply skipping the addChildWindow step if something has a higher window level than the parent is probably Just Fine and has the same window-ordering guarantees.

The caveat is that is does _not_ have the same moves-with-parent-window guarantees. For menus that shouldn't matter. For other bubbles that care, they likely already have redundant "watch parent-move and mimc" logic since:
 - resizing the top corners of a window need to move these bubbles anyway
 - moves done by programatic serFrame calls need to be watched regardless
 - other platforms don't have this mimic-move stuff

Where it might break down is when a window moves to a new space. I'm pretty sure the parentWindow goop is what makes persistent bubbles (e.g. permissions requests) teleport with the window into the new space.
Cutting the parent/child relationship works for menus. Can we establish rules of when this happens?
BTW, re SetAlwaysOnTop, that's such a disaster. It's called for menu and drag window types, but baking the Windows notion of only having two window levels into Views when on the Mac there are UINT64_MAX window levels is just ... ugh.
Aura exposes an entire layer tree.. see subclasses of WindowStackingClient. That results in fun bugs like  Issue 848327  (/me hides). So a "level" is actually a pointer into the tree that has an array of other windows at your level, which you can be at the top of or the bottom of. But aura is a window manager. Likely the macOS WindowServer process has something equivalent internally that it presents a simplified API to via window levels.

I think a map from WidgetDelegate/Widget::InitParams::Type to 'bool' => "should I maintain [parent addChildWindow]" is reasonable.

E.g. window modal sheets already opt out.

For now, I'd say just those and menus until there's a strong use case for something else.
That makes sense. I'm still trying to work out what to do with SetAlwaysOnTop though.

Widget::InitParams::InitParams has
  keep_on_top(type == TYPE_MENU || type == TYPE_DRAG)

and by the time our code processes SetAlwaysOnTop it has no idea what window level to use. Menu? Drag? Generic floating window?

The only way I can see out of this is to make keep_on_top an enum for a few different window levels for our map, including a generic one for things like extensions (this is exposed to extensions!) and changing things so that on other platforms any of them is "on top" but we have the data on the Mac to pick the right value.

This seems a bit down my priority list tho.
Cross-posting from email: it looks like window levels can work for child windows, you just need to set the level *after* adding the window as a child. Fun stuff.
WindowLevels.zip
3.2 KB Download
Components: Internals>Views

Sign in to add a comment