Add special handling for popups which will contain remote browsers

RESOLVED FIXED in mozilla55

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks 1 bug)

unspecified
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged)

Attachments

(7 attachments)

(Assignee)

Description

2 years ago
Popups with remote browsers currently do not render correctly on any platform. Since support for composited popups is marginal, at best, at the moment, we only want to enable the features we need for these popups when they actually contain remote content.

This bug adds an initial stopgap implementation, primarily for testing purposes, that force enables compositing for remote popups, despite the UI quirks it causes.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8868666 [details]
Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X.

https://reviewboard.mozilla.org/r/140258/#review143604

::: widget/cocoa/nsCocoaWindow.mm:481
(Diff revision 1)
>      [mWindow setLevel:kCGDesktopWindowLevelKey];
>    }
>  
>    if (mWindowType == eWindowType_popup) {
>      SetPopupWindowLevel();
> -    [mWindow setHasShadow:YES];
> +    [mWindow setHasShadow:mDropShadow];

I think this is going to be overwritten almost immediately by nsCocoaWindow::SetWindowShadowStyle. I'd prefer to keep ignoring the initData->mDropShadow field and only respect the -moz-window-shadow property.

Does this change actually make a difference? I'd expect the window to not have a shadow either way, due to the accelerated content.

Comment 9

2 years ago
mozreview-review
Comment on attachment 8868662 [details]
Bug 1365660: Part 3 - Disable transparency (where necessary) and drop shadows for popups which contain remote content. .schouten

https://reviewboard.mozilla.org/r/140250/#review143606

::: layout/xul/nsMenuPopupFrame.cpp:324
(Diff revision 1)
>    nsIAtom *tag = nullptr;
>    if (parentContent && parentContent->IsXULElement())
>      tag = parentContent->NodeInfo()->NameAtom();
>    widgetData.mHasRemoteContent = remote;
>    widgetData.mSupportTranslucency = mode == eTransparencyTransparent;
> -  widgetData.mDropShadow = !(mode == eTransparencyTransparent || tag == nsGkAtoms::menulist);
> +  widgetData.mDropShadow = !(remote || mode == eTransparencyTransparent || tag == nsGkAtoms::menulist);

Does this change make a difference on Windows?
(Assignee)

Comment 10

2 years ago
mozreview-review-reply
Comment on attachment 8868666 [details]
Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X.

https://reviewboard.mozilla.org/r/140258/#review143604

> I think this is going to be overwritten almost immediately by nsCocoaWindow::SetWindowShadowStyle. I'd prefer to keep ignoring the initData->mDropShadow field and only respect the -moz-window-shadow property.
> 
> Does this change actually make a difference? I'd expect the window to not have a shadow either way, due to the accelerated content.

The window definitely has a drop shadow without this change. I'm still waiting for a build to finish so I can test with it. The MacBook that I use for testing OS-X is extremely slow.
(Assignee)

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8868662 [details]
Bug 1365660: Part 3 - Disable transparency (where necessary) and drop shadows for popups which contain remote content. .schouten

https://reviewboard.mozilla.org/r/140250/#review143606

> Does this change make a difference on Windows?

I believe so, but I'm not certain. I don't have a Windows machine to test on at the moment (still waiting for an order), but someone else tested for me:

https://usercontent.irccloud-cdn.com/file/Z8XSLfqA/jk.png

That build was with transparency disabled on all platforms, so there may be a drop shadow hidden behind the opaque areas. I'm not sure what the results are with it enabled, but either way they should be good enough for testing. This won't be enabled by default until good UI support.

Comment 12

2 years ago
mozreview-review
Comment on attachment 8868663 [details]
Bug 1365660: Part 4 - Only enable APZ for popups which contain remote content.

https://reviewboard.mozilla.org/r/140252/#review143618
Attachment #8868663 - Flags: review?(bugmail) → review+

Comment 13

2 years ago
mozreview-review
Comment on attachment 8868661 [details]
Bug 1365660: Part 2 - Add a HasRemoteContent flag to popup widgets that contain remote browsers. .schouten

https://reviewboard.mozilla.org/r/140248/#review143620
Attachment #8868661 - Flags: review?(bugmail) → review+
(Assignee)

Comment 14

2 years ago
mozreview-review-reply
Comment on attachment 8868666 [details]
Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X.

https://reviewboard.mozilla.org/r/140258/#review143604

> The window definitely has a drop shadow without this change. I'm still waiting for a build to finish so I can test with it. The MacBook that I use for testing OS-X is extremely slow.

You're right, it doesn't make a difference. So I guess we should just remove the mDropShadow properly entirely (since it seems to actually be overridden by CSS on all platforms), and just disable drop shadows for composited popups until we get them working properly.
I would prefer that, yes.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

2 years ago
mozreview-review
Comment on attachment 8868660 [details]
Bug 1365660: Part 1 - Add a remote="true" attribute to popups with remote browsers.

https://reviewboard.mozilla.org/r/140246/#review143668
Attachment #8868660 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 22

2 years ago
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #11)
> Comment on attachment 8868662 [details]
> Bug 1365660: Part 3 - Disable transparency (where necessary) and drop
> shadows for popups which contain remote content. .schouten
> 
> https://reviewboard.mozilla.org/r/140250/#review143606
> 
> > Does this change make a difference on Windows?
> 
> I believe so, but I'm not certain. I don't have a Windows machine to test on
> at the moment (still waiting for an order), but someone else tested for me:
> 
> https://usercontent.irccloud-cdn.com/file/Z8XSLfqA/jk.png
> 
> That build was with transparency disabled on all platforms, so there may be
> a drop shadow hidden behind the opaque areas. I'm not sure what the results
> are with it enabled, but either way they should be good enough for testing.
> This won't be enabled by default until good UI support.

Zombie tested again with the latest build that didn't disable transparently, and apparently drop shadows are not disabled, but the popup appears to display without any visual issues:

https://usercontent.irccloud-cdn.com/file/LdAV2FgG/Untitled3.png

Updated

2 years ago
Priority: -- → P1
Whiteboard: triaged
(Assignee)

Updated

2 years ago
Blocks: webext-oop

Comment 23

2 years ago
mozreview-review
Comment on attachment 8868662 [details]
Bug 1365660: Part 3 - Disable transparency (where necessary) and drop shadows for popups which contain remote content. .schouten

https://reviewboard.mozilla.org/r/140250/#review144236

::: commit-message-400c8:3
(Diff revision 2)
> +Drop shadows are currently not handled correctly for animated, composited
> +popups on Windows and OS-X. The shadows are rendered, but are not correctly
> +updated to match the final popup size.
> +
> +Ideally, we should support drop shadows for these popups correctly, but in the
> +interim, disabling them gives better results.

This part of the commit message seems no longer relevant.

::: layout/xul/nsMenuPopupFrame.cpp:308
(Diff revision 2)
>    }
>  
>    bool remote = HasRemoteContent();
>  
>    nsTransparencyMode mode = nsLayoutUtils::GetFrameTransparency(this, this);
> +#ifdef MOZ_WIDGET_GTK

