1369899 - Listen for MozDOMFullscreenEvents for update doorhangers on OSX

Status: RESOLVED FIXED

(Toolkit :: Application Update, enhancement)

Description

[Alex Thayer [:alexical] (she/her)]

While we now correctly display doorhangers while in fullscreen on OSX, they don't come back into view when exiting fullscreen videos. Listening to MozDOMFullscreen events instead of fullscreen events on OSX should fix this.

Comment 1

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1369899 - Listen to MozDOMFullscreen events

While we are now correctly evaluating the fullscreen situation
in OSX so that our update doorhangers behave as if we're in
windowed mode, we aren't correctly listening for state changes.
This addresses that, and tries to limit the chattiness by only
listening to the fullscreen events that we care about.

Review commit: https://reviewboard.mozilla.org/r/145490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145490/

Comment 2

[:Gijs (he/him)]

Comment on attachment 8874042 [details]
Bug 1369899 - Listen to MozDOMFullscreen events

https://reviewboard.mozilla.org/r/145490/#review149782

This looks OK but my suggestion to simplify this using defineLazyPreferenceGetter will mean a lot of the code will change, so I guess I might as well look at it again.

The other reason there's no r+ is that it took me so long to figure out what we're fixing, that I thought maybe it's worth having an automated test for it? :-)
This applies both to the Windows case where we can change the autohide pref, and to the OS X case where we can't.

::: browser/components/customizableui/content/panelUI.js:379
(Diff revision 1)
>      let panelState = this.notificationPanel.state;
>  
>      return panelState == "showing" || panelState == "open";
>    },
>  
> +  get autoHideToolbarInFullScreen() {

So... I think you can tell the constructor to define this using XPCOMUtils.defineLazyPreferenceGetter(). You can use the last argument (transform function) to return false on OS X at all times.

Then you can do the listener change handling (that is in the pref observer right now) in the observer function you pass to it, and you don't need to manually add/remove the observers, check that the right pref is being changed in the observer code, etc.

So it should be a slightly smaller patch. :-)

Comment 3

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8874042 [details]
Bug 1369899 - Listen to MozDOMFullscreen events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145490/diff/1-2/

Comment 4

[:Gijs (he/him)]

Comment on attachment 8874042 [details]
Bug 1369899 - Listen to MozDOMFullscreen events

https://reviewboard.mozilla.org/r/145490/#review149850

::: browser/components/customizableui/test/browser_panelUINotifications_fullscreen_noAutoHideToolbar.js:13
(Diff revision 2)
> +      set: [
> +        ["browser.fullscreen.autohide", false],
> +      ]});
> +  }
> +
> +  let doc = document;

You only use this once, at the bottom, you can probably just use it directly there. :-)

::: browser/components/customizableui/test/browser_panelUINotifications_fullscreen_noAutoHideToolbar.js:28
(Diff revision 2)
> +  let notifications = [...PanelUI.notificationPanel.children].filter(n => !n.hidden);
> +  is(notifications.length, 1, "PanelUI doorhanger is only displaying one notification.");
> +  let doorhanger = notifications[0];
> +  is(doorhanger.id, "appMenu-update-manual-notification", "PanelUI is displaying the update-manual notification.");
> +
> +  EventUtils.synthesizeKey("VK_F11", {});

I am so surprised this works on OS X, as it uses a different shortcut and actually pressing F11 on my machine, at least, doesn't work. :-\

Anyway, it clearly does because it's in another test already, so carry on!

Comment 5

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8874042 [details]
Bug 1369899 - Listen to MozDOMFullscreen events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145490/diff/2-3/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1d5b019c52c
Listen to MozDOMFullscreen events r=Gijs

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/bec546987332 for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=104687853&repo=autoland

Comment 8

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8874042 [details]
Bug 1369899 - Listen to MozDOMFullscreen events

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145490/diff/3-4/

Comment 9

[Alex Thayer [:alexical] (she/her)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=54cf98880fc60206da4d07483a0df349a1ee7f48

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b92c5e65face
Listen to MozDOMFullscreen events r=Gijs

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b92c5e65face