Open
Bug 1320361
Opened 7 years ago
Updated 1 year ago
Set the "noautohide" attribute properly before opening popup notifications
Categories
(Firefox :: Site Identity, defect, P3)
Firefox
Site Identity
Tracking
()
NEW
Tracking | Status | |
---|---|---|
firefox53 | --- | affected |
People
(Reporter: past, Unassigned)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [fxprivacy])
Attachments
(1 file, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
Details |
In bug 1109868 comment 16 Neil said that panels created with noautohide="false" (on Linux at least) are created as top-level windows that don't react to the parent window being minimized. This makes the permission prompts remain open when the browser window is minimized. To fix this we could swap the logic and make noautohide="true" the default on creation and only remove the attribute before opening the popup. This way persistent popups will be properly minimized and old-style, non-persistent ones will continue to have the same issue on minimize (i.e. no change).
Comment 1•7 years ago
|
||
I believe this change would make the panel always act like noautohide on Linux though (from what I remember dealing with noautohide in UITour.jsm). i.e. non-persistent popups will become persistent. See bug 1109868 comment 21 for the approach we use in UITour.jsm.
Updated•7 years ago
|
Iteration: --- → 53.2 - Dec 12
Priority: -- → P1
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Updated•7 years ago
|
Assignee: nobody → nhnt11
Updated•7 years ago
|
Iteration: 53.2 - Dec 12 → 53.5 - Jan 23
Updated•7 years ago
|
Flags: qe-verify?
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #1) > I believe this change would make the panel always act like noautohide on > Linux though (from what I remember dealing with noautohide in UITour.jsm). > i.e. non-persistent popups will become persistent. See bug 1109868 comment > 21 for the approach we use in UITour.jsm. I couldn't reproduce this. I tested this one line patch on OS X and Linux, and the behavior of all the popups I tested was unchanged, except that they were correctly hidden when Firefox was defocused.
Updated•7 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8827111 [details] Bug 1320361 - Make PopupNotifications panels noautohide by default. https://reviewboard.mozilla.org/r/104934/#review105684 The change looks and works fine, but there are a lot of test failures on try, so this is not ready to land yet.
Attachment #8827111 -
Flags: review?(past) → review-
Reporter | ||
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8827111 -
Flags: review?(past)
Updated•7 years ago
|
Iteration: 53.5 - Jan 23 → 54.1 - Feb 6
Updated•7 years ago
|
Iteration: 54.1 - Feb 6 → 54.2 - Feb 20
Comment 6•7 years ago
|
||
I've gotten nowhere trying to debug these test failures. Using a one-click loaner, I noticed that once popups were dismissed, focus was not restored to the window. But adding code to focus the window didn't help - more failures, albeit seemingly different ones. I got a test machine loaner slave but I couldn't reproduce the failures on that machine. I'll have another go with another one-click loaner but I don't expect to be successful - at this point I've been looking long enough that I'll probably miss even obvious things (and somehow I expect I'm already missing something obvious) so someone should probably take over :(
Updated•7 years ago
|
Assignee: nhnt11 → nobody
Status: ASSIGNED → NEW
Comment 7•7 years ago
|
||
:past marked this for triage but since we won't meet on monday, I'm wondering if you can help load balance this one. Thank you.
Flags: needinfo?(past)
Whiteboard: [fxprivacy] → [fxprivacy][triage]
Reporter | ||
Comment 8•7 years ago
|
||
I've already asked paolo to take a look at this.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Flags: needinfo?(past)
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8827111 [details] Bug 1320361 - Make PopupNotifications panels noautohide by default. https://reviewboard.mozilla.org/r/104934/#review114954 These are the results of my investigation so far. ::: toolkit/modules/PopupNotifications.jsm:642 (Diff revision 2) > } > > let browser = this.panel.firstChild && > this.panel.firstChild.notification.browser; > this.panel.hidePopup(); > + this.panel.ownerGlobal.focus(); This fix restores focus to the window after a notification is closed, but apparently doesn't solve the focus issues I've observed on Linux. On Windows, at least one of the tests seems to fail because the focus is taken away from the window when the panel is opened: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/cb6104a624d3cac2a9e3712e56e405880743dc54f4f37ca1c0311133d973ff574b63486847d286d37b27758e122e480626e24460a025e7b2ddc55d46a48e7838 The test stops here: https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/browser/base/content/test/popupNotifications/browser_popupNotification_4.js#143 This is a known intermittent (bug 1325265) but the "noautohide" change may have worsened it. ::: toolkit/modules/PopupNotifications.jsm:864 (Diff revision 2) > > _showPanel: function PopupNotifications_showPanel(notificationsToShow, anchorElement) { > this.panel.hidden = false; So, if I understand the previous comments in this bug correctly, setting "hidden" to false here means that the widget created for the panel on Linux (and maybe other platforms?) will depend on the state of the "noautohide" attribute when the next layout flush happens. Unfortunately, this function has a number of early returns, so we may or may not reach the code that sets the "noautohide" attribute based on the type of popup notification that is displayed. In case of early return, the default value of the "noautohide" attribute will influence the type of widget that is created, in the other case, the type of notification that is displayed first for this window will influence the type of widget that is created. Some test failures may be related to the first case happening at some point. Forcing a layout flush is something that Matt suggested and maybe it's what we need here to get consistent behavior. We still have to figure out the focus issues created by changing the type of widget though.
Comment 10•7 years ago
|
||
Johann, we may have to investigate how bug 1340112 may affect or be affected by this bug, or by the case of bug 1325265 that I mentioned above. I know you've done some investigations around focus and panels so you may have some insights.
Flags: needinfo?(jhofmann)
Updated•7 years ago
|
Summary: Make PopupNotifications panels noautohide by default → Popup notifications don't hide when the window is minimized on Linux.
Comment 11•7 years ago
|
||
I have a patch, but it runs into the focus-related bug 545265 which is specific to Linux environments that use the Metacity window manager. Basically, until that bug is solved we either keep the bug with the minimize behavior or break the Linux tests. The latter would possibly also result in a broken focus behavior, like the inability to use TAB after clicking in the popup window.
Depends on: 545265
Flags: needinfo?(jhofmann)
Updated•7 years ago
|
Attachment #8827111 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Whiteboard: [fxprivacy] → [fxprivacy][triage]
Updated•7 years ago
|
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Updated•7 years ago
|
Assignee: paolo.mozmail → nobody
Status: ASSIGNED → NEW
Iteration: 54.2 - Feb 20 → ---
Priority: P1 → --
Summary: Popup notifications don't hide when the window is minimized on Linux. → Set the "noautohide" attribute properly before opening popup notifications
Whiteboard: [fxprivacy] → [fxprivacy][triage]
Comment 14•7 years ago
|
||
The MozReview repository seems locked right now, however the patch is small enough that you can review the changeset here: https://hg.mozilla.org/try/rev/127151ac31ba0f772482c582fd16f26cc9807b70
Flags: needinfo?(jhofmann)
Comment 15•7 years ago
|
||
Since that was the patch I just reviewed I assume I can cancel my needinfo :)
Flags: needinfo?(jhofmann)
Updated•7 years ago
|
Priority: -- → P3
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Updated•1 year ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•