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?

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 :)

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.

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);

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.

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; > }

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?

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

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.

CCing people who touched code around popup position

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.

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?

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.

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

(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.

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

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.

(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".

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...

(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.

another try with fix for those tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=44d9a68af1f1af7281b60c48b9831456f77a2807

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...)

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.

CCing people who touched cocoa window

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.

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

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.

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)

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.

I have no idea where to look into next. can you help me?

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).

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.

(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.

(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?

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

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.

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])) {

(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.

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

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

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.

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?

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?

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.

(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.

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.