989947 - [Mac]Unable to close 'See what`s new' notification from first run experience (tour) while 'Default Browser' pop-up is up

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Bogdan Maris, Desktop Test Engineering]

Attachment: Screenshot

Reproducible on Firefox 29 Beta 3 (BuildID: 20140327140727):
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0		
Reproducible on the latest Aurora (BuildID: 20140331004001):
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0		
Reproducible on the latest Nightly (BuildID: 20140330030202): 
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0

Steps to reproduce:
1. Start Firefox with new profile.
2. Don`t close 'Default Browser' pop-up.
3. Try to close 'See what`s new' window.

Expected results: 'See what`s new' window is closed, like in Windows and Linux.

Actual results: If both 'Default Browser' and 'See what`s new' pop-ups are opened there is no way to close 'See what`s new' until 'Default Browser' is closed.

Notes:
1. This issue is not reproducible on Windows 7 and Ubuntu.

Comment 1

[Stefan [:stefanh]]

Hmm, I haven't tested, but I wonder if bug 957209 caused this

Comment 2

[Stefan [:stefanh]]

Aha, now when I look at the screenshot this doesn't seems related to bug 957209.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

I'm not able to reproduce this.  I tested with today's m-c nightly and a new profile on OS X 10.9.2 and 10.7.5.  The "See what's new" window didn't open.

Comment 4

[Timothy Nikkel (:tnikkel)]

You need to run an older version of Firefox that doesn't have australis with the same profile, and then run a newer version with australis to get that I think.

Comment 5

[Bogdan Maris, Desktop Test Engineering]

Or:
1) Open Nightly with new profile (Be sure to let the Default Browser pop-up run everytime you open Nightly - Hit NO on pop-up)
2) Mark as homepage https://www.mozilla.org/en-US/firefox/29.0/firstrun/
3) Make sure Nightly starts with Homepage (default)
3) Restart Nightly

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Thanks.  Now I can reproduce this.  I used Bogdan's STR.

Unfortunately this is by design.  The Default Browser dialog (a native sheet) is window-modal, which means that you can't do anything in the window (above which it's displayed) until you close the dialog (the sheet).

I think the best way around this is to prevent the "See what's new" dialog from being displayed if a modal dialog is open above the same window.  This may not be necessary on Windows and Linux, because modality works very differently on Windows and Linux.

Comment 7

[:Gijs (out for now; he/him)]

