Listen for MozDOMFullscreenEvents for update doorhangers on OSX

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: dthayer, Assigned: dthayer)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
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. :-)
Attachment #8874042 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
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!
Attachment #8874042 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)

Comment 6

a year ago
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c1d5b019c52c
Listen to MozDOMFullscreen events r=Gijs
Comment hidden (mozreview-request)

Comment 10

a year ago
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b92c5e65face
Listen to MozDOMFullscreen events r=Gijs

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b92c5e65face
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.