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

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Shell, defect, P3)

Description

[Brian R. Bondy [:bbondy]]

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

Comment 1

[Allison Naaktgeboren :ally]

:bbondy, is there anything this bug needs before it is [shovel-ready] ?

Comment 2

[Brian R. Bondy [:bbondy]]

should be ready to go

Comment 3

[Marco Mucci [:MarcoM]]

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.

Comment 4

[Allison Naaktgeboren :ally]

There is really a good one, so let's do the sync flyout & the about flyout since those appear to be the defective cases

Comment 5

[Allison Naaktgeboren :ally]

Attachment: 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.

Comment 6

[Allison Naaktgeboren :ally]

I commented the followup in the wrong bug >.<
https://bugzilla.mozilla.org/show_bug.cgi?id=867495#c1
https://bugzilla.mozilla.org/show_bug.cgi?id=867495#c2
https://bugzilla.mozilla.org/show_bug.cgi?id=867495#c3

Comment 7

[Allison Naaktgeboren :ally]

(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.

Comment 8

[Allison Naaktgeboren :ally]

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.

Comment 9

[Allison Naaktgeboren :ally]

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

Comment 10

[Allison Naaktgeboren :ally]

looks like tim's suggestion is spot on. let's go with that

Comment 11

[Allison Naaktgeboren :ally]

Attachment: FlyoutPanelsUI listening for "deactivate" event

Comment 12

[Brian R. Bondy [:bbondy]]

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.

Comment 13

[Allison Naaktgeboren :ally]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd9e82e152d5

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Marco Mucci [:MarcoM]]

For testing and verification.

Comment 16

[Jim Mathies [:jimm]]

verified for it8 - re-opening the setting charms menu using a side swipe while our flyout is visible closes our flyout.

Comment 17

[Virgil Dicu [:virgil] [QA]]

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.

Comment 18

[Samvedana (:Samvedana)]

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.