(In reply to Steven Michaud from comment #6)
> I think the best way around this is to prevent the "See what's new" dialog
> from being displayed if a modal dialog is open above the same window.

Can we check if this is the case from chrome JS in some way, and also get notified when the dialog goes away?

Comment 8

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #7)

I don't know this code at all (the chrome JS code that displays the "See what's new" dialog).  So I'm not the best one to look at it, though I can in default of anybody else.

Timothy, can you look at it, or suggest someone else to do that?

Comment 9

[Stefan [:stefanh]]

Fwiw, the default browser sheet is opened from here: http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#765

Comment 11

[Timothy Nikkel (:tnikkel)]

I also don't know the JS code for this at all. Maybe Gijs or Stefan? They sound like they know what they are talking about. :)

Comment 12

[:Gijs (out for now; he/him)]

(In reply to Timothy Nikkel (:tn) from comment #11)
> I also don't know the JS code for this at all. Maybe Gijs or Stefan? They
> sound like they know what they are talking about. :)

Sure, the problem is this kind of issue could happen for any kind of native modal dialog. There are various code paths for that (e.g. master password dialog, http auth dialog, onbeforeunload dialog, ...). I guess we'd need a way to tell if such a popup was open through the bit of native code that does the actual window creation.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

How about, for starters, stopping the "See what's new" dialog from appearing if there are *any* modal dialogs open?

Comment 14

[:Gijs (out for now; he/him)]

(In reply to Steven Michaud from comment #13)
> How about, for starters, stopping the "See what's new" dialog from appearing
> if there are *any* modal dialogs open?

Right, which is why I asked in comment 7 how we can tell if such a dialog is open...

Comment 15

[Steven Michaud [:smichaud] (Retired)]

I will look when I get the chance, which may not be for a few days.

Comment 16

[Timothy Nikkel (:tnikkel)]

Oh, I see what you are asking for now, maybe Neil knows if such a thing exists or where it would gets it's info from.

Comment 17

[Neil Deakin]

For this particular case, you want to move the code that opens the what's new dialog to just be after the call that opens the dialog. But looking at the code the dialog opens from an observer, so maybe that isn't an option.

In general though, you probably want to:
 - change nsXULPopupManager::MayShowPopup to return false when the popup's window returns true for nsGlobalWindow::IsInModalState.
 - make the what's new dialog attempt to reopen the dialog later if it didn't open before.

Comment 20

[Jet Villegas (inactive)]

Bumping this up as I just reproduced this on a brand new Mac. Pretty annoying and poor UX for a new user.

Comment 21

[Dan Callahan [:callahad]]

Attachment: developer-edition.png

Added screenshot. This bug also affects the Developer Edition.

Comment 22

[Alex Gibson [:agibson]]

+1 to get this fixed for the Dev Browser Edition, seen a few people Tweet about it.

Comment 23

[Alex Gibson [:agibson]]

CC'ing MattN

Comment 24

[Steven Michaud [:smichaud] (Retired)]

This *is* fixed in mozilla-central nightlies, as far as I know.

Comment 25

[Alex Gibson [:agibson]]

How quickly could we get this uplifted to dev-browser?

Comment 26

[:Gijs (out for now; he/him)]

(In reply to Steven Michaud from comment #24)
> This *is* fixed in mozilla-central nightlies, as far as I know.

Huh? How/where? We used a notification bar for some time, instead of a modal alert, which was a workaround - but we intentionally backed that out.

I'm not aware of anyone fixing the OS X / widget / XUL side of things.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

> This *is* fixed in mozilla-central nightlies, as far as I know.

Oops, I take that back.  It seems that m-c nightlies don't have this notification.

Comment 28

[Jennifer Bertsch [:jbertsch]]

Hi Joe and Jeff-

Can we get this fixed and uplifted asap?

Thx,
Jen

Comment 29

[Jeff Griffiths (:canuckistani) (:⚡︎)]

we're looking into it. As I understand it this is some kind of merge problem, as we were running on Gum we were seeing yellow notificationbox warnings that didn't block the UI.

Comment 30

[:Gijs (out for now; he/him)]

(In reply to Jeff Griffiths (:canuckistani) from comment #29)
> we're looking into it. As I understand it this is some kind of merge
> problem, as we were running on Gum we were seeing yellow notificationbox
> warnings that didn't block the UI.

No, that's because nobody merged aurora to gum when you were running on gum. We backed out the notification box from aurora, see bug 1086958 and related. We always used a modal alert, we just went back to that.

Comment 31

[Brian Grinstead [:bgrins]]

(In reply to :Gijs Kruitbosch from comment #30)
> (In reply to Jeff Griffiths (:canuckistani) from comment #29)
> > we're looking into it. As I understand it this is some kind of merge
> > problem, as we were running on Gum we were seeing yellow notificationbox
> > warnings that didn't block the UI.
> 
> No, that's because nobody merged aurora to gum when you were running on gum.
> We backed out the notification box from aurora, see bug 1086958 and related.
> We always used a modal alert, we just went back to that.

There was a merge at Fri Nov 7 09:37:59 2014 PST: https://tbpl.mozilla.org/?tree=Gum&rev=9a35e16bfa55.  Are there changesets after that that need to be merged?

Comment 32

[:Gijs (out for now; he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #31)
> (In reply to :Gijs Kruitbosch from comment #30)
> > (In reply to Jeff Griffiths (:canuckistani) from comment #29)
> > > we're looking into it. As I understand it this is some kind of merge
> > > problem, as we were running on Gum we were seeing yellow notificationbox
> > > warnings that didn't block the UI.
> > 
> > No, that's because nobody merged aurora to gum when you were running on gum.
> > We backed out the notification box from aurora, see bug 1086958 and related.
> > We always used a modal alert, we just went back to that.
> 
> There was a merge at Fri Nov 7 09:37:59 2014 PST:
> https://tbpl.mozilla.org/?tree=Gum&rev=9a35e16bfa55.  Are there changesets
> after that that need to be merged?

Landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/7f4cb8cbeaed

Landed on gum: http://hg.mozilla.org/projects/gum/rev/7f4cb8cbeaed

via

http://hg.mozilla.org/projects/gum/pushloghtml?changeset=7f4cb8cbeaed

which has nothing, which is very strange, and so I have no idea what's going on there.

In any case, my point was: this backout was intentional on aurora and we shouldn't back out the backout without due consideration.

I expect comment #17 is enough to fix this, and I'm just looking at whether I can make that happen right now - all I can say is we have too many *Window interfaces.

Comment 33

[Panos Astithas (he/him) [:past] (please ni?)]

Gijs is right, I had originally filed bug 1093538 before I realized that this change was made on purpose.

Comment 34

[:Gijs (out for now; he/him)]

Attachment: Part 1: disable popups during modal alerts on OS X,

This seems to be enough.

Comment 35

[:Gijs (out for now; he/him)]

Attachment: Part 2: tell UITour consumer that we didn't show the popup,

But we should send a notification to content, I guess... I imagine this would work? Untested because I don't know how.

Comment 36

[:Gijs (out for now; he/him)]

Marco, can you add this? Thanks!

Comment 37

[Alex Gibson [:agibson]]

Will this proposed fix still guarantee that the door-hangers still get shown when the modal is dismissed? Excuse my naivety, just making sure this will not break things on the web side.

Comment 38

[:Gijs (out for now; he/him)]

(In reply to Alex Gibson [:agibson] from comment #37)
> Will this proposed fix still guarantee that the door-hangers still get shown
> when the modal is dismissed? Excuse my naivety, just making sure this will
> not break things on the web side.

We have no way to be notified that the modal state goes away, as far as I know. The second patch adds a callback that you can opt in to which we'll call immediately to tell you opening the popup failed. The best I can think of is periodically re-calling showInfo (maybe a second later?) from that callback, with the same callback. That'll essentially keep trying until it works (at which point the "it didn't work" callback won't be called anymore). Does that make sense?

Comment 39

[Alex Gibson [:agibson]]

(In reply to :Gijs Kruitbosch from comment #38)
> We have no way to be notified that the modal state goes away, as far as I
> know. The second patch adds a callback that you can opt in to which we'll
> call immediately to tell you opening the popup failed. The best I can think
> of is periodically re-calling showInfo (maybe a second later?) from that
> callback, with the same callback. That'll essentially keep trying until it
> works (at which point the "it didn't work" callback won't be called
> anymore). Does that make sense?

I guess this could work OK, unless Matt has any better suggestions?

Is this due to land in Nightly first, and then uplifted to Dev Edition? I'll need to make updates on the web content side to both the standard /firstrun and dev-edition /firstrun to accommodate this.

Comment 40

[:Gijs (out for now; he/him)]

Attachment: disable popups during modal alerts on OS X,

I guess the alternative solution here is to just add an event we can listen for...

Comment 41

[:Gijs (out for now; he/him)]

Attachment: reshow popup when in modal state,

... and then listen for that.

Comment 42

[Marco Mucci [:MarcoM]]

Added to IT 36.2

Comment 43

[:Gijs (out for now; he/him)]

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a745845c5307

Comment 44

[Matthew N. [:MattN]]

Comment on attachment 8520150 [details] [diff] [review]
reshow popup when in modal state,

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

I think this is fine (assuming r=enn on the other part) but I think it should have a test. Looking at existing showInfo tests plus existing modal dialog tests shouldn't require too much new code.

Comment 45

[:Gijs (out for now; he/him)]

Attachment: test UITour interaction with modal popup,

I'd give you a try push for this but try is closed. Again.

Comment 47

[Matthew N. [:MattN]]

Comment on attachment 8520236 [details] [diff] [review]
test UITour interaction with modal popup,

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

Thanks for writing this

::: browser/modules/test/browser_UITour_modalDialog.js
@@ +88,5 @@
> +
> +    handleDialog = (doc) => {
> +      popup = document.getElementById("UITourTooltip");
> +      gContentAPI.showInfo("appMenu", "test title", "test text");
> +      panelShown =  promisePanelElementShown(window, popup);

Nit: extra space

@@ +89,5 @@
> +    handleDialog = (doc) => {
> +      popup = document.getElementById("UITourTooltip");
> +      gContentAPI.showInfo("appMenu", "test title", "test text");
> +      panelShown =  promisePanelElementShown(window, popup);
> +      is(popup.state, "closed", "Popup shouldn't be shown while dialog is up");

This assumes that openPopup is called synchronously as part of showInfo but with e10s this definitely won't be true. Obviously the timeouts below aren't ideal but I think this should be inside a delay too.

@@ +95,5 @@
> +      let dialog = doc.getElementById("commonDialog");
> +      dialog.acceptDialog();
> +    };
> +    startCallbackTimer();
> +    setTimeout(() => alert('hi'), 10);

Why not executeSoon? Nit: double quotes

@@ +98,5 @@
> +    startCallbackTimer();
> +    setTimeout(() => alert('hi'), 10);
> +    let waitPromise = new Promise((resolve, reject) => setTimeout(resolve, 100));
> +    yield waitPromise;
> +    yield panelShown;

before assuming panelShown was assigned to, add an ok(panelShown, …) above it otherwise it's possible for this test to complete without waiting for the dialog/info panel

Comment 48

[:Gijs (out for now; he/him)]

Comment on attachment 8520148 [details] [diff] [review]
disable popups during modal alerts on OS X,

Enn is on PTO - Neil or Boris, do you have time to review this? :-)

Comment 49

[:Gijs (out for now; he/him)]

Attachment: test UITour interaction with modal popup,

Like this?

Comment 50

[Matthew N. [:MattN]]

Comment on attachment 8520724 [details] [diff] [review]
test UITour interaction with modal popup,

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

Yep

::: browser/modules/test/head.js
@@ +7,5 @@
>  
>  const SINGLE_TRY_TIMEOUT = 100;
>  const NUMBER_OF_TRIES = 30;
>  
> +function waitForConditionPromise(condition, timeoutMsg, tryCount=NUMBER_OF_TRIES) {

Nit: put spaces around operators

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8520148 [details] [diff] [review]
disable popups during modal alerts on OS X,

Why are we adding an event fired at random web pages here?

Comment 52

[:Gijs (out for now; he/him)]

(In reply to Boris Zbarsky [:bz] from comment #51)
> Comment on attachment 8520148 [details] [diff] [review]
> disable popups during modal alerts on OS X,
> 
> Why are we adding an event fired at random web pages here?

Presumably because I messed up and don't understand how to have it fired just at the chrome document. Is that possible at all?

(as to why I want it in chrome, because AFAICT there's no other way to be notified when the window stops being in a modal state)

Comment 53

[Boris Zbarsky [:bzbarsky]]

You want nsContentUtils::DispatchChromeEvent.

Comment 54

[:Gijs (out for now; he/him)]

Comment on attachment 8520148 [details] [diff] [review]
disable popups during modal alerts on OS X,

Need to implement comment 53

Comment 55

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #54)
> Comment on attachment 8520148 [details] [diff] [review]
> disable popups during modal alerts on OS X,
> 
> Need to implement comment 53

I tried, but this:

    nsContentUtils::DispatchChromeEvent(mDoc, GetOuterWindow(), NS_LITERAL_STRING("endmodalstate"), true /* bubbles */, false /* not cancellable */);


doesn't seem to work. Boris, sorry for the dumb question but what am I doing wrong here? :-(

(I also tried s/GetOuterWindow()/ToSupports(this)/ but that was no better)

Comment 56

[Boris Zbarsky [:bzbarsky]]

When you say "doesn't seem to work", is the call returning an error, or is it returning NS_OK but your listener is not seeing the event?

Comment 58

[:Gijs (out for now; he/him)]

(In reply to Boris Zbarsky [:bz] from comment #56)
> When you say "doesn't seem to work", is the call returning an error, or is
> it returning NS_OK but your listener is not seeing the event?

Sorry, the latter (I've verified it returns NS_OK now). I've tried variations of:

window.addEventListener("endmodalstate", function(e) { Cu.reportError('ohi'); }, true);

window.addEventListener("endmodalstate", function(e) { Cu.reportError('ohi'); }, false);

etc., and I don't see any of them report anything in the console. Attaching the listener to the document or documentElement doesn't trigger anything either. Testing by just doing alert('hi') from the browser console, and clicking "OK" manually.

I can see if I can debug this further in xcode, but I'm bouncing the needinfo just to see if you have immediate ideas as to what's going wrong...

Comment 59

[:Gijs (out for now; he/him)]

Attachment: disable popups during modal alerts on OS X,

This seems to work. Code courtesy of smaug and ipc/TabParent.cpp

Comment 60

[Boris Zbarsky [:bzbarsky]]

Ah, DispatchChromeEvent fires on the chrome event handler for the window, not the window itself.

Comment 61

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8521497 [details] [diff] [review]
disable popups during modal alerts on OS X,

r=me

Comment 62

[:Gijs (out for now; he/him)]

Wanted to try-push this before landing because the previous trypush was so exceedingly orange...

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0929f0063f5c

Comment 63

[:Gijs (out for now; he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/62df790c6e2e
remote:   https://hg.mozilla.org/integration/fx-team/rev/96b98f7a64d9
remote:   https://hg.mozilla.org/integration/fx-team/rev/413573925059

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62df790c6e2e
https://hg.mozilla.org/mozilla-central/rev/96b98f7a64d9
https://hg.mozilla.org/mozilla-central/rev/413573925059

Comment 65

[:Gijs (out for now; he/him)]

Comment on attachment 8521497 [details] [diff] [review]
disable popups during modal alerts on OS X,

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: dev edition users get default browser prompt and UItour popups at the same time and have a hard time closing them / getting a functional browser

NB: this is essentially involved in ship-blocking stuff for devedition vs. beta9 things because beta9 promotes devedition and then that has this bug (which, as per dups, is pretty annoying)

[Describe test coverage new/current, TBPL]: has automated test coverage
[Risks and why]: low, mac-only change to whether popups are allowed to open while the window is in a modal state, and an event that broadcasts when a window loses modal state. Not aware of anything that could break because of this change.
[String/UUID change made/needed]: no

Comment 66

[:Gijs (out for now; he/him)]

To be clear, I'd like approval on all 3 patches, but the approval request would essentially be the same, so I've filed just the one.

Comment 67

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8521497 [details] [diff] [review]
disable popups during modal alerts on OS X,

Sounds good, thanks Gijs

Comment 68

[:Gavin Sharp [email: [e-mail]]]

Pushed the functional patches to Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/42ed43162f49
https://hg.mozilla.org/releases/mozilla-aurora/rev/65e5b79b374a

And a followup bustage fix:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ccf0f34d9390

I didn't land the test patch (attachment 8520724 [details] [diff] [review]) since it had a slight conflict (bug 1073238 didn't land on Aurora). Hoping Gijs can followup there when he's next available.

Comment 69

[Bogdan Maris, Desktop Test Engineering]

Using latest Developer Edition build I am not able to reproduce the initial issue so this is verified Fixed. I checked using either the Australis first run or Dev Edition first run experience on Mac OS X 10.9.5 and 10.8.5

Comment 70

[Bogdan Maris, Desktop Test Engineering]

After a better look I found a possible issue.

If I open Aurora with new profile and I close the Default browser message before the firstrun page loads, the blue circle indicator will not be displayed on Developer Tools icon. If the firstrun page loads, after closing Default browser message the blue indicator will show up. 

Is this something to do with this bug or should I log a new one?

Comment 71

[Bogdan Maris, Desktop Test Engineering]

Sorry for the spam but the behavior is actually the other way around:

Closing the message after loading the page will not display the blue indicator.
Closing the message before loading the page will display the blue indicator.

Comment 72

[:Gijs (out for now; he/him)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #71)
> Sorry for the spam but the behavior is actually the other way around:
> 
> Closing the message after loading the page will not display the blue
> indicator.
> Closing the message before loading the page will display the blue indicator.

Makes more sense that way around, too. Please file a followup bug and needinfo me there. Thank you!

Comment 73

[:Gijs (out for now; he/him)]

Try push for a fixed up version of the test against current m-aurora tip:

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=687d3f235013

I'll land this assuming that comes back green (on everything that's enabled on aurora)

Comment 74

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #73)
> Try push for a fixed up version of the test against current m-aurora tip:
> 
> remote:  
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=687d3f235013
> 
> I'll land this assuming that comes back green (on everything that's enabled
> on aurora)

Grmbl.

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e10b23b7fbcc

Comment 75

[:Gijs (out for now; he/him)]

Third time lucky, I hope:

remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cbfc68cc6c3f

Comment 76

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #75)
> Third time lucky, I hope:
> 
> remote:  
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cbfc68cc6c3f

This failed, too. Unfortunately, I don't actually understand why. The test passes for me locally. Will try some more tomorrow. :-\

Comment 77

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #76)
> (In reply to :Gijs Kruitbosch from comment #75)
> > Third time lucky, I hope:
> > 
> > remote:  
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cbfc68cc6c3f
> 
> This failed, too. Unfortunately, I don't actually understand why. The test
> passes for me locally. Will try some more tomorrow. :-\

Egh. Sometimes, the clear light of morning is everything. This test was only meant to work on OS X in the first place, and I committed it as such on m-c, but never updated the patch here. :-\

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/8651ad90ff2f

Comment 78

[Bogdan Maris, Desktop Test Engineering]

Using latest Nightly build on Mac OS X 10.9.5 I am not able to reproduce the initial issue so marking this as verified fixed on 36 branch.