Defect - Metro Firefox Settings, About, and Sync flyouts should close when Win8 Settings panel opens

VERIFIED FIXED in Firefox 24

Status

P3
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: bbondy, Assigned: ally)

Tracking

Trunk
Firefox 24
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=2)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
1. Go to the settings charm, open the options panel (or about, or sync).
2. Go to the settings charm, open the permissions flyout

You'll see 2 flyouts shown but only one should be shown.
I notice permissions hides itself as soon as the settings charm is expanded, we should do this for the other flyouts too.

p=2
Summary: Metro Firefox Settings, About, and Sync flyouts should close when Win8 Settings panel opens → Defect - Metro Firefox Settings, About, and Sync flyouts should close when Win8 Settings panel opens
Whiteboard: feature=defect c=tbd u=tbd p=0

Updated

6 years ago
Priority: -- → P3
(Assignee)

Comment 1

6 years ago
:bbondy, is there anything this bug needs before it is [shovel-ready] ?
(Reporter)

Comment 2

6 years ago
should be ready to go
Whiteboard: feature=defect c=tbd u=tbd p=0 → feature=defect c=tbd u=tbd p=0 [shovel-ready]
(Assignee)

Updated

6 years ago
Assignee: nobody → ally
Whiteboard: feature=defect c=tbd u=tbd p=0 [shovel-ready] → feature=defect c=tbd u=tbd p=2
Hi Ally, if you'll be taking this in Iteration #8 please add the Bug ID of the stories the defect relates to as a block.
Flags: needinfo?(ally)
(Assignee)

Comment 4

6 years ago
There is really a good one, so let's do the sync flyout & the about flyout since those appear to be the defective cases
Blocks: 831958, 831955
Flags: needinfo?(ally)
Blocks: 875024
No longer blocks: 859003
QA Contact: jbecerra
Whiteboard: feature=defect c=tbd u=tbd p=2 → feature=defect c=Settings_pane_options_and_about u=metro_firefox_user p=2
Status: NEW → ASSIGNED
(Assignee)

Comment 5

6 years ago
Created attachment 753528 [details] [diff] [review]
half fix at best

Hides all xul flyouts if a xul flyout is selected. This means the windows 8 settings flyout still sits over the old one until another fly out is selected. This is a half fix best (you still see the overlay of hte setting panel but not hte 2 flyouts over each other) and at worst no better (there's still two things causing visual clutter).

I think the answer lies elsewhere.
(Assignee)

Comment 7

6 years ago
(In reply to Brian R. Bondy [:bbondy] from comment #3)
> Actually I think a good place might be when the window is deactivated.  If I
> recall correctly, anytime a metro flyout is displayed our window will be
> deactivated.
> 
> http://dxr.mozilla.org/mozilla-central/widget/windows/winrt/FrameworkView.
> cpp#l444
> 
> I think there's already an event sent there.
There is an event, but I noticed halfway through today that "Content:Deactivate" doesn't fire when flyouts happen. Which is to say if that were true, than I expect to see 
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bindings/browser.js#707 fire, and I do not. This is the event I would expect the Cpp to be sending as the function is called "SendActivationEvent()"
 :( 

Maybe it's actually sending a different message. Let's find out.
> 
> This will also fix another bug of when you switch away from Metro the
> permissions flyout dismisses but our flyouts don't.
(Assignee)

Comment 8

6 years ago
I did some digging starting from SendActivationEvent(), called in FrameworkView::OnWindowActivated().

SendActivationEvent()
http://dxr.mozilla.org/mozilla-central/widget/windows/winrt/FrameworkView.cpp#l322
of which the most relevant line is probably the call to
http://dxr.mozilla.org/mozilla-central/widget/windows/winrt/MetroWidget.cpp#l1089
which tosses over to (a file that dxr could not find, but mxr can?)
http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsWebShellWindow.cpp#397
whose key line I think is
http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsWebShellWindow.cpp#405
which I think leads to this ball of wax
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#631
which to the best of my (very rusty) c++ does not raise any message, event, or observer notification. I think there is currently no way for the JS to be aware of this event, and that's the missing link.

(pretty please bbondy, someone, tell me I'm wrong! :( )

I think starting at OnWindowActivated() might be the right path. So, I think this bug is going to need two parts: A)backend, cpp to message B) frontend, js to actually control our flyouts.
Flags: needinfo?(netzen)
(Assignee)

Comment 9

6 years ago
TimA suggests that http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#762 might actually be the answer.
(Assignee)

Comment 10

6 years ago
looks like tim's suggestion is spot on. let's go with that
Flags: needinfo?(netzen)
(Assignee)

Comment 11

6 years ago
Created attachment 755118 [details] [diff] [review]
FlyoutPanelsUI listening for "deactivate" event
Attachment #753528 - Attachment is obsolete: true
Attachment #755118 - Flags: review?(netzen)
(Reporter)

Comment 12

6 years ago
Comment on attachment 755118 [details] [diff] [review]
FlyoutPanelsUI listening for "deactivate" event

Review of attachment 755118 [details] [diff] [review]:
-----------------------------------------------------------------

Yup seems like widget activated/deactivated leads to are activate/deactivate.
There's a non exhaustive list of events here: https://developer.mozilla.org/en-US/docs/XUL/Events

Looks good and this fixes a bunch of issues with the flyout being inconsistent with the permissions flyout.
Attachment #755118 - Flags: review?(netzen) → review+
(Assignee)

Comment 13

6 years ago
I'm all for killing multiple bugs with one stone.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd9e82e152d5
https://hg.mozilla.org/mozilla-central/rev/bd9e82e152d5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
For testing and verification.
Flags: needinfo?(virgil.dicu)
verified for it8 - re-opening the setting charms menu using a side swipe while our flyout is visible closes our flyout.
Mozilla/5.0 (Windows NT 6.2; rv:24.0) Gecko/20130612 Firefox/24.0

Verified that all flyouts (Options, Sync, About, Permissions) are closed when Setting are open.
Status: RESOLVED → VERIFIED
Flags: needinfo?(virgil.dicu)
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130816030205
Built from http://hg.mozilla.org/mozilla-central/rev/1ed5a88cd4d0

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0 and got expected result.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.