Permission prompts misplaced when dragging tab out to be its own window

VERIFIED FIXED in Firefox 60

Status

()

defect
P1
normal
VERIFIED FIXED
a year ago
22 days ago

People

(Reporter: jib, Assigned: arai)

Tracking

({regression})

60 Branch
Firefox 61
Unspecified
macOS
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60 verified, firefox61 verified)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
STRs:
  1. Open browser.
  2. Open https://jsfiddle.net/jib1/r60bzmrs/ in a new (second) tab.
  3. Don't answer the prompt.
  4. Instead, drag this tab into its own window.

Expected result:
  Permission prompt remains open, hanging off the camera icon in the url bar.

Actual result:
  Permission prompt remains open, but severely misplaced, to the right and down.

  See attached image.

Regression range:

 3:07.35 INFO: Narrowed inbound regression window from [7cc75e6d, 84ea4220] (4 builds) to [c842abb7, 84ea4220] (2 builds) (~1 steps left)
 3:07.35 INFO: No more inbound revisions, bisection finished.
 3:07.35 INFO: Last good revision: c842abb7cfbf39e0c1cfb9a1f3a1d6415cd10744
 3:07.35 INFO: First bad revision: 84ea422093dd615bd191a3e579ed299d96c802f1
 3:07.35 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c842abb7cfbf39e0c1cfb9a1f3a1d6415cd10744&tochange=84ea422093dd615bd191a3e579ed299d96c802f1

My guess is bug 1193394.
Hmm works for me on Linux.  Need dual monitors?
Component: General → Site Identity and Permission Panels
Priority: P2 → --
(Assignee)

Comment 2

a year ago
reproduces for me on macOS.
(not dual monitors but mirroring)
Are you looking into this? It would be great to fix this in 60...

Thanks :)
Flags: needinfo?(arai.unmht)
Priority: -- → P1
(Assignee)

Updated

a year ago
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
(Assignee)

Comment 4

a year ago
interestingly, Inspector's [Pick an element from the page] feature detects the prompt's coordinate as the expected place.
so, when I hover on the expected place (where no prompt is shown), the red dashed border is rendered to the misplaced prompt.
(Assignee)

Comment 5