Please annotate this with a bug number for a bug that tracks fixing this properly.
Attachment #8868662 - Flags: review?(mstange) → review+

Comment 24

2 years ago
mozreview-review
Comment on attachment 8868666 [details]
Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X.

https://reviewboard.mozilla.org/r/140258/#review144238

::: widget/cocoa/nsChildView.mm:1428
(Diff revision 2)
> +  if (HasRemoteContent())
> +    return true;

Please add braces. I know this file isn't very consistent with respect to that rule, but  we should follow the style guide on new code.

::: widget/cocoa/nsCocoaWindow.h:364
(Diff revision 2)
>  
>    nsresult             CreateNativeWindow(const NSRect &aRect,
>                                            nsBorderStyle aBorderStyle,
>                                            bool aRectIsFrameRect);
> -  nsresult             CreatePopupContentView(const LayoutDeviceIntRect &aRect);
> +  nsresult             CreatePopupContentView(const LayoutDeviceIntRect &aRect,
> +                                              nsWidgetInitData* aInitData);

Is this change still needed?
Attachment #8868666 - Flags: review?(mstange) → review+
(Assignee)

Comment 25

2 years ago
mozreview-review-reply
Comment on attachment 8868666 [details]
Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X.

https://reviewboard.mozilla.org/r/140258/#review144238

