Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2521753003:
MacViews: Protect against orderOut: on an NSWindow with an attached sheet. (Closed)
|
Created:
4 years, 1 month ago by tapted Modified:
3 years, 11 months ago Reviewers:
karandeepb CC:
chromium-reviews, tfarina, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org, Elly Fong-Jones Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Protect against orderOut: on an NSWindow with an attached sheet.
This fixes and re-enables NativeWidgetMacTest.WindowModalSheet with some
additional test expectations. It was reverted in r433028 when it started
failing.
Fundamentally, the problem is that calling orderOut: on an NSWindow with
an attached sheet results in utterly broken behavior in AppKit,
documented in http://crbug.com/667602.
AppKit's broken behaviour with orderOut and regular child windows is
already handled by BridgedNativeWidget by recreating the parent/child
relationships. However, it can't do this for sheets. It's not possible
to "reattach" a sheet once it becomes detached. And even giving a sheet
a parentWindow results in graphical glitches (http://crbug.com/605098).
Sheets use -[NSWindow sheetParent], not -[NSWindow parentWindow].
Note that -[NSApp hide:] works. It's just -[NSWindow orderOut:] that's
broken. But we need the triggers from -[NSApp hide:] to be consistent
with the different things that need to happen for sheets.
Add checks to BridgedNativeWidget to detect attempts to orderOut: a
window with an attached sheet. Different solutions need to be sought for
these, such as what Chrome already does for the certificate viewer sheet
when we make it tab-modal.
Update DCHECKs in WidgetOwnerNSWindowAdapter to properly support sheets.
This allows NativeWidgetMacTest.WindowModalSheet to be re-enabled.
Updates BridgedNativeWidget::OnVisibilityChanged() to properly fix
http://crbug.com/605098. This bug still manifests in Chrome, but only
after a call to [NSApp hide:]. Re-showing a window improperly attaches
the parent as a parentWindow (rather than only sheetParent), and this is
the cause of the graphical glitches.
BUG=666503, 605098, 667602
Committed: https://crrev.com/9464d37d42d7b7895421a05bd3f0b5f0a3eb3266
Cr-Commit-Position: refs/heads/master@{#434882}
Patch Set 1 #Patch Set 2 : Fix and wow. #Patch Set 3 : Nit comment #
Total comments: 10
Messages
Total messages: 29 (16 generated)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Revert "MacViews: Disable NativeWidgetMacTest.WindowModalSheet." This reverts commit cf8095820e7183dfd786330291460d7ea3b0a94f. BUG=666503 ========== to ========== MacViews: Protect against orderOut: on an NSWindow with at attached sheet. This fixes and re-enables NativeWidgetMacTest.WindowModalSheet with some additional test expectations. It was reverted in r433028 when it started failing. Fundamentally, the problem is that calling orderOut: on an NSWindow with an attached sheet results in utterly broken behavior in AppKit, documented in http://crbug.com/667602. AppKit's broken behaviour with orderOut and regular child windows is already handled by BridgedNativeWidget by recreating the parent/child relationships. However, it can't do this for sheets. It's not possible to "reattach" a sheet once it becomes detached. And even giving a sheet a parentWindow results in graphical glitches (http://crbug.com/605098). Sheets use -[NSWindow sheetParent], not -[NSWindow parentWindow]. Note that -[NSApp hide:] works. It's just -[NSWindow orderOut:] that's broken. But we need the triggers from -[NSApp hide:] to be consistent with the different things that need to happen for sheets. Add checks to BridgedNativeWidget to detect attempts to orderOut: a window with an attached sheet. Different solutions need to be sought for these, such as what Chrome already does for the certificate viewer sheet when we make it tab-modal. Update DCHECKs in WidgetOwnerNSWindowAdapter to properly support sheets. BUG=666503, 605098, 667602 ==========
tapted@chromium.org changed reviewers: + ellyjones@chromium.org
Description was changed from ========== MacViews: Protect against orderOut: on an NSWindow with at attached sheet. This fixes and re-enables NativeWidgetMacTest.WindowModalSheet with some additional test expectations. It was reverted in r433028 when it started failing. Fundamentally, the problem is that calling orderOut: on an NSWindow with an attached sheet results in utterly broken behavior in AppKit, documented in http://crbug.com/667602. AppKit's broken behaviour with orderOut and regular child windows is already handled by BridgedNativeWidget by recreating the parent/child relationships. However, it can't do this for sheets. It's not possible to "reattach" a sheet once it becomes detached. And even giving a sheet a parentWindow results in graphical glitches (http://crbug.com/605098). Sheets use -[NSWindow sheetParent], not -[NSWindow parentWindow]. Note that -[NSApp hide:] works. It's just -[NSWindow orderOut:] that's broken. But we need the triggers from -[NSApp hide:] to be consistent with the different things that need to happen for sheets. Add checks to BridgedNativeWidget to detect attempts to orderOut: a window with an attached sheet. Different solutions need to be sought for these, such as what Chrome already does for the certificate viewer sheet when we make it tab-modal. Update DCHECKs in WidgetOwnerNSWindowAdapter to properly support sheets. BUG=666503, 605098, 667602 ========== to ========== MacViews: Protect against orderOut: on an NSWindow with at attached sheet. This fixes and re-enables NativeWidgetMacTest.WindowModalSheet with some additional test expectations. It was reverted in r433028 when it started failing. Fundamentally, the problem is that calling orderOut: on an NSWindow with an attached sheet results in utterly broken behavior in AppKit, documented in http://crbug.com/667602. AppKit's broken behaviour with orderOut and regular child windows is already handled by BridgedNativeWidget by recreating the parent/child relationships. However, it can't do this for sheets. It's not possible to "reattach" a sheet once it becomes detached. And even giving a sheet a parentWindow results in graphical glitches (http://crbug.com/605098). Sheets use -[NSWindow sheetParent], not -[NSWindow parentWindow]. Note that -[NSApp hide:] works. It's just -[NSWindow orderOut:] that's broken. But we need the triggers from -[NSApp hide:] to be consistent with the different things that need to happen for sheets. Add checks to BridgedNativeWidget to detect attempts to orderOut: a window with an attached sheet. Different solutions need to be sought for these, such as what Chrome already does for the certificate viewer sheet when we make it tab-modal. Update DCHECKs in WidgetOwnerNSWindowAdapter to properly support sheets. This allows NativeWidgetMacTest.WindowModalSheet to be re-enabled. BUG=666503, 605098, 667602 ==========
Description was changed from ========== MacViews: Protect against orderOut: on an NSWindow with at attached sheet. This fixes and re-enables NativeWidgetMacTest.WindowModalSheet with some additional test expectations. It was reverted in r433028 when it started failing. Fundamentally, the problem is that calling orderOut: on an NSWindow with an attached sheet results in utterly broken behavior in AppKit, documented in http://crbug.com/667602. AppKit's broken behaviour with orderOut and regular child windows is already handled by BridgedNativeWidget by recreating the parent/child relationships. However, it can't do this for sheets. It's not possible to "reattach" a sheet once it becomes detached. And even giving a sheet a parentWindow results in graphical glitches (http://crbug.com/605098). Sheets use -[NSWindow sheetParent], not -[NSWindow parentWindow]. Note that -[NSApp hide:] works. It's just -[NSWindow orderOut:] that's broken. But we need the triggers from -[NSApp hide:] to be consistent with the different things that need to happen for sheets. Add checks to BridgedNativeWidget to detect attempts to orderOut: a window with an attached sheet. Different solutions need to be sought for these, such as what Chrome already does for the certificate viewer sheet when we make it tab-modal. Update DCHECKs in WidgetOwnerNSWindowAdapter to properly support sheets. This allows NativeWidgetMacTest.WindowModalSheet to be re-enabled. BUG=666503, 605098, 667602 ========== to ========== MacViews: Protect against orderOut: on an NSWindow with an attached sheet. This fixes and re-enables NativeWidgetMacTest.WindowModalSheet with some additional test expectations. It was reverted in r433028 when it started failing. Fundamentally, the problem is that calling orderOut: on an NSWindow with an attached sheet results in utterly broken behavior in AppKit, documented in http://crbug.com/667602. AppKit's broken behaviour with orderOut and regular child windows is already handled by BridgedNativeWidget by recreating the parent/child relationships. However, it can't do this for sheets. It's not possible to "reattach" a sheet once it becomes detached. And even giving a sheet a parentWindow results in graphical glitches (http://crbug.com/605098). Sheets use -[NSWindow sheetParent], not -[NSWindow parentWindow]. Note that -[NSApp hide:] works. It's just -[NSWindow orderOut:] that's broken. But we need the triggers from -[NSApp hide:] to be consistent with the different things that need to happen for sheets. Add checks to BridgedNativeWidget to detect attempts to orderOut: a window with an attached sheet. Different solutions need to be sought for these, such as what Chrome already does for the certificate viewer sheet when we make it tab-modal. Update DCHECKs in WidgetOwnerNSWindowAdapter to properly support sheets. This allows NativeWidgetMacTest.WindowModalSheet to be re-enabled. BUG=666503, 605098, 667602 ==========
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Elly, please take a look - thanks!
Elly: ping?
tapted@chromium.org changed reviewers: + karandeepb@chromium.org - ellyjones@chromium.org
+karan maybe you can take a look now you're back :) - Thanks!
https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:566: DCHECK(![window_ attachedSheet]); Won't this trigger for the case given in issue 667602 or has that been fixed somewhere else? https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:838: // glitches (http://crbug.com/605098). I can't see it being concluded on issue 605098 that the glitch was due to the sheet having a parent window. Is this a new conclusion? https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/widget_o... File ui/views/cocoa/widget_owner_nswindow_adapter.mm (right): https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:108: DCHECK([child_->ns_window() isSheet] != So this was/is slightly confusing. Is this correct: For sheet BNW, the BNW->parent()->ns_window() refers to the NSWindow the sheet is attached to. But the actual parentWindow property ([[BNW->ns_window()] parentWindow]) for a sheet BNW should be nil? For non-sheet windows, [BNW->ns_window() parentWindow] == BNW->parent()->ns_window(). Using the same parent_ terminology for both cases is slightly confusing. Maybe this can be clarified somewhere in bridged_native_widget.h.
https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:566: DCHECK(![window_ attachedSheet]); On 2016/11/28 05:19:32, karandeepb wrote: > Won't this trigger for the case given in issue 667602 or has that been fixed > somewhere else? Yes, but only with chrome://flags/#mac-views-native-app-windows which is unlikely to launch given the packaged app deprecation on Mac (although it's useful for testing WebContents behaviour with MacViews). The goal is to detect any future things that may rely on the broken behaviour in an obvious way. https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:838: // glitches (http://crbug.com/605098). On 2016/11/28 05:19:32, karandeepb wrote: > I can't see it being concluded on issue 605098 that the glitch was due to the > sheet having a parent window. Is this a new conclusion? Yup :). Turns out this was the culprit. It went away in r403166 because adding the early exit in OnVisibilityChanged() turned the *first* visibility change into a No-op [skipping over this addChildWindow], but not later visibility changes. http://crbug.com/605098 is not completely gone in Canary - if you show Edit Bookmark, then [NSApp hide] with cmd+h, then re-show Chrome, *then* dismiss the sheet you get the same glitch, because the logic here adds it as a child window. https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/widget_o... File ui/views/cocoa/widget_owner_nswindow_adapter.mm (right): https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:108: DCHECK([child_->ns_window() isSheet] != On 2016/11/28 05:19:32, karandeepb wrote: > So this was/is slightly confusing. Is this correct: > > For sheet BNW, the BNW->parent()->ns_window() refers to the NSWindow the sheet > is attached to. But the actual parentWindow property ([[BNW->ns_window()] > parentWindow]) for a sheet BNW should be nil? yes. ([NSWindow parentWindow] should be nil -- but sheets also have [NSWindow sheetParent]) > > For non-sheet windows, [BNW->ns_window() parentWindow] == > BNW->parent()->ns_window(). yes (so long as the windows are visible) > > Using the same parent_ terminology for both cases is slightly confusing. Maybe > this can be clarified somewhere in bridged_native_widget.h. I think the complexity is Cocoa's fault. The nicest thing we can do is probably isolate the BridgedNativeWidget API from anything to do with NSWindow parentWindow/sheetParent, and just manage write defensive code under the hood. (also it's possible we'll drop sheets completely with the modality proposal, and just use tab-modals that don't use sheets at all, but we need to keep this for a bit longer so we aren't changing too much UX at once).
LGTM. https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:566: DCHECK(![window_ attachedSheet]); On 2016/11/28 06:15:41, tapted wrote: > On 2016/11/28 05:19:32, karandeepb wrote: > > Won't this trigger for the case given in issue 667602 or has that been fixed > > somewhere else? > > Yes, but only with chrome://flags/#mac-views-native-app-windows which is > unlikely to launch given the packaged app deprecation on Mac (although it's > useful for testing WebContents behaviour with MacViews). The goal is to detect > any future things that may rely on the broken behaviour in an obvious way. Acknowledged. https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:838: // glitches (http://crbug.com/605098). On 2016/11/28 06:15:41, tapted wrote: > On 2016/11/28 05:19:32, karandeepb wrote: > > I can't see it being concluded on issue 605098 that the glitch was due to the > > sheet having a parent window. Is this a new conclusion? > > Yup :). Turns out this was the culprit. It went away in r403166 because adding > the early exit in OnVisibilityChanged() turned the *first* visibility change > into a No-op [skipping over this addChildWindow], but not later visibility > changes. http://crbug.com/605098 is not completely gone in Canary - if you show > Edit Bookmark, then [NSApp hide] with cmd+h, then re-show Chrome, *then* dismiss > the sheet you get the same glitch, because the logic here adds it as a child > window. Acknowledged. Maybe mention this explicitly in the CL description, since this fixes some behavior. https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/widget_o... File ui/views/cocoa/widget_owner_nswindow_adapter.mm (right): https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:108: DCHECK([child_->ns_window() isSheet] != On 2016/11/28 06:15:41, tapted wrote: > On 2016/11/28 05:19:32, karandeepb wrote: > > So this was/is slightly confusing. Is this correct: > > > > For sheet BNW, the BNW->parent()->ns_window() refers to the NSWindow the sheet > > is attached to. But the actual parentWindow property ([[BNW->ns_window()] > > parentWindow]) for a sheet BNW should be nil? > > yes. ([NSWindow parentWindow] should be nil -- but sheets also have [NSWindow > sheetParent]) > > > > > For non-sheet windows, [BNW->ns_window() parentWindow] == > > BNW->parent()->ns_window(). > > yes (so long as the windows are visible) > > > > > Using the same parent_ terminology for both cases is slightly confusing. Maybe > > this can be clarified somewhere in bridged_native_widget.h. > > I think the complexity is Cocoa's fault. The nicest thing we can do is probably > isolate the BridgedNativeWidget API from anything to do with NSWindow > parentWindow/sheetParent, and just manage write defensive code under the hood. > > (also it's possible we'll drop sheets completely with the modality proposal, and > just use tab-modals that don't use sheets at all, but we need to keep this for a > bit longer so we aren't changing too much UX at once). Acknowledged.
Description was changed from ========== MacViews: Protect against orderOut: on an NSWindow with an attached sheet. This fixes and re-enables NativeWidgetMacTest.WindowModalSheet with some additional test expectations. It was reverted in r433028 when it started failing. Fundamentally, the problem is that calling orderOut: on an NSWindow with an attached sheet results in utterly broken behavior in AppKit, documented in http://crbug.com/667602. AppKit's broken behaviour with orderOut and regular child windows is already handled by BridgedNativeWidget by recreating the parent/child relationships. However, it can't do this for sheets. It's not possible to "reattach" a sheet once it becomes detached. And even giving a sheet a parentWindow results in graphical glitches (http://crbug.com/605098). Sheets use -[NSWindow sheetParent], not -[NSWindow parentWindow]. Note that -[NSApp hide:] works. It's just -[NSWindow orderOut:] that's broken. But we need the triggers from -[NSApp hide:] to be consistent with the different things that need to happen for sheets. Add checks to BridgedNativeWidget to detect attempts to orderOut: a window with an attached sheet. Different solutions need to be sought for these, such as what Chrome already does for the certificate viewer sheet when we make it tab-modal. Update DCHECKs in WidgetOwnerNSWindowAdapter to properly support sheets. This allows NativeWidgetMacTest.WindowModalSheet to be re-enabled. BUG=666503, 605098, 667602 ========== to ========== MacViews: Protect against orderOut: on an NSWindow with an attached sheet. This fixes and re-enables NativeWidgetMacTest.WindowModalSheet with some additional test expectations. It was reverted in r433028 when it started failing. Fundamentally, the problem is that calling orderOut: on an NSWindow with an attached sheet results in utterly broken behavior in AppKit, documented in http://crbug.com/667602. AppKit's broken behaviour with orderOut and regular child windows is already handled by BridgedNativeWidget by recreating the parent/child relationships. However, it can't do this for sheets. It's not possible to "reattach" a sheet once it becomes detached. And even giving a sheet a parentWindow results in graphical glitches (http://crbug.com/605098). Sheets use -[NSWindow sheetParent], not -[NSWindow parentWindow]. Note that -[NSApp hide:] works. It's just -[NSWindow orderOut:] that's broken. But we need the triggers from -[NSApp hide:] to be consistent with the different things that need to happen for sheets. Add checks to BridgedNativeWidget to detect attempts to orderOut: a window with an attached sheet. Different solutions need to be sought for these, such as what Chrome already does for the certificate viewer sheet when we make it tab-modal. Update DCHECKs in WidgetOwnerNSWindowAdapter to properly support sheets. This allows NativeWidgetMacTest.WindowModalSheet to be re-enabled. Updates BridgedNativeWidget::OnVisibilityChanged() to properly fix http://crbug.com/605098. This bug still manifests in Chrome, but only after a call to [NSApp hide:]. Re-showing a window improperly attaches the parent as a parentWindow (rather than only sheetParent), and this is the cause of the graphical glitches. BUG=666503, 605098, 667602 ==========
https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/2521753003/diff/30004/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.mm:838: // glitches (http://crbug.com/605098). On 2016/11/28 23:37:57, karandeepb wrote: > On 2016/11/28 06:15:41, tapted wrote: > > On 2016/11/28 05:19:32, karandeepb wrote: > > > I can't see it being concluded on issue 605098 that the glitch was due to > the > > > sheet having a parent window. Is this a new conclusion? > > > > Yup :). Turns out this was the culprit. It went away in r403166 because adding > > the early exit in OnVisibilityChanged() turned the *first* visibility change > > into a No-op [skipping over this addChildWindow], but not later visibility > > changes. http://crbug.com/605098 is not completely gone in Canary - if you > show > > Edit Bookmark, then [NSApp hide] with cmd+h, then re-show Chrome, *then* > dismiss > > the sheet you get the same glitch, because the logic here adds it as a child > > window. > > Acknowledged. Maybe mention this explicitly in the CL description, since this > fixes some behavior. Done.
The CQ bit was checked by tapted@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 30004, "attempt_start_ts": 1480390154446500,
"parent_rev": "7f8ad55de072b5a8da9908da82e4c1ea68468299", "commit_rev":
"d6b9c63fd9e8c02d29a0cc33cef58be503a6bde3"}
Message was sent while issue was closed.
Description was changed from ========== MacViews: Protect against orderOut: on an NSWindow with an attached sheet. This fixes and re-enables NativeWidgetMacTest.WindowModalSheet with some additional test expectations. It was reverted in r433028 when it started failing. Fundamentally, the problem is that calling orderOut: on an NSWindow with an attached sheet results in utterly broken behavior in AppKit, documented in http://crbug.com/667602. AppKit's broken behaviour with orderOut and regular child windows is already handled by BridgedNativeWidget by recreating the parent/child relationships. However, it can't do this for sheets. It's not possible to "reattach" a sheet once it becomes detached. And even giving a sheet a parentWindow results in graphical glitches (http://crbug.com/605098). Sheets use -[NSWindow sheetParent], not -[NSWindow parentWindow]. Note that -[NSApp hide:] works. It's just -[NSWindow orderOut:] that's broken. But we need the triggers from -[NSApp hide:] to be consistent with the different things that need to happen for sheets. Add checks to BridgedNativeWidget to detect attempts to orderOut: a window with an attached sheet. Different solutions need to be sought for these, such as what Chrome already does for the certificate viewer sheet when we make it tab-modal. Update DCHECKs in WidgetOwnerNSWindowAdapter to properly support sheets. This allows NativeWidgetMacTest.WindowModalSheet to be re-enabled. Updates BridgedNativeWidget::OnVisibilityChanged() to properly fix http://crbug.com/605098. This bug still manifests in Chrome, but only after a call to [NSApp hide:]. Re-showing a window improperly attaches the parent as a parentWindow (rather than only sheetParent), and this is the cause of the graphical glitches. BUG=666503, 605098, 667602 ========== to ========== MacViews: Protect against orderOut: on an NSWindow with an attached sheet. This fixes and re-enables NativeWidgetMacTest.WindowModalSheet with some additional test expectations. It was reverted in r433028 when it started failing. Fundamentally, the problem is that calling orderOut: on an NSWindow with an attached sheet results in utterly broken behavior in AppKit, documented in http://crbug.com/667602. AppKit's broken behaviour with orderOut and regular child windows is already handled by BridgedNativeWidget by recreating the parent/child relationships. However, it can't do this for sheets. It's not possible to "reattach" a sheet once it becomes detached. And even giving a sheet a parentWindow results in graphical glitches (http://crbug.com/605098). Sheets use -[NSWindow sheetParent], not -[NSWindow parentWindow]. Note that -[NSApp hide:] works. It's just -[NSWindow orderOut:] that's broken. But we need the triggers from -[NSApp hide:] to be consistent with the different things that need to happen for sheets. Add checks to BridgedNativeWidget to detect attempts to orderOut: a window with an attached sheet. Different solutions need to be sought for these, such as what Chrome already does for the certificate viewer sheet when we make it tab-modal. Update DCHECKs in WidgetOwnerNSWindowAdapter to properly support sheets. This allows NativeWidgetMacTest.WindowModalSheet to be re-enabled. Updates BridgedNativeWidget::OnVisibilityChanged() to properly fix http://crbug.com/605098. This bug still manifests in Chrome, but only after a call to [NSApp hide:]. Re-showing a window improperly attaches the parent as a parentWindow (rather than only sheetParent), and this is the cause of the graphical glitches. BUG=666503, 605098, 667602 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:30004)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Protect against orderOut: on an NSWindow with an attached sheet. This fixes and re-enables NativeWidgetMacTest.WindowModalSheet with some additional test expectations. It was reverted in r433028 when it started failing. Fundamentally, the problem is that calling orderOut: on an NSWindow with an attached sheet results in utterly broken behavior in AppKit, documented in http://crbug.com/667602. AppKit's broken behaviour with orderOut and regular child windows is already handled by BridgedNativeWidget by recreating the parent/child relationships. However, it can't do this for sheets. It's not possible to "reattach" a sheet once it becomes detached. And even giving a sheet a parentWindow results in graphical glitches (http://crbug.com/605098). Sheets use -[NSWindow sheetParent], not -[NSWindow parentWindow]. Note that -[NSApp hide:] works. It's just -[NSWindow orderOut:] that's broken. But we need the triggers from -[NSApp hide:] to be consistent with the different things that need to happen for sheets. Add checks to BridgedNativeWidget to detect attempts to orderOut: a window with an attached sheet. Different solutions need to be sought for these, such as what Chrome already does for the certificate viewer sheet when we make it tab-modal. Update DCHECKs in WidgetOwnerNSWindowAdapter to properly support sheets. This allows NativeWidgetMacTest.WindowModalSheet to be re-enabled. Updates BridgedNativeWidget::OnVisibilityChanged() to properly fix http://crbug.com/605098. This bug still manifests in Chrome, but only after a call to [NSApp hide:]. Re-showing a window improperly attaches the parent as a parentWindow (rather than only sheetParent), and this is the cause of the graphical glitches. BUG=666503, 605098, 667602 ========== to ========== MacViews: Protect against orderOut: on an NSWindow with an attached sheet. This fixes and re-enables NativeWidgetMacTest.WindowModalSheet with some additional test expectations. It was reverted in r433028 when it started failing. Fundamentally, the problem is that calling orderOut: on an NSWindow with an attached sheet results in utterly broken behavior in AppKit, documented in http://crbug.com/667602. AppKit's broken behaviour with orderOut and regular child windows is already handled by BridgedNativeWidget by recreating the parent/child relationships. However, it can't do this for sheets. It's not possible to "reattach" a sheet once it becomes detached. And even giving a sheet a parentWindow results in graphical glitches (http://crbug.com/605098). Sheets use -[NSWindow sheetParent], not -[NSWindow parentWindow]. Note that -[NSApp hide:] works. It's just -[NSWindow orderOut:] that's broken. But we need the triggers from -[NSApp hide:] to be consistent with the different things that need to happen for sheets. Add checks to BridgedNativeWidget to detect attempts to orderOut: a window with an attached sheet. Different solutions need to be sought for these, such as what Chrome already does for the certificate viewer sheet when we make it tab-modal. Update DCHECKs in WidgetOwnerNSWindowAdapter to properly support sheets. This allows NativeWidgetMacTest.WindowModalSheet to be re-enabled. Updates BridgedNativeWidget::OnVisibilityChanged() to properly fix http://crbug.com/605098. This bug still manifests in Chrome, but only after a call to [NSApp hide:]. Re-showing a window improperly attaches the parent as a parentWindow (rather than only sheetParent), and this is the cause of the graphical glitches. BUG=666503, 605098, 667602 Committed: https://crrev.com/9464d37d42d7b7895421a05bd3f0b5f0a3eb3266 Cr-Commit-Position: refs/heads/master@{#434882} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9464d37d42d7b7895421a05bd3f0b5f0a3eb3266 Cr-Commit-Position: refs/heads/master@{#434882}
Message was sent while issue was closed.
This CL broke WebDialogBrowserTest.CloseParentWindow test on mac_views_browser build. Test now fails on newly added DCHECK in bridged_native_widget.mm.
Message was sent while issue was closed.
On 2016/12/28 13:36:20, atimoxin wrote: > This CL broke WebDialogBrowserTest.CloseParentWindow test on mac_views_browser > build. > > Test now fails on newly added DCHECK in bridged_native_widget.mm. Can you please file a bug? Trent can take a look when he gets back.
Message was sent while issue was closed.
On 2016/12/28 17:04:29, karandeepb wrote: > On 2016/12/28 13:36:20, atimoxin wrote: > > This CL broke WebDialogBrowserTest.CloseParentWindow test on mac_views_browser > > build. > > > > Test now fails on newly added DCHECK in bridged_native_widget.mm. > > Can you please file a bug? Trent can take a look when he gets back. Yes, of course: crbug.com/677520 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
