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)

defect

Tracking

()

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)

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).
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.
Iteration: --- → 53.2 - Dec 12
Priority: -- → P1
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Assignee: nobody → nhnt11
Iteration: 53.2 - Dec 12 → 53.5 - Jan 23
Flags: qe-verify?
(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.
Status: NEW → ASSIGNED
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-
Flags: qe-verify? → qe-verify+
Iteration: 53.5 - Jan 23 → 54.1 - Feb 6
Iteration: 54.1 - Feb 6 → 54.2 - Feb 20
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 :(
Assignee: nhnt11 → nobody
Status: ASSIGNED → NEW
: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]
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 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.
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)
Summary: Make PopupNotifications panels noautohide by default → Popup notifications don't hide when the window is minimized on Linux.
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)
Attachment #8827111 - Attachment is obsolete: true
Whiteboard: [fxprivacy] → [fxprivacy][triage]
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Depends on: 1345449
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]
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)
Since that was the patch I just reviewed I assume I can cancel my needinfo :)
Flags: needinfo?(jhofmann)
Priority: -- → P3
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.