> Is this change still needed?

Yes, so that we can propagate the mHasRemoteContent flag to the content view, where it's actually checked.

Comment 26

2 years ago
mozreview-review-reply
Comment on attachment 8868666 [details]
Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X.

https://reviewboard.mozilla.org/r/140258/#review144238

> Yes, so that we can propagate the mHasRemoteContent flag to the content view, where it's actually checked.

Ah, makes sense.

Comment 27

2 years ago
mozreview-review
Comment on attachment 8868661 [details]
Bug 1365660: Part 2 - Add a HasRemoteContent flag to popup widgets that contain remote browsers. .schouten

https://reviewboard.mozilla.org/r/140248/#review144314
Attachment #8868661 - Flags: review?(bas) → review+

Comment 28

2 years ago
mozreview-review
Comment on attachment 8868662 [details]
Bug 1365660: Part 3 - Disable transparency (where necessary) and drop shadows for popups which contain remote content. .schouten

https://reviewboard.mozilla.org/r/140250/#review144316

I agree with Markus.
Attachment #8868662 - Flags: review?(bas) → review+

Comment 29

2 years ago
mozreview-review
Comment on attachment 8868665 [details]
Bug 1365660: Part 5b - Enable compositing for popups with remote content on Windows. .schouten

https://reviewboard.mozilla.org/r/140256/#review144318

::: widget/windows/nsWindow.cpp:7172
(Diff revision 2)
>  nsWindow::ShouldUseOffMainThreadCompositing()
>  {
>    // We don't currently support using an accelerated layer manager with
>    // transparent windows so don't even try. I'm also not sure if we even
>    // want to support this case. See bug 593471
> -  if (mTransparencyMode == eTransparencyTransparent) {
> +  if (!HasRemoteContent() && mTransparencyMode == eTransparencyTransparent) {

This causes assertions right now. See my and Jimm's comments in bug 1356317.
(Assignee)

Comment 30

2 years ago
(In reply to Bas Schouten (:bas.schouten) from comment #29)
> Comment on attachment 8868665 [details]
> Bug 1365660: Part 5b - Enable compositing for popups with remote content on
> Windows. .schouten
> 
> https://reviewboard.mozilla.org/r/140256/#review144318
> 
> ::: widget/windows/nsWindow.cpp:7172
> (Diff revision 2)
> >  nsWindow::ShouldUseOffMainThreadCompositing()
> >  {
> >    // We don't currently support using an accelerated layer manager with
> >    // transparent windows so don't even try. I'm also not sure if we even
> >    // want to support this case. See bug 593471
> > -  if (mTransparencyMode == eTransparencyTransparent) {
> > +  if (!HasRemoteContent() && mTransparencyMode == eTransparencyTransparent) {
> 
> This causes assertions right now. See my and Jimm's comments in bug 1356317.

I saw, thanks. We'll leave OOP extensions disabled by default on Windows until that bug is fixed.
Comment on attachment 8868664 [details]
Bug 1365660: Part 5a - Enable compositing for popups with remote content on Linux.

It's been a while since I last touched this code, redirecting to karlt.
Attachment #8868664 - Flags: review?(mcastelluccio) → review?(karlt)
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 32

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e53d393c722f451c0e8dca642dbb1ace5331f6f
Bug 1365660: Part 1 - Add a remote="true" attribute to popups with remote browsers. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfe30fc6158c4dcb6c9fe2bc9bff35674ed9b247
Bug 1365660: Part 2 - Add a HasRemoteContent flag to popup widgets that contain remote browsers. r=kats,bas

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb4b09616e235659ac4b6570c99f5a771d2d9206
Bug 1365660: Part 3 - Disable transparency (where necessary) for popups which contain remote content. r=mstange,bas

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b38e46d1a8b5cde3885c753919c36115ba9439c
Bug 1365660: Part 4 - Only enable APZ for popups which contain remote content. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/38f3bef8b8651935cb3d9eac0ff5d93da87bee3d
Bug 1365660: Part 5c - Enable compositing for popups with remote content on OS-X. r=mstange
(Assignee)

Updated

2 years ago
Blocks: 1362987
Blocks: 1365984
(Assignee)

Comment 34

2 years ago
Bas, is there a reason that we can't land the Windows portion of this before bug 1356317 is fixed, as long as remote popups are disabled behind a hidden pref? It would be really helpful for QA, since they want to get as much testing done for this as possible, as soon as they can.
Flags: needinfo?(bas)

Comment 35

2 years ago
mozreview-review-reply
Comment on attachment 8868662 [details]
Bug 1365660: Part 3 - Disable transparency (where necessary) and drop shadows for popups which contain remote content. .schouten

https://reviewboard.mozilla.org/r/140250/#review144236

> Please annotate this with a bug number for a bug that tracks fixing this properly.

This was annotated with bug 630346, which is closed:
https://hg.mozilla.org/mozilla-central/rev/cb4b09616e235659ac4b6570c99f5a771d2d9206#l1.18

The review asked for a bug that tracks fixing this.

Note also that bug 630346 was about GL layers.
OMTC on Linux does not use GL layers without about:config changes.

Since changes for bug 408284, which was fixed after bug 630346, shaping is necessary for translucency only when the window manager does not composite windows.  I don't know whether or not there are good reasons to avoid OMTC for translucency when there is a compositing window manager.

I'm not clear whether the black you see is merely due to skipping the shaping code required with non-compositing window manager, or whether there are additional problems with the OMTC compositor in Gecko.

Comment 36

2 years ago
mozreview-review
Comment on attachment 8868664 [details]
Bug 1365660: Part 5a - Enable compositing for popups with remote content on Linux.

https://reviewboard.mozilla.org/r/140254/#review145768

::: widget/gtk/nsWindow.cpp:6429
(Diff revision 2)
> +nsWindow::ShouldUseOffMainThreadCompositing()
> +{
> +    return (nsBaseWidget::ShouldUseOffMainThreadCompositing() &&
> +            (HasRemoteContent() || GetTransparencyMode() != eTransparencyTransparent));

Doesn't GetTransparencyMode() return eTransparencyTransparent when HasRemoteContent() due to this?
http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/xul/nsMenuPopupFrame.cpp#315

That would make the HasRemoteContent() check here unnecessary?

As the HasRemoteContent() test seems to be merely a workaround to avoid unresolved issues, I'd prefer not to add the unnecessary workaround.
Attachment #8868664 - Flags: review?(karlt)

Comment 37

2 years ago
mozreview-review
Comment on attachment 8868665 [details]
Bug 1365660: Part 5b - Enable compositing for popups with remote content on Windows. .schouten

https://reviewboard.mozilla.org/r/140256/#review147108

::: widget/windows/nsWindow.cpp:7172
(Diff revision 2)
>  nsWindow::ShouldUseOffMainThreadCompositing()
>  {
>    // We don't currently support using an accelerated layer manager with
>    // transparent windows so don't even try. I'm also not sure if we even
>    // want to support this case. See bug 593471
> -  if (mTransparencyMode == eTransparencyTransparent) {
> +  if (!HasRemoteContent() && mTransparencyMode == eTransparencyTransparent) {

I'd like this to be explicitly blocked by a pref, so that even if remote=true would be specified this won't cause OMTC to be used for these windows on release.

So for example:

if (!(HasRemoteContent() && IsPrefSet(OOP)) && mTransparencyMode == eTransparencyTransparent) {
Attachment #8868665 - Flags: review?(bas) → review+
(Assignee)

Comment 38

2 years ago
mozreview-review-reply
Comment on attachment 8868665 [details]
Bug 1365660: Part 5b - Enable compositing for popups with remote content on Windows. .schouten

https://reviewboard.mozilla.org/r/140256/#review147108

> I'd like this to be explicitly blocked by a pref, so that even if remote=true would be specified this won't cause OMTC to be used for these windows on release.
> 
> So for example:
> 
> if (!(HasRemoteContent() && IsPrefSet(OOP)) && mTransparencyMode == eTransparencyTransparent) {

This attribute only ever exists when remote WebExtensions are enabled, and if it needs to be switched off, I'd rather remote extensions also be switched off, rather than having only one or the other enabled.

I suppose I could add another pref and unconditionally disable remote extensions unless it's enabled, though, if you think it's worth it.
(Assignee)

Comment 39

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a54f3cba5c24d4a12fe3029282fcab8a7b32a3e8
Bug 1365660: Part 5b - Enable compositing for popups with remote content on Windows. r=bas
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #38)
> Comment on attachment 8868665 [details]
> Bug 1365660: Part 5b - Enable compositing for popups with remote content on
> Windows. .schouten
> 
> https://reviewboard.mozilla.org/r/140256/#review147108
> 
> > I'd like this to be explicitly blocked by a pref, so that even if remote=true would be specified this won't cause OMTC to be used for these windows on release.
> > 
> > So for example:
> > 
> > if (!(HasRemoteContent() && IsPrefSet(OOP)) && mTransparencyMode == eTransparencyTransparent) {
> 
> This attribute only ever exists when remote WebExtensions are enabled, and
> if it needs to be switched off, I'd rather remote extensions also be
> switched off, rather than having only one or the other enabled.
> 
> I suppose I could add another pref and unconditionally disable remote
> extensions unless it's enabled, though, if you think it's worth it.

That is fine with me as well, whatever you think is the preferred route. I just don't want someone at the front-end to be able to add a remote attribute that makes this codepath be used without it ever going passed a widget/gfx person for review.
Flags: needinfo?(bas)

Updated

2 years ago
No longer blocks: 1356317

Updated

2 years ago
Priority: P1 → P3
Can we close this bug and move any follow-up work to another bug? It's confusing to leave this open when the patches landed months ago.
Flags: needinfo?(kmaglione+bmo)
Status: NEW → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(kmaglione+bmo)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1444595

Comment 43

a year ago
mozreview-review-reply
Comment on attachment 8868664 [details]
Bug 1365660: Part 5a - Enable compositing for popups with remote content on Linux.

https://reviewboard.mozilla.org/r/140254/#review145768

> Doesn't GetTransparencyMode() return eTransparencyTransparent when HasRemoteContent() due to this?
> http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/xul/nsMenuPopupFrame.cpp#315
> 
> That would make the HasRemoteContent() check here unnecessary?
> 
> As the HasRemoteContent() test seems to be merely a workaround to avoid unresolved issues, I'd prefer not to add the unnecessary workaround.

The HasRemoteContent() condition makes more sense if nsMenuPopupFrame::CreateWidgetForView() does not force remote popups to eTransparencyOpaque.  A comment would be helpful to explain that having black in place of transparent areas on systems with non-compositing window managers is preferable to the remote content not drawing at all, which is what happens with basic layers.
No longer depends on: 1444595

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.