a year ago
PopupNotifications.prototype._showPanel is called twice after detaching the tab.
If I add early return for 1st case (where setTimeout isn't in backtrace), the popup is shown in correct place.

[1]

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/modules/PopupNotifications.jsm#945
> PopupNotifications.prototype = {
> ...
>   _showPanel: function PopupNotifications_showPanel(notificationsToShow, anchorElement) {

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/modules/PopupNotifications.jsm#1137
> PopupNotifications.prototype = {
> ...
>   _update: function PopupNotifications_update(notifications, anchors = new Set(), dismissShowing = false) {
> ...
>     if (notificationsToShow.length > 0) {
>       let anchorElement = anchors.values().next().value;
>       if (anchorElement) {
>         this._showPanel(notificationsToShow, anchorElement);

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/modules/PopupNotifications.jsm#591-592
> PopupNotifications.prototype = {
> ...
>   anchorVisibilityChange() {
>     let suppress = this._shouldSuppress();
>     if (!suppress) {
>       // If notifications are not suppressed, always update the visibility.
>       this._suppress = false;
>       let notifications =
>         this._getNotificationsForBrowser(this.tabbrowser.selectedBrowser);
>       this._update(notifications, this._getAnchorsForNotifications(notifications,
>         getAnchorFromBrowser(this.tabbrowser.selectedBrowser)));

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/browser.js#2952
> function UpdatePopupNotificationsVisibility() {
> ...
>   // Notify PopupNotifications that the visible anchors may have changed. This
>   // also checks the suppression state according to the "shouldSuppress"
>   // function defined earlier in this file.
>   PopupNotifications.anchorVisibilityChange();

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/browser.js#2939
> function SetPageProxyState(aState) {
> ...
>   UpdatePopupNotificationsVisibility();

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/browser.js#2803
> function URLBarSetURI(aURI) {
> ...
>   SetPageProxyState(valid ? "valid" : "invalid");

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/browser.js#4667
> var XULBrowserWindow = {
> ...
>   onLocationChange(aWebProgress, aRequest, aLocationURI, aFlags) {
> ...
>     if (aWebProgress.isTopLevel) {
> ...
>       URLBarSetURI(aLocationURI);

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/tabbrowser.js#696
> window._gBrowser = {
> ...
>   _callProgressListeners(aBrowser, aMethod, aArguments, aCallGlobalListeners = true, aCallTabsListeners = true) {
>     var rv = true;
> 
>     function callListeners(listeners, args) {
>       for (let p of listeners) {
>         if (aMethod in p) {
>           try {
>             if (!p[aMethod].apply(p, args))

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/tabbrowser.js#709
> window._gBrowser = {
> ...
>   _callProgressListeners(aBrowser, aMethod, aArguments, aCallGlobalListeners = true, aCallTabsListeners = true) {
> ...
>     if (aCallGlobalListeners && aBrowser == this.selectedBrowser) {
>       callListeners(this.mProgressListeners, aArguments);
>     }

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/tabbrowser.js#949-951
> window._gBrowser = {
> ...
>   updateCurrentBrowser(aForceUpdate) {
> ...
>     let webProgress = newBrowser.webProgress;
>     this._callProgressListeners(null, "onLocationChange",
>                                 [webProgress, null, newBrowser.currentURI, 0],
>                                 true, false);

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/tabbrowser.js#3111
> window._gBrowser = {
> ...
>   swapBrowsersAndCloseOther(aOurTab, aOtherTab) {
> ...
>     // If the tab was already selected (this happpens in the scenario
>     // of replaceTabWithWindow), notify onLocationChange, etc.
>     if (aOurTab.selected)
>       this.updateCurrentBrowser(true);

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/browser.js#1397
>   onLoad() {
> ...
>     if (tabToOpen instanceof XULElement) {
>       // Clear the reference to the tab from the arguments array.
>       window.arguments[0] = null;
> 
>       // Stop the about:blank load
>       gBrowser.stop();
>       // make sure it has a docshell
>       gBrowser.docShell;
> 
>       try {
>         gBrowser.swapBrowsersAndCloseOther(gBrowser.selectedTab, tabToOpen);

[2]

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/modules/PopupNotifications.jsm#945
> PopupNotifications.prototype = {
> ...
>   _showPanel: function PopupNotifications_showPanel(notificationsToShow, anchorElement) {


https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/modules/PopupNotifications.jsm#1137
> PopupNotifications.prototype = {
> ...
>   _update: function PopupNotifications_update(notifications, anchors = new Set(), dismissShowing = false) {
> ...
>     if (notificationsToShow.length > 0) {
>       let anchorElement = anchors.values().next().value;
>       if (anchorElement) {
>         this._showPanel(notificationsToShow, anchorElement);

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/modules/PopupNotifications.jsm#639-641
> PopupNotifications.prototype = {
> ...
>   handleEvent(aEvent) {
>     switch (aEvent.type) {
> ...
>       case "TabSelect":
>         let self = this;
>         // This is where we could detect if the panel is dismissed if the page
>         // was switched. Unfortunately, the user usually has clicked elsewhere
>         // at this point so this value only gets recorded for programmatic
>         // reasons, like the "Learn More" link being clicked and resulting in a
>         // tab switch.
>         this.nextDismissReason = TELEMETRY_STAT_DISMISSAL_LEAVE_PAGE;
>         // setTimeout(..., 0) needed, otherwise openPopup from "activate" event
>         // handler results in the popup being hidden again for some reason...
>         this.window.setTimeout(function() {
>           self._update();
>         }, 0);

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/tabbrowser.js#1022-1029
>   updateCurrentBrowser(aForceUpdate) {
> ...
>     if (!this._previewMode) {
>       // We've selected the new tab, so go ahead and notify listeners.
>       let event = new CustomEvent("TabSelect", {
>         bubbles: true,
>         cancelable: false,
>         detail: {
>           previousTab: oldTab
>         }
>       });
>       newTab.dispatchEvent(event);

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/tabbrowser.js#3111
> window._gBrowser = {
> ...
>   swapBrowsersAndCloseOther(aOurTab, aOtherTab) {
> ...
>     // If the tab was already selected (this happpens in the scenario
>     // of replaceTabWithWindow), notify onLocationChange, etc.
>     if (aOurTab.selected)
>       this.updateCurrentBrowser(true);

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/browser.js#1397
>   onLoad() {
> ...
>     if (tabToOpen instanceof XULElement) {
>       // Clear the reference to the tab from the arguments array.
>       window.arguments[0] = null;
> 
>       // Stop the about:blank load
>       gBrowser.stop();
>       // make sure it has a docshell
>       gBrowser.docShell;
> 
>       try {
>         gBrowser.swapBrowsersAndCloseOther(gBrowser.selectedTab, tabToOpen);
(Assignee)

Comment 6

a year ago
https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/modules/PopupNotifications.jsm#1010
>   _showPanel: function PopupNotifications_showPanel(notificationsToShow, anchorElement) {
> ...
>     this._hidePanel().then(() => {

If I put 1 second wait after this step, the popup is shown in expected place.
So I think the execution timing difference of this block is causing the issue.
(Assignee)

Comment 7

a year ago
the 2nd call to _showPanel returns without calling openPopup, here:

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/modules/PopupNotifications.jsm#1004
>     if (this.isPanelOpen && this._currentAnchorElement == anchorElement) {
>       notificationsToShow.forEach(function(n) {
>         this._fireCallback(n, NOTIFICATION_EVENT_SHOWN);
>       }, this);
> 
>       // Make sure we update the noautohide attribute on the panel, in case it changed.
>       if (notificationsToShow.some(n => n.options.persistent)) {
>         this.panel.setAttribute("noautohide", "true");
>       } else {
>         this.panel.removeAttribute("noautohide");
>       }
> 
>       // Let tests know that the panel was updated and what notifications it was
>       // updated with so that tests can wait for the correct notifications to be
>       // added.
>       let event = new this.window.CustomEvent("PanelUpdated",
>                                               {"detail": notificationIds});
>       this.panel.dispatchEvent(event);
>       return;
>     }
(Assignee)

Comment 8

a year ago
The panel is closed for the 1st call.

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/toolkit/modules/PopupNotifications.jsm#710
>   _hidePanel: function PopupNotifications_hide() {
>     if (this.panel.state == "closed") {
>       return Promise.resolve();

So, the resolution handler will be executed in the same MicroTask checkpoint.
Maybe something is not yet ready around anchor or popup at that point?
(Assignee)

Comment 9

a year ago
The misplacement happens even if I use openPopupAtScreen, with correct x and y values.
(or even if fixed value like 100,100)
(Assignee)

Comment 10

a year ago
looks like, the x and y coordinates are doubled.
If I specify (0,0), the popup is expected to be shown just below the topmost menu bar, but it's shifted down about the height of the menu bar.

I suspect the conversion between the app units, CSS pixels, or device pixels are not yet initialized properly.
(Assignee)

Comment 11

a year ago
CCing people who touched code around popup position
(Assignee)

Comment 12

a year ago
simple workaround here is just that wait for the next Event tick by setTimeout(..., 0) after _hidePanel.
if it's urgent, applying it will solve the issue.

I'll continue investigating the actual reason.
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 13

a year ago
There's no difference between expected case and wrong case, for mRect, viewPoint, and some other values here.

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/layout/xul/nsMenuPopupFrame.cpp#1700
>   nsBoxFrame::SetPosition(viewPoint - GetParent()->GetOffsetTo(rootFrame));

maybe it's happening in more underlying code?
(Assignee)

Comment 14

a year ago
I also don't see any difference in nsChildView.mBounds

Then, Xcode's Accessibility Inspector also reports the expected frame position/size.
(see attached image, that shows misplaced popup and overlay on expected place)
So looks like it's definitely inside widget code, might be inside Cocoa side.
(Assignee)

Comment 15

a year ago
anyway, given that the bug is happening deep inside the widget code (and also there's almost no simple way to test this on automated test),
I'd like to apply the workaround here, and leave the actual fix to another bug.
I'll post the patch after try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ce6593b8031462cef956bf6e0c0d6b1b1908c74
(Assignee)

Updated

a year ago
Attachment #8962594 - Attachment description: inspector.png → Xcode's Accessibility Inspector also reports non-misplaced position
(Assignee)

Comment 16

a year ago
(In reply to Tooru Fujisawa [:arai] from comment #14)
> Created attachment 8962594 [details]
> Xcode's Accessibility Inspector also reports non-misplaced position
> 
> I also don't see any difference in nsChildView.mBounds
> 
> Then, Xcode's Accessibility Inspector also reports the expected frame
> position/size.
> (see attached image, that shows misplaced popup and overlay on expected
> place)
> So looks like it's definitely inside widget code, might be inside Cocoa side.

forgot to clarify that I used openPopupAtScreen with fixed value, instead of openPopup with anchor.
so the "expected" place is not under the indicator.
the point here is that the overlay and the popup aren't in the same place.
(Assignee)

Comment 17

a year ago
Looks like the workaround doesn't work well.
It's causing another failures.
I guess it delayed the execution too much :/

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ce6593b8031462cef956bf6e0c0d6b1b1908c74&group_state=expanded&selectedJob=170459264
(Assignee)

Comment 18

a year ago
The issue is here.

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/test/popupNotifications/browser_popupNotification_no_anchors.js#60-61
>     async onShown(popup) {
>       await promiseTabLoadEvent(gBrowser.selectedTab, "about:blank");
> 
>       checkPopup(popup, this.notifyObj);
>       is(document.getElementById("geo-notification-icon").boxObject.width, 0,
>          "geo anchor shouldn't be visible");
>       is(popup.anchorNode.id, "identity-icon",
>          "notification anchored to identity icon");

popup.anchorNode becomes null while awaiting on promiseTabLoadEvent.
without the workaround, the popup's state is "showing",
but with the workaround the, state is "closed".

Then, that's not what the testcase is testing, so I think we can just skip the check, or maybe check only if the state is not "closed",
but basically I think it's just racing against the closing popup.
(Assignee)

Comment 19

a year ago
(In reply to Tooru Fujisawa [:arai] from comment #18)
> Then, that's not what the testcase is testing,

I meant, the intention there shouldn't be "the popup is still showing after loading another page",
but just that "if the popup is still shown, it should anchor to the given node".
(Assignee)

Comment 20

a year ago
Error on toolkit/mozapps/extensions/test/xpinstall/browser_httphash6.js seems to be that, either the test or the PopupNotification.jsm is looking wrong notification data than the shown one.
this seems to also be timing issue...
(Assignee)

Comment 21

a year ago
(In reply to Tooru Fujisawa [:arai] from comment #20)
> Error on toolkit/mozapps/extensions/test/xpinstall/browser_httphash6.js
> seems to be that, either the test or the PopupNotification.jsm is looking
> wrong notification data than the shown one.
> this seems to also be timing issue...

looks like, both addon-webext-permissions and addon-install-failed are shown at the same time,
and closing addon-install-failed doesn't cause the next popupshown, because addon-webext-permissions is already there.
(Assignee)

Comment 23

a year ago
mmm, the patch is getting larger :P
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e5c30d4cf9dfc017f884b649b90e977e035af46&group_state=expanded

I wonder if we should go with this workaround way...
(anyway, the test should be fixed, but I'm worrying about uplift...)
(Assignee)

Comment 24

a year ago
given that there's so many intermittent issue happens across testcases because of the workaround,
I think it's not a good idea to apply this and uplift the patch.
I'll look for the actual fix for the underlying issue.
(Assignee)

Comment 25

a year ago
CCing people who touched cocoa window
(Assignee)

Comment 26

a year ago
confirmed that the issue happens also on macOS with non-retina display, thanks to :mantaroh.
So this could be something different than high-dpi issue.
(Assignee)

Comment 27

a year ago
now I'm looking nsDeviceContext::SetDPI, which sounds like related to DPI and also there are some branched that depends on the existence of mWidget, that can be related to window initialization
(Assignee)

Comment 28

a year ago
there are multiple cases happening in nsDeviceContext::SetDPI (I haven't checked which device context they are tho):
  * if mWidget is null
    * mAppUnitsPerDevPixelAtUnitFullZoom=60.000000
      mAppUnitsPerPhysicalInch=5760.000000
  * if mWidget is non-null
    * mAppUnitsPerDevPixelAtUnitFullZoom=30.000000
      mAppUnitsPerPhysicalInch=8940.000000
    * mAppUnitsPerDevPixelAtUnitFullZoom=30.000000
      mAppUnitsPerPhysicalInch=2880.000000

anyway, mAppUnitsPerDevPixelAtUnitFullZoom becomes 60.0 only if mWidget is null.
and such case happens just before calling openPopup.
(Assignee)

Comment 29

a year ago
forcibly setting those properties to fixed values doesn't solve the misplacement,
so at lease this won't be the only reason.
(the issue can be totally different, or this is just the result of more underlying issue)
(Assignee)

Comment 30

a year ago
also, one more observation.
mouse hover event on buttons in popup works on *expected* placement.
so, if I hover over the place that the button is expected to be shown, the button in misplaced popup is highlighted.
You may want to do your investigation first and then post your findings as one comment when you either found a solution or got stuck somewhere. Otherwise, the important info gets lost in all the comments.
(Assignee)

Comment 32

a year ago
I have no idea where to look into next.
can you help me?
Flags: needinfo?(spohl.mozilla.bugs)
Unfortunately, I don't have any pointers off-hand and it will be a while before I can look into this myself (currently working on a couple top-crashers).
Flags: needinfo?(spohl.mozilla.bugs)
The fact that Xcode's Accessibility Inspector also reports the correct position makes me think that this could have something to do with the window transform.

Arai, could you check whether disabling nsCocoaWindow::SetWindowTransform fixes the bug?

It's possible that mBounds.x/.y have outdated values at
https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/widget/cocoa/nsCocoaWindow.mm#2318
Oh, we even have a pref for it, widget.window-transforms.disabled.
(Assignee)

Comment 36

a year ago
(In reply to Markus Stange [:mstange] from comment #34)
> The fact that Xcode's Accessibility Inspector also reports the correct
> position makes me think that this could have something to do with the window
> transform.
> 
> Arai, could you check whether disabling nsCocoaWindow::SetWindowTransform
> fixes the bug?
> 
> It's possible that mBounds.x/.y have outdated values at
> https://searchfox.org/mozilla-central/rev/
> a0665934fa05158a5a943d4c8b277465910c029c/widget/cocoa/nsCocoaWindow.mm#2318
(In reply to Markus Stange [:mstange] from comment #35)
> Oh, we even have a pref for it, widget.window-transforms.disabled.

Thank you for the info :D
Yes, flipping widget.window-transforms.disabled to true fixes the bug.
I'll look into the details.
(Assignee)

Comment 37

a year ago
(In reply to Markus Stange [:mstange] from comment #34)
> It's possible that mBounds.x/.y have outdated values at
> https://searchfox.org/mozilla-central/rev/
> a0665934fa05158a5a943d4c8b277465910c029c/widget/cocoa/nsCocoaWindow.mm#2318

Looks like no values there are wrong/outdated.
Just calling CGSSetWindowTransform again with the same parameters after 1ms with a timer callback fixes the position, after the current call that misplaces the popup,
it would mean that the values are correct, but the timing is problematic.

also, I confirmed that delaying the CGSSetWindowTransform call for a second doesn't change the window position, between before/after call.

so, what I can think of is, CGSSetWindowTransform disables the normal window positioning for successful case (is it correct?),
that results in placing the popup in the expected position while both the transform and normal window positioning have the same x,y values.
in failure case, CGSSetWindowTransform somehow doesn't disable normal window positioning, and that results in placing the window in doubled x,y values.
I haven't yet figure out if it's really true, or how it can happen tho.

:mstange, how do you think?
Flags: needinfo?(mstange)
(Assignee)

Comment 38

a year ago
early-returning from nsCocoaWindow::SetWindowTransform when !mWindow.visible, or when mWindow.frame has zero size fixes the issue for me.
so I think we shouldn't call CGSSetWindowTransform when the window is not visible.

here's try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eed318976e6df4a55ee2162de16e0689b7859f3e
(Assignee)

Comment 39

a year ago
So, calling CGSSetWindowTransform while the window (notification popup) is not visible causes the issue.
This patch adds early-return into nsCocoaWindow::SetWindowTransform for such case.

This fixes the misplacement for me, with the steps in comment #0.

This patch doesn't contain testcase, since this is not observable from window parameter, but only visually, and I think taking a screenshot isn't a good idea.
Attachment #8965192 - Flags: review?(mstange)
Comment on attachment 8965192 [details] [diff] [review]
Do not call CGSSetWindowTransform if the window is not visible.

Review of attachment 8965192 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thank you!

Could you change the syntax a bit, though, for consistency? We're not using the .property syntax for accessing ObjC properties in most other places in the widget code.
if (![mWindow isVisible] || NSIsEmptyRect([mWindow frame])) {
Attachment #8965192 - Flags: review?(mstange) → review+
(In reply to Tooru Fujisawa [:arai] from comment #37)
> so, what I can think of is, CGSSetWindowTransform disables the normal window
> positioning for successful case (is it correct?),

That certainly sounds plausible! CGSSetWindowTransform is not a public API, so it doesn't really have any specified behavior for the edge cases.

> in failure case, CGSSetWindowTransform somehow doesn't disable normal window
> positioning, and that results in placing the window in doubled x,y values.

Interesting. A bit strange, but believable.
Flags: needinfo?(mstange)
(Assignee)

Comment 42

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fce6a9a47b75622d79de27d969134b7e6005e90
Bug 1448132 - Do not call CGSSetWindowTransform if the window is not visible. r=mstange
(Assignee)

Comment 43

a year ago
Here's updated patch, rebased onto mozilla-beta.

Approval Request Comment
> [Feature/Bug causing the regression]
bug 1193394

> [User impact if declined]
Notification popup is misplaced when a tab with notification popup is detached

> [Is this code covered by automated tests?]
No.
This issue is not observable in automation.

> [Has the fix been verified in Nightly?]
Not yet.
it's verified on taskcluster build (comment #38).

> [Needs manual test from QE? If yes, steps to reproduce]
Yes.

Steps:
  1. Open Nightly with clean profile on macOS
  2. Open https://jsfiddle.net/jib1/r60bzmrs/ in a new (second) tab
  3. Don't answer the notification popup
  4. Detach the tab by drag-and-drop
  5. Confirm that the notification popup is placed in the same place as before detaching
     (that is, under the icon in location bar)

> [List of other uplifts needed for the feature/fix]
None

> [Is the change risky?]
No

> [Why is the change risky/not risky?]
This just disables optimization for some situation that wasn't working before.
This fallbacks to non-optimized path.

> [String changes made/needed]
None
Attachment #8965192 - Attachment is obsolete: true
Attachment #8965552 - Flags: review+
Attachment #8965552 - Flags: approval-mozilla-beta?

Comment 44

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6fce6a9a47b7
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Flags: qe-verify+
Comment on attachment 8965552 [details] [diff] [review]
Do not call CGSSetWindowTransform if the window is not visible. r=mstange

Fix a new regression in 60. Approved for 60.0b11.
Attachment #8965552 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Jan-Ivar Bruaroey, I can't reproduce this issue on the affected Nightly (2018-03-22). I tried on macOS 10.10, 10.11, 10.12, 10.13. Can you still reproduce it? And if you do, can you provide additional information on how to reproduce it?
Flags: needinfo?(jib)
(Assignee)

Comment 48

a year ago
I can reproduce the issue on Nightly (2018-03-22) with the exact same step as comment #0 with clean profile, on macOS 10.12.6,
and I confirmed the fix on Nightly (2018-04-08).

If it doesn't reproduce, what happens instead?
The popup is placed in the expected place in the detached window?
Flags: needinfo?(catalin.sasca)
Posted video re-test.mp4
Hi Tooru, for me the popup is placed in the expected place. I've retested again on macOS 10.12.6 with Nightly (2018-03-22) and clean profile with STR from Comment 0.

I'll put a screenshare with it. If I do something wrong please let me know. Thank you.
Flags: needinfo?(catalin.sasca)
(Assignee)

Comment 50

a year ago
(In reply to Sasca Catalin, QA [:csasca] from comment #49)
> Created attachment 8967657 [details]
> re-test.mp4
> 
> Hi Tooru, for me the popup is placed in the expected place. I've retested
> again on macOS 10.12.6 with Nightly (2018-03-22) and clean profile with STR
> from Comment 0.
> 
> I'll put a screenshare with it. If I do something wrong please let me know.
> Thank you.

It looks like same steps as mine.

Perhaps the issue is not reproducible on all environment, but there's some other unknown reasons that the API fails.
Since we're using a private API of Core Graphics, it's not guaranteed that it always works unexpectedly.

Comment 51

a year ago
I have reproduced the issue using Firefox 61.0a1 (2018-03-22) on Mac OS X 10.12.

On the latest Firefox Nightly (Build ID: 20180422223305) and latest Firefox Beta 60.0b14, the issue cannot be reproduced on Mac OS X 10.12, Windows 10 x64 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
(Reporter)

Updated

a year ago
Flags: needinfo?(jib)
You need to log in before you can comment on or make changes to this bug.