Closed Bug 1348396 Opened 7 years ago Closed 7 years ago

buttons on app update doorhanger can't be clicked when in fullscreen and mouse is placed at top of screen to display the doorhanger.

Categories

(Toolkit :: Application Update, defect)

Product:
Toolkit
Component:
Application Update
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: robert.strong.bugs, Assigned: alexical)

References

Details

Attachments

(1 file)

Bug 1348396 - Only show a badge on PanelUI while in fullscreen
59 bytes, text/x-review-board-request
enndeakin
: review+
robert.strong.bugs
: review+
Details 
With the new application update ui being created in bug 893505 it is possible to see the new doorhanger when placing the mouse at the top of the screen to temporarily exit fullscreen but it isn't possible to click any of the buttons on the doorhanger due to going back into fullscreen before getting to the buttons.

If this is difficult perhaps just show the update badge instead of the doorhanger for this case.
Bram, I'd like your input on this.
Flags: needinfo?(bram)
In bug 893505 comment 139, I suggested that the update doorhanger should block the toolbar from minimising unless it’s been interacted with.

It turned out that the Control Center doorhanger also behaves the same way and blocks the toolbar:
https://d3uepj124s5rcx.cloudfront.net/items/2H0l1i0u0a3Z272W1U2q/control-center-doorhanger-fullscreen.gif

Can we copy this behaviour? If not, then we can just show the update badge on fullscreen, and the doorhanger when fullscreen is exited.
Flags: needinfo?(bram)
It was done that way by Doug previously but it is important to note that the Control Center doorhanger requires user interaction by the client to open it prior to it blocking whereas this would just block without the client performing an action to intentionally open it. With that in mind and as you pointed out it isn't possible to start full screen so it would be shown on startup it seems that just showing the badge would be less annoying to the client.
Good point. The Control Center is triggered manually, and but is automatic, and that might feel odd. Why is a doorhanger that I haven’t asked for blocked my full screen experience?

If we are to increase the rate of conversion (users who click “Update” instead of “Not Now”), it would be best to only show the doorhanger when it doesn’t impede a fullscreen video. Therefore, in this situation, let’s just show the badge.

Last question: when you quit fullscreen and haven’t interacted with the doorhanger, can we trigger it to open automatically, or would that be too interruptive?
(In reply to Bram Pitoyo [:bram] from comment #4)
> Good point. The Control Center is triggered manually, and but is automatic,
> and that might feel odd. Why is a doorhanger that I haven’t asked for
> blocked my full screen experience?
> 
> If we are to increase the rate of conversion (users who click “Update”
> instead of “Not Now”), it would be best to only show the doorhanger when it
> doesn’t impede a fullscreen video. Therefore, in this situation, let’s just
> show the badge.
Sounds good.

> 
> Last question: when you quit fullscreen and haven’t interacted with the
> doorhanger, can we trigger it to open automatically, or would that be too
> interruptive?
I suspect it will be possible but I haven't looked at the code yet and I bet that dthayer would be able to answer than better than I could.
Flags: needinfo?(dothayer)
Yup! If I'm hearing you right, then it should be doing that already. Let me know if you're seeing something to the contrary though.
Flags: needinfo?(dothayer)
dthayer, see comment #0 for what I am seeing
Flags: needinfo?(dothayer)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #7)
> dthayer, see comment #0 for what I am seeing

Right, I was referring to your comment #5:

> > Last question: when you quit fullscreen and haven’t interacted with the
> > doorhanger, can we trigger it to open automatically, or would that be too
> > interruptive?
> I suspect it will be possible but I haven't looked at the code yet and I bet
> that dthayer would be able to answer than better than I could.

It should already open automatically when quitting fullscreen. Regarding showing a badge instead of the doorhanger that should also be trivial.
Flags: needinfo?(dothayer)
(In reply to Doug Thayer [:dthayer] from comment #8)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #7)
> > dthayer, see comment #0 for what I am seeing
> 
> Right, I was referring to your comment #5:
> 
> > > Last question: when you quit fullscreen and haven’t interacted with the
> > > doorhanger, can we trigger it to open automatically, or would that be too
> > > interruptive?
> > I suspect it will be possible but I haven't looked at the code yet and I bet
> > that dthayer would be able to answer than better than I could.
> 
> It should already open automatically when quitting fullscreen. Regarding
> showing a badge instead of the doorhanger that should also be trivial.
Just for clarity. When in fullscreen the badge would be shown and when exiting fullscreen the doorhanger would be shown instead even though the badge was shown while in fullscreen.
Attachment #8856611 - Flags: review?(enndeakin)
How does this relate to 1353980 where notifications were wanted when in fullscreen mode?
+bram for comment, but I think the decision in Bug 1353980 doesn't apply, since that is for a notification which is directly relevant to the page which is in fullscreen mode. Our notifications are 1) not the direct result of user action, and 2) not related to the content that is currently being viewed. Accordingly we don't want them to be intrusive. Additionally I think this reasoning is likely to apply to any future panelUI notifications, since if they are attached to the panelUI, it's likely they're more to do with the browser itself rather than the content on display.
Flags: needinfo?(bram)
Assignee: nobody → dothayer
(In reply to Neil Deakin from comment #11)
> How does this relate to 1353980 where notifications were wanted when in
> fullscreen mode?

Doug is absolutely right. I can’t say any more helpful reason than what he had written.

I’d just like to add that, with these kinds of notifications, we want to still show it but be minimally interruptive. If you put a page on full-screen, we assume that you mostly likely want to focus on the content. Our doorhanger won’t ever steal your focus. We’ll wait to show up until you’ve exited the mode.
Flags: needinfo?(bram)
Comment on attachment 8856611 [details]
Bug 1348396 - Only show a badge on PanelUI while in fullscreen

https://reviewboard.mozilla.org/r/128566/#review131388
Attachment #8856611 - Flags: review?(enndeakin) → review+
Thanks for the review, Neil! Would you mind autolanding this for me?
Flags: needinfo?(enndeakin)
Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6d5806e5695
Only show a badge on PanelUI while in fullscreen r=enndeakin+6102
Flags: needinfo?(enndeakin)
Backed out for frequently failing modified test:

https://hg.mozilla.org/integration/autoland/rev/474af558075cf7a8d47b6b9001f65bf39746bd7c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d6d5806e56958014654e0a7c5ab687b1832937c8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=90562933&repo=autoland

11:17:44     INFO - Entering test bound testFullscreen
11:17:44     INFO - TEST-PASS | browser/components/customizableui/test/browser_panelUINotifications.js | update-manual doorhanger is closed. - 
11:17:44     INFO - TEST-PASS | browser/components/customizableui/test/browser_panelUINotifications.js | update-manual doorhanger is showing. - 
11:17:44     INFO - TEST-PASS | browser/components/customizableui/test/browser_panelUINotifications.js | PanelUI doorhanger is only displaying one notification. - 
11:17:44     INFO - TEST-PASS | browser/components/customizableui/test/browser_panelUINotifications.js | PanelUI is displaying the update-manual notification. - 
11:17:44     INFO - Buffered messages finished
11:17:44     INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_panelUINotifications.js | Test timed out -
Flags: needinfo?(dothayer)
Neil would you mind taking another look / landing it if it looks good?

Have a try build here:

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

(The failures above were on PGO builds, so I don't know how much weight that try build carries. I can MOZ_PGO=1 hack for a PGO try if you think that's warranted.)
Flags: needinfo?(dothayer) → needinfo?(enndeakin)
Comment on attachment 8856611 [details]
Bug 1348396 - Only show a badge on PanelUI while in fullscreen

https://reviewboard.mozilla.org/r/128566/#review131698
Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a9fb56b87d01
Only show a badge on PanelUI while in fullscreen r=enndeakin+6102
sorry had to back this out for frequent customizableui browser_panelUINotifications.js failures like https://treeherder.mozilla.org/logviewer.html#?job_id=90827273&repo=autoland&lineNumber=13857

https://hg.mozilla.org/integration/autoland/rev/bc086e9044e6
Flags: needinfo?(dothayer)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64d967b1c9e09b6d4a988fe1169e5ce2a9588514

Re-ran bc2 (where the browser_panelUINotifications.js tests were) several times on Linux debug. Seems stable.
Flags: needinfo?(dothayer)
On Mac os, in full screen mode (which seems different from other OSes) door hanger ui appears and both buttons are clickable. See video in bug 1357917 for reference.
Comment on attachment 8856611 [details]
Bug 1348396 - Only show a badge on PanelUI while in fullscreen

https://reviewboard.mozilla.org/r/128566/#review135480

r+ from me for the test changes.
Attachment #8856611 - Flags: review+
Comment on attachment 8856611 [details]
Bug 1348396 - Only show a badge on PanelUI while in fullscreen

https://reviewboard.mozilla.org/r/128566/#review135482
Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a2771a2c77ab
Only show a badge on PanelUI while in fullscreen r=enndeakin+6102,rstrong
Flags: needinfo?(enndeakin)
https://hg.mozilla.org/mozilla-central/rev/a2771a2c77ab
Status: NEW → RESOLVED
Closed: 7 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: