Closed Bug 1369899 Opened 7 years ago Closed 7 years ago

Listen for MozDOMFullscreenEvents for update doorhangers on OSX


(Toolkit :: Application Update, enhancement)

Not set



Tracking Status
firefox55 --- fixed


(Reporter: alexical, Assigned: alexical)




(1 file)

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 on attachment 8874042 [details]
Bug 1369899 - Listen to MozDOMFullscreen events

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. :-)
Attachment #8874042 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8874042 [details]
Bug 1369899 - Listen to MozDOMFullscreen events

::: 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(, "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!
Attachment #8874042 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by
Listen to MozDOMFullscreen events r=Gijs
Pushed by
Listen to MozDOMFullscreen events r=Gijs
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.