brycer22 Reporter
	Description • 18 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

[Suggestion]

Instead of a javascript alert() box changing the active tab, the tab could change colour or flash. it is annoying reading something and have the tab suddenly change

Reproducible: Always

Steps to Reproduce:
1. Have more than one tab open
2. Have one tab alert() you.

	Jesse Ruderman
	Comment 2 • 18 years ago

Fixing bug 123913 is one way to fix this.

	Daniel.S
	Comment 4 • 16 years ago

(In reply to comment #3)
> This bug doesn't belong in Firefox/General, but where does it? I hesitate
> between "Firefox/Tabbed Browser" and "Core/Javascript Engine".

The former for now. The request is about a visual indication. Of course, I wouldn't mind if this was marked as a dupe, since a discussion about the visual indication would most likely be held anyway when fixing bug 123913.

	Glenn Maynard
	Comment 5 • 13 years ago

Reviving this bug:

Tab-modal prompts are now implemented, but calling alert() still steals focus from other tabs.  As the OP described, this should use a visual indicator: flash the tab.  If the tab's window isn't focused, use the windowing system's window alert mechanism (eg. flash the Windows taskbar).

Web page should never be allowed to steal focus.

	Glenn Maynard
	Comment 6 • 13 years ago

Opera does this, by the way.

	David Balažic
	Comment 7 • 12 years ago

Still an issues with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0

	Tim Abraldes [:TimAbraldes] [:tabraldes]
	Comment 9 • 11 years ago

From what I recall when digging through our tab-modal dialog code, we explicitly switch to any tab when it shows a tab-modal dialog. I kind of agree that that's not really necessary behavior and I'd prefer the behavior to be that we don't switch to tabs, regardless of what they're doing prompt-wise.

	Tim Abraldes [:TimAbraldes] [:tabraldes]
	Comment 10 • 11 years ago

We'll need to remove line 3173 here [1] and the comment here [2]. I'm not sure if that will be enough to correctly modify the behavior: There might be places where, during tab-modal dialog creation, we're assuming that the tab for which we're creating a dialog is the currently selected tab.

[1] https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml?rev=754cf7fc84cd#3166
[2] https://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/nsPrompter.js?rev=72bc1aebb5d0#173

	Tim Abraldes [:TimAbraldes] [:tabraldes]
	Comment 11 • 11 years ago

Oops, sorry! We can't remove the lines mentioned above because those are used in the modal dialog case as well. More likely we'll want to remove lines 398 and 418 here [1]. Then we'll have to make sure that there are no unforeseen consequences from not firing the "DOMWillOpenModalDialog" event.

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/nsPrompter.js?rev=72bc1aebb5d0#397

	breck7
	Comment 12 • 10 years ago

I agree that this can be really annoying.

I could also see it being useful for some situations.

What if a second parameter was added to the confirm and alert methods:

```
alert(message: string, stealFocus: boolean);
confirm(message: string, stealFocus: boolean);
```

	Glenn Maynard
	Comment 13 • 10 years ago

It doesn't matter how useful an author might think it is, it's never okay to steal focus.

	:Gijs (he/him) Assignee
	Comment 23 • 9 years ago

I think this is an annoying papercut, and I would like to (get someone to) fix this, but we need some UI to indicate which tab is throwing up the alert/message so that you don't miss the ones where the user does agree with the web dev that the alert is important (in my case, to give an example, I sometimes see websites that pop up alerts saying they will log me out in however many seconds because of "inactivity" and I don't want them to do this).

Philipp, I was thinking we could change the tab's font to be bold and add an icon (or replace the favicon). Does that sound sensible and/or do you have other/better ideas?

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo
	Comment 24 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #23)
> Philipp, I was thinking we could change the tab's font to be bold and add an
> icon (or replace the favicon). Does that sound sensible and/or do you have
> other/better ideas?

Yeah, makes sense! We can use the glow we currently have for pinned tabs here.

	Jerry Baker
	Comment 25 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #23)
> Philipp, I was thinking we could change the tab's font to be bold and add an
> icon (or replace the favicon). Does that sound sensible and/or do you have
> other/better ideas?

In Windows, when an out of focus window needs attention the taskbar icon flashes. For Windows users at least, the precedent of flashing to indicate attention is needed has been established. Is it possible to stick with the "meme" as it were in Windows and flash the tab?

	:Gijs (he/him) Assignee
	Comment 26 • 9 years ago

(In reply to Jerry Baker from comment #25)
> (In reply to :Gijs Kruitbosch from comment #23)
> > Philipp, I was thinking we could change the tab's font to be bold and add an
> > icon (or replace the favicon). Does that sound sensible and/or do you have
> > other/better ideas?
> 
> In Windows, when an out of focus window needs attention the taskbar icon
> flashes. For Windows users at least, the precedent of flashing to indicate
> attention is needed has been established. Is it possible to stick with the
> "meme" as it were in Windows and flash the tab?

This is difficult to do because we don't know the color of the tab, because we don't know the color of the tabbar in the (most common) scenario where tabs are in the titlebar, on Windows 8.1 and below (and on high contrast mode in win10). Having a contrasting color to draw attention (and ensuring that the text on the tab and any icons, like the tab muting icon, on it would remain visible in these cases) would be a lot more complex. It also has accessibility issues (for people who are colorblind or have epilepsy, for instance). So it will require a lot more engineering in order to do well than the suggestion I made. Finally, while it might be platform-conformant on Windows, on OS X things "jump" in the dock (they literally move up and down) and on Linux things depend on the window manager in use. We don't have the engineering capacity to work on platform conformant solutions for all these platforms (that also have to look good and not crazy - I don't fancy the prospect of tabs jumping up and down...).

That said, you could probably do something to accomplish this (esp. for the narrow case of your own machine) with a userstyle/addon after we implement this.

	Justin Dolske [:Dolske]
	Comment 29 • 9 years ago

(In reply to Gijs Kruitbosch (PTO until Sep 6) from comment #23)
> I think this is an annoying papercut, and I would like to (get someone to)
> fix this, but we need some UI to indicate which tab is throwing up the
> alert/message so that you don't miss the ones where the user does agree with
> the web dev that the alert is important (in my case, to give an example, I
> sometimes see websites that pop up alerts saying they will log me out in
> however many seconds because of "inactivity" and I don't want them to do
> this).

Couple of thoughts:

Is the list of sites small enough that we could just have a pref/permission for allowing their prompts to steal focus? Maybe this a case of perfect-bs-good-enough, and we can essentially fix this by just providing some manual opt-in for rare cases where a user really wants it. Google Calendar reminders being the canonical example, and the one(?) widespread enough that we'd probably need to whitelist it OOTB.

Prompts triggered from a background tab could have a "[ ] Allow this site to focus the tab in the background" checkbox... Although that feels like it's too prominent for a rare need.

Alternatively, what if we had prompts in background windows spawn a notification instead of stealing focus? (EG: "The background tab https://site.com says: Blah blah blah") Clicking it would focus the tab. That would avoid the focus stealing, but preserve the notification-like ability of the prompt.

Notifications might need to deal with potential abuse by background tabs, but (1) it's already an abuse vector with focus stealing and (2) I think SimplePush is already working on such UI.

	Lucius Chiaraviglio
	Comment 30 • 9 years ago

As a user, I vote for the last option = notify that a background tab has an alert.  For long alert text/large dialog box, you might need to abbreviate the alert text (the user can see the rest of it when they go to the tab).

	Alex Red
	Comment 31 • 9 years ago

Its annoying when you open a link in a new tab and it "steals" your focus from the current tab to the new one. If you want examples I can give you some.
This BUG doesnt bother users who jump to the newly opened tab automatically, the just click "ok" on the popup message box.

This bug has been flagged as NEEDINFO, what information exactly do you need ?

	:Gijs (he/him) Assignee
	Comment 32 • 9 years ago

(In reply to Alex Red from comment #31)
> This bug has been flagged as NEEDINFO, what information exactly do you need ?

It's flagged so I remember to get back to it. The number of bugs I get email about and touch is sufficiently large (one week absence: 1000+ emails, 300+ conversations) to necessitate some manner of doing so.

	:Gijs (he/him) Assignee
	Comment 33 • 9 years ago

Bug 332195 - part 1: don't focus tabs by default, add pref to be able to revert late if we see too many issues, r?mconley

	:Gijs (he/him) Assignee
	Comment 34 • 9 years ago

Bug 332195 - part 2: allow the user to control this behaviour with a checkbox, r?mconley

	:Gijs (he/him) Assignee
	Comment 35 • 9 years ago

(In reply to Justin Dolske [:Dolske] from comment #29)
> (In reply to Gijs Kruitbosch (PTO until Sep 6) from comment #23)
> > I think this is an annoying papercut, and I would like to (get someone to)
> > fix this, but we need some UI to indicate which tab is throwing up the
> > alert/message so that you don't miss the ones where the user does agree with
> > the web dev that the alert is important (in my case, to give an example, I
> > sometimes see websites that pop up alerts saying they will log me out in
> > however many seconds because of "inactivity" and I don't want them to do
> > this).
> 
> Couple of thoughts:
> 
> Is the list of sites small enough that we could just have a pref/permission
> for allowing their prompts to steal focus? Maybe this a case of
> perfect-bs-good-enough, and we can essentially fix this by just providing
> some manual opt-in for rare cases where a user really wants it. Google
> Calendar reminders being the canonical example, and the one(?) widespread
> enough that we'd probably need to whitelist it OOTB.

I don't know Google Calendar's case (don't use their reminders), but I added a way for the user to permit individual sites to steal focus for the v1 because it seems likely that everyone / many people has/have 1 (or maybe a few) sites where they want to keep current behaviour.


> Prompts triggered from a background tab could have a "[ ] Allow this site to
> focus the tab in the background" checkbox... Although that feels like it's
> too prominent for a rare need.

I don't think that many pages will hit this... so this is what I went with - though another point would be that we don't currently have some kind of settings icon with a dropdown or whatever to make it less prominent (making it another button just for this seemed even more prominent, and will run into issues with confirmEx). I figured that for the perfect vs. good enough case, I'd start with the checkbox. If UX wants to improve this, we can do that in a followup bug.

> Alternatively, what if we had prompts in background windows spawn a
> notification instead of stealing focus? (EG: "The background tab
> https://site.com says: Blah blah blah") Clicking it would focus the tab.
> That would avoid the focus stealing, but preserve the notification-like
> ability of the prompt.

I considered this, but currently before websites can use these they need to request user permission. It seemed weird to then give them a way to "circumvent" that by using alert() - definitely not an API we really love and want to encourage anyway - so I left it out for v1. We could do a followup about this, but because it would need to cope with a number of different things (spam-protection so multiple alerts per domain don't throw up more notifications; a way for the user to turn off that behaviour either globally / per-site, etc.) I left it out of v1. It would be relatively easy to do a minimalist add-on for this if people felt so inclined.

	:Gijs (he/him) Assignee
	Comment 36 • 9 years ago

Regarding the implementation: the insertion of the extra checkbox row is a little hacky... but I didn't want to break the "stop this page showing more dialogs" thing, which is actually run from within nsGlobalWindow.cpp, and so I worked around it rather than trying to come to some conclusion about which of the checkboxes is more important or whatever.

Another implementation decision I made that might be a bit controversial is the decision process for the checkbox living in tabbrowser's DOMWindowWillOpen listener, from which it was a pain to interface with the tabprompt object which we just keep creating per browser. I tried to work out how to make them persist, but that looks scary - there's a closure, a number of explicit references to self/browser, and so I think the only way to fix that would be to make the promptbox a property on the browser and to be methodical about erasing it again when the browser goes away, but I couldn't find a place to do the latter, so I gave up on that and hacked around it instead. Shudder.

Finally, none of this is going to apply to showModalDialog, for one because we want it to go away, for another because AIUI it currently doesn't use the tabmodal prompting so not switching tabs would be confusing - you're getting a popup window in your face and you should be able to see which tab caused it (and you can't interact with it anyway) otherwise there'd be opportunities for phishing...

Right, I think that's most of it covered, though there's one more thing that I noticed in the existing code...

	:Gijs (he/him) Assignee
	Comment 37 • 9 years ago

https://reviewboard.mozilla.org/r/21833/#review19593

::: toolkit/components/prompts/src/nsPrompter.js:489
(Diff revision 1)
>      args.showAlertOrigin = topPrincipal.equals(promptPrincipal);

So careful reading of this line should trigger some alarm bells. Surely we want to show this only for the opposite case?

Fret not, because despite the misleading name, that is apparently what it does... https://dxr.mozilla.org/mozilla-central/rev/11ff0ccb7d59311df4c190d331c8b58c6e35a0c8/toolkit/components/prompts/content/tabprompts.xml#170

but yeah. Confusing. :-\

	:Gijs (he/him) Assignee
	Comment 38 • 9 years ago

Comment on attachment 8673013 [details]
MozReview Request: Bug 332195 - part 2: allow the user to control this behaviour with a checkbox, r?mconley

Based on vidyo and IRC feedback, maybe we really want something else instead of a(n extra) checkbox.

Philipp, can you play with the UI here and if necessary suggest adjustments? There's a try build being created here:

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

	Mike Conley (:mconley) (:⚙️)
	Comment 39 • 9 years ago

Comment on attachment 8673012 [details]
MozReview Request: Bug 332195 - part 1: don't focus tabs by default, add pref to be able to revert late if we see too many issues, r?mconley

https://reviewboard.mozilla.org/r/21831/#review19721

::: browser/base/content/tabbrowser.xml:4368
(Diff revision 1)
> -          // browser, but that's in the originalTarget.
> -          // XXX Why originalTarget for the browser?
> +          // browser, but that's in the originalTarget (not the target, because
> +          // it's across a XBL boundary (that of the tabbrowser))

Nested parentheses! :D

Let's try this:

but that's in the originalTarget and not the target (since it's across the tabbrowser's XBL boundary).

::: browser/base/content/tabbrowser.xml:5877
(Diff revision 1)
> -        <xul:hbox xbl:inherits="pinned,selected,visuallyselected,titlechanged,fadein"
> +        <xul:hbox xbl:inherits="pinned,selected,visuallyselected,fadein"
>                    class="tab-background">
> -          <xul:hbox xbl:inherits="pinned,selected,visuallyselected,titlechanged"
> +          <xul:hbox xbl:inherits="pinned,selected,visuallyselected"
>                      class="tab-background-start"/>
> -          <xul:hbox xbl:inherits="pinned,selected,visuallyselected,titlechanged"
> +          <xul:hbox xbl:inherits="pinned,selected,visuallyselected"
>                      class="tab-background-middle"/>
> -          <xul:hbox xbl:inherits="pinned,selected,visuallyselected,titlechanged"
> +          <xul:hbox xbl:inherits="pinned,selected,visuallyselected"

I assume you've audited the tab-background and tab-background-[start|middle|end] styles to ensure that titlechanged attribute isn't searched for?

Maybe we'll need an add-on compat flag here too, since there might be some themes or add-ons relying on that attribute.

::: browser/themes/shared/devedition.inc.css:286
(Diff revision 1)
> -.tabbrowser-tab[pinned][titlechanged]:not([visuallyselected="true"]) > .tab-stack > .tab-content {
> +.tabbrowser-tab[image] > .tab-stack > .tab-content[attention]:not([visuallyselected="true"]),
> +.tabbrowser-tab > .tab-stack > .tab-content[pinned][titlechanged]:not([visuallyselected="true"]) {
>    background-image: var(--pinned-tab-glow);
>    background-position: center;
>    background-size: 100%;
>  }
>  
> +.tabbrowser-tab[image] > .tab-stack > .tab-content[attention]:not([pinned]):not([visuallyselected="true"]) {
> +  background-position: left bottom var(--tab-toolbar-navbar-overlap);
> +  background-size: 34px 100%;
> +}
> +

Would you mind posting some screenshots with what this looks like?

::: browser/themes/shared/tabs.inc.css:447
(Diff revision 1)
> -.tabbrowser-tab[pinned][titlechanged]:not([visuallyselected="true"]) > .tab-stack > .tab-content {
> +.tabbrowser-tab[image] > .tab-stack > .tab-content[attention]:not([visuallyselected="true"]),
> +.tabbrowser-tab > .tab-stack > .tab-content[pinned][titlechanged]:not([visuallyselected="true"]) {
>    background-image: radial-gradient(farthest-corner at center bottom, rgb(255,255,255) 3%, rgba(186,221,251,0.75) 20%, rgba(127,179,255,0.25) 40%, transparent 70%);
>    background-position: center bottom var(--tab-toolbar-navbar-overlap);
>    background-repeat: no-repeat;
>    background-size: 85% 100%;
>  }
>  
> +.tabbrowser-tab[image] > .tab-stack > .tab-content[attention]:not([pinned]):not([visuallyselected="true"]) {
> +  background-position: left bottom var(--tab-toolbar-navbar-overlap);
> +  background-size: 34px 100%;
> +}
> +
> +.tab-label[attention]:not([visuallyselected="true"]) {
> +  font-weight: bold;
> +}

So, am I reading this right that we're only showing the glow for tabs that have attention _and_ favicons? And to just bold the title otherwise?

	:Gijs (he/him) Assignee
	Comment 40 • 9 years ago

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #39)
> Comment on attachment 8673012 [details]
> MozReview Request: Bug 332195 - part 1: don't focus tabs by default, add
> pref to be able to revert late if we see too many issues, r?mconley
> 
> https://reviewboard.mozilla.org/r/21831/#review19721
> 
> ::: browser/base/content/tabbrowser.xml:4368
> (Diff revision 1)
> > -          // browser, but that's in the originalTarget.
> > -          // XXX Why originalTarget for the browser?
> > +          // browser, but that's in the originalTarget (not the target, because
> > +          // it's across a XBL boundary (that of the tabbrowser))
> 
> Nested parentheses! :D
> 
> Let's try this:
> 
> but that's in the originalTarget and not the target (since it's across the
> tabbrowser's XBL boundary).
> 
> ::: browser/base/content/tabbrowser.xml:5877
> (Diff revision 1)
> > -        <xul:hbox xbl:inherits="pinned,selected,visuallyselected,titlechanged,fadein"
> > +        <xul:hbox xbl:inherits="pinned,selected,visuallyselected,fadein"
> >                    class="tab-background">
> > -          <xul:hbox xbl:inherits="pinned,selected,visuallyselected,titlechanged"
> > +          <xul:hbox xbl:inherits="pinned,selected,visuallyselected"
> >                      class="tab-background-start"/>
> > -          <xul:hbox xbl:inherits="pinned,selected,visuallyselected,titlechanged"
> > +          <xul:hbox xbl:inherits="pinned,selected,visuallyselected"
> >                      class="tab-background-middle"/>
> > -          <xul:hbox xbl:inherits="pinned,selected,visuallyselected,titlechanged"
> > +          <xul:hbox xbl:inherits="pinned,selected,visuallyselected"
> 
> I assume you've audited the tab-background and
> tab-background-[start|middle|end] styles to ensure that titlechanged
> attribute isn't searched for?

Yes, I looked. This was originally added in an older bug about OSX styles, which have since been removed again (possibly in Australis), without adjusting the inheritance here.

You're right that this will impact add-ons. TBH I don't really see a lot of reason to inherit this all the way down into the bg, if necessary add-ons could use child selectors to get the behaviour they want.

> Maybe we'll need an add-on compat flag here too, since there might be some
> themes or add-ons relying on that attribute.

For sure.

> ::: browser/themes/shared/devedition.inc.css:286
> (Diff revision 1)
> > -.tabbrowser-tab[pinned][titlechanged]:not([visuallyselected="true"]) > .tab-stack > .tab-content {
> > +.tabbrowser-tab[image] > .tab-stack > .tab-content[attention]:not([visuallyselected="true"]),
> > +.tabbrowser-tab > .tab-stack > .tab-content[pinned][titlechanged]:not([visuallyselected="true"]) {
> >    background-image: var(--pinned-tab-glow);
> >    background-position: center;
> >    background-size: 100%;
> >  }
> >  
> > +.tabbrowser-tab[image] > .tab-stack > .tab-content[attention]:not([pinned]):not([visuallyselected="true"]) {
> > +  background-position: left bottom var(--tab-toolbar-navbar-overlap);
> > +  background-size: 34px 100%;
> > +}
> > +
> 
> Would you mind posting some screenshots with what this looks like?

You mean specifically for devedition, or in general?

> ::: browser/themes/shared/tabs.inc.css:447
> (Diff revision 1)
> > -.tabbrowser-tab[pinned][titlechanged]:not([visuallyselected="true"]) > .tab-stack > .tab-content {
> > +.tabbrowser-tab[image] > .tab-stack > .tab-content[attention]:not([visuallyselected="true"]),
> > +.tabbrowser-tab > .tab-stack > .tab-content[pinned][titlechanged]:not([visuallyselected="true"]) {
> >    background-image: radial-gradient(farthest-corner at center bottom, rgb(255,255,255) 3%, rgba(186,221,251,0.75) 20%, rgba(127,179,255,0.25) 40%, transparent 70%);
> >    background-position: center bottom var(--tab-toolbar-navbar-overlap);
> >    background-repeat: no-repeat;
> >    background-size: 85% 100%;
> >  }
> >  
> > +.tabbrowser-tab[image] > .tab-stack > .tab-content[attention]:not([pinned]):not([visuallyselected="true"]) {
> > +  background-position: left bottom var(--tab-toolbar-navbar-overlap);
> > +  background-size: 34px 100%;
> > +}
> > +
> > +.tab-label[attention]:not([visuallyselected="true"]) {
> > +  font-weight: bold;
> > +}
> 
> So, am I reading this right that we're only showing the glow for tabs that
> have attention _and_ favicons? And to just bold the title otherwise?

Yes. Without the favicon we don't really have a good way of showing the glow. We could force the empty icon (with dotted lines etc.) to be visible the way we do for pinned tabs for pages with no icon, of course, but that would be more jarring when you switch tabs and the effect goes away...

	Mike Conley (:mconley) (:⚙️)
	Comment 41 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #40)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #39)
> > 
> > Would you mind posting some screenshots with what this looks like?
> 
> You mean specifically for devedition, or in general?
> 

In general is fine.

	Mike Conley (:mconley) (:⚙️)
	Comment 42 • 9 years ago

Comment on attachment 8673013 [details]
MozReview Request: Bug 332195 - part 2: allow the user to control this behaviour with a checkbox, r?mconley

https://reviewboard.mozilla.org/r/21833/#review20029

::: browser/base/content/tabbrowser.xml:494
(Diff revision 1)
> +                // Hacks... but making the promptBox persist per browser so the information can
> +                // live in here is likely to cause leaks through the various ways that
> +                // browsers appear and disappear, because this closure has an awful
> +                // lot of references to aforementioned browser.

Perhaps we should then use a WeakMap inside the tabbrowser.xml then, keyed on the <xul:browser>'s permanent key or something?

I say permanent key, because I'm not 100% sure this attribute will survive after a tab tear out.

::: browser/base/content/tabbrowser.xml:4411
(Diff revision 1)
> +                                  (targetIsWindow && event.target.document.nodePrincipal) ||

It still bugs me that this evaluates to event.target.document.nodePrincipal, and not "true". :/

Bah, JavaScript.

::: browser/base/content/tabbrowser.xml:4414
(Diff revision 1)
> +            if (promptPrincipal.isNullPrincipal) {

So I guess there are no cases where promptPrincipal will be false-y?

::: browser/base/content/tabbrowser.xml:4422
(Diff revision 1)
> +                Services.perms.ALLOW_ACTION != Services.perms.testPermissionFromPrincipal(promptPrincipal, "focus-tab-by-prompt")) {

Maybe locally alias Services.perms, or somehow otherwise reduce the length of this line if you can.

::: toolkit/components/prompts/src/nsPrompter.js:489
(Diff revision 1)
>      args.showAlertOrigin = topPrincipal.equals(promptPrincipal);

Maybe this is a good time to flip the logic, and callers everywhere?

Or at least file a bug for it? Because yeah, pretty unintuitive.

	:Gijs (he/him) Assignee
	Comment 43 • 9 years ago

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #42)
> Comment on attachment 8673013 [details]
> MozReview Request: Bug 332195 - part 2: allow the user to control this
> behaviour with a checkbox, r?mconley
> 
> https://reviewboard.mozilla.org/r/21833/#review20029
> 
> ::: browser/base/content/tabbrowser.xml:494
> (Diff revision 1)
> > +                // Hacks... but making the promptBox persist per browser so the information can
> > +                // live in here is likely to cause leaks through the various ways that
> > +                // browsers appear and disappear, because this closure has an awful
> > +                // lot of references to aforementioned browser.
> 
> Perhaps we should then use a WeakMap inside the tabbrowser.xml then, keyed
> on the <xul:browser>'s permanent key or something?

The tabbox has a closure reference to the browser element (and needs that in order to function). So that wouldn't help; the browser would keep being referenced by the value of the item in the weakmap, which would therefore keep its key alive.

> I say permanent key, because I'm not 100% sure this attribute will survive
> after a tab tear out.

Ditto.

> ::: browser/base/content/tabbrowser.xml:4414
> (Diff revision 1)
> > +            if (promptPrincipal.isNullPrincipal) {
> 
> So I guess there are no cases where promptPrincipal will be false-y?

I don't think so, no...

> ::: browser/base/content/tabbrowser.xml:4422
> (Diff revision 1)
> > +                Services.perms.ALLOW_ACTION != Services.perms.testPermissionFromPrincipal(promptPrincipal, "focus-tab-by-prompt")) {
> 
> Maybe locally alias Services.perms, or somehow otherwise reduce the length
> of this line if you can.

OK.

> ::: toolkit/components/prompts/src/nsPrompter.js:489
> (Diff revision 1)
> >      args.showAlertOrigin = topPrincipal.equals(promptPrincipal);
> 
> Maybe this is a good time to flip the logic, and callers everywhere?
> 
> Or at least file a bug for it? Because yeah, pretty unintuitive.

I can file that bug.


It seems like we really want/need a UI review though, when we discussed this in the win10 meeting not everyone seemed thrilled by the idea of the (extra) checkbox here.

	(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo
	Comment 47 • 9 years ago

Comment on attachment 8673013 [details]
MozReview Request: Bug 332195 - part 2: allow the user to control this behaviour with a checkbox, r?mconley

Looks good!
The checkbox is a compromise, but it's definitely better than the current behavior.

	:Gijs (he/him) Assignee
	Comment 48 • 9 years ago

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #42)
> Comment on attachment 8673013 [details]
> MozReview Request: Bug 332195 - part 2: allow the user to control this
> behaviour with a checkbox, r?mconley
> 
> https://reviewboard.mozilla.org/r/21833/#review20029
> 
> ::: browser/base/content/tabbrowser.xml:494
> (Diff revision 1)
> > +                // Hacks... but making the promptBox persist per browser so the information can
> > +                // live in here is likely to cause leaks through the various ways that
> > +                // browsers appear and disappear, because this closure has an awful
> > +                // lot of references to aforementioned browser.
> 
> Perhaps we should then use a WeakMap inside the tabbrowser.xml then, keyed
> on the <xul:browser>'s permanent key or something?
> 
> I say permanent key, because I'm not 100% sure this attribute will survive
> after a tab tear out.

So the two calls in question are the firing of the DOMWillOpenModalDialog event, and the actual Prompt being opened via the call to appendPrompt.

In the non-e10s case all of this is sync. The event fires and synchronously, after it has been fully dispatched, we call appendPrompt. It is impossible for someone to tear the tab "inbetween" these two calls (unless we create code listening to that event that does that).

In the e10s case, both the event and the actual creation of the prompt are done from a single message from content that reaches RemotePrompts.jsm, so it is likewise all sync, from DOMWillOpenModalDialog until we show the dialog.

Therefore I don't think the tear-out is a real problem.

Regarding the WeakMap, a bit more explicitly: because the value in the map will hold a strong reference (indirect, if we key on permanentKey, ie tabPrompt->browser->permanentKey) to the key, the key will never be GC'd triggering removal from the weakmap, which will mean we'll just leak everything.

One other solution would be to try to make the decision about whether we tab-switch, and/or need to show the extra checkbox, in the calling code, but that would result in it being duplicated

I think we should land that part of it, at least, as-is, unless you have a different suggestion about how to solve this problem.


The one other thing we could do would be to move the logic into the prompters, but then they have to quiz the tabbrowser on whether the tab is selected, which also feels wrong...

	:Gijs (he/him) Assignee
	Comment 49 • 9 years ago

Bug 332195 - part 0: refactor tabprompts to persist per-browser, r?mconley

	:Gijs (he/him) Assignee
	Comment 50 • 9 years ago

Comment on attachment 8673012 [details]
MozReview Request: Bug 332195 - part 1: don't focus tabs by default, add pref to be able to revert late if we see too many issues, r?mconley

Bug 332195 - part 1: don't focus tabs by default, add pref to be able to revert late if we see too many issues, r?mconley

	:Gijs (he/him) Assignee
	Comment 51 • 9 years ago

Comment on attachment 8673013 [details]
MozReview Request: Bug 332195 - part 2: allow the user to control this behaviour with a checkbox, r?mconley

Bug 332195 - part 2: allow the user to control this behaviour with a checkbox, r?mconley

	:Gijs (he/him) Assignee
	Comment 52 • 9 years ago

Bug 332195 - part 4: add rudimentary test, r?mconley

	:Gijs (he/him) Assignee
	Comment 53 • 9 years ago

Tests pass locally, but we all know that's just to make us all get deceived when we finally land stuff, and this is the real test (well, kind of):

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

	Mike Conley (:mconley) (:⚙️)
	Comment 54 • 9 years ago

Comment on attachment 8678182 [details]
MozReview Request: Bug 332195 - part 0: refactor tabprompts to persist per-browser, r?mconley

https://reviewboard.mozilla.org/r/23117/#review20611

Beautiful!

	:Gijs (he/him) Assignee
	Comment 55 • 9 years ago

It seems the test I added fails on infra in e10s mode, and that the change might be causing tabview test failures (sigh). It also seems like infra really likes builds to fail despite them uploading usable artifacts (double sigh).

I'll look at this either this weekend or next week. Setting needinfo so I don't forget.

	Mike Conley (:mconley) (:⚙️)
	Comment 56 • 9 years ago

Comment on attachment 8673012 [details]
MozReview Request: Bug 332195 - part 1: don't focus tabs by default, add pref to be able to revert late if we see too many issues, r?mconley

https://reviewboard.mozilla.org/r/21831/#review20635

	Mike Conley (:mconley) (:⚙️)
	Comment 57 • 9 years ago

Comment on attachment 8673013 [details]
MozReview Request: Bug 332195 - part 2: allow the user to control this behaviour with a checkbox, r?mconley

https://reviewboard.mozilla.org/r/21833/#review20613

::: browser/locales/en-US/chrome/browser/tabbrowser.properties:48
(Diff revision 2)
> +tabs.allowTabFocusByPromptForSite=Allow dialogs from %S to make their tab the selected tab

Maybe we should run this by Matej to make sure the language is as simple as possible.

	Mike Conley (:mconley) (:⚙️)
	Comment 58 • 9 years ago

Comment on attachment 8678183 [details]
MozReview Request: Bug 332195 - part 4: add rudimentary test, r?mconley

https://reviewboard.mozilla.org/r/23119/#review20637

Thanks for the test! This is great.

::: browser/base/content/test/general/browser_openPromptInBackgroundTab.js:10
(Diff revision 1)
> +add_task(function*() {

Please add a brief comment describing what exactly this test tests.

	:Gijs (he/him) Assignee
	Comment 59 • 9 years ago

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #57)
> Comment on attachment 8673013 [details]
> MozReview Request: Bug 332195 - part 2: allow the user to control this
> behaviour with a checkbox, r?mconley
> 
> https://reviewboard.mozilla.org/r/21833/#review20613
> 
> ::: browser/locales/en-US/chrome/browser/tabbrowser.properties:48
> (Diff revision 2)
> > +tabs.allowTabFocusByPromptForSite=Allow dialogs from %S to make their tab the selected tab
> 
> Maybe we should run this by Matej to make sure the language is as simple as
> possible.

Hmm, probably:

Allow dialogs from %S to select their tab

is simpler and better.

Matej, do you have a different suggestion or does that sound OK to you?

	Mark Straver
	Comment 60 • 9 years ago

What's wrong with just using the word "focus"? :P

"Allow dialogs from %S to select their tab" is certainly not very clear to me.

	:Gijs (he/him) Assignee
	Comment 61 • 9 years ago

(In reply to Mark Straver from comment #60)
> What's wrong with just using the word "focus"? :P
> 
> "Allow dialogs from %S to select their tab" is certainly not very clear to
> me.

It's a more technical term (than "select"), and it's not actually correct here: if the tab (not the tab's content!) was focused but not selected, you could then activate/select the tab more easily, but the display of the page wouldn't change until you did that, and depending on what you were doing with your keyboard, it might or might not have kept working. The annoying thing is that the selected tab changes, causing different content to be displayed, and meaning that e.g. accel-w will now suddenly close a different tab.

	Mark Straver
	Comment 62 • 9 years ago

So what you're looking for is then either "switch to" or "bring to the foreground" or somesuch. To me, "select" is not the same as either of those since a selection is not exclusive and the resulting language is confusing...

	Jerry Baker
	Comment 63 • 9 years ago

Automatically switch to tab with an active dialog?

	Jerry Baker
	Comment 64 • 9 years ago

My previous wording doesn't really paint the right picture because it doesn't really suggest that popping a new dialog will trigger the switch. Perhaps a better way to say it might be:

When a page generates a dialog, automatically switch to that tab.

	:Gijs (he/him) Assignee
	Comment 65 • 9 years ago

Comment on attachment 8678182 [details]
MozReview Request: Bug 332195 - part 0: refactor tabprompts to persist per-browser, r?mconley

Bug 332195 - part 0: refactor tabprompts to persist per-browser, r?mconley

	:Gijs (he/him) Assignee
	Comment 66 • 9 years ago

Comment on attachment 8673012 [details]
MozReview Request: Bug 332195 - part 1: don't focus tabs by default, add pref to be able to revert late if we see too many issues, r?mconley

Bug 332195 - part 1: don't focus tabs by default, add pref to be able to revert late if we see too many issues, r?mconley

	:Gijs (he/him) Assignee
	Comment 67 • 9 years ago

Comment on attachment 8673013 [details]
MozReview Request: Bug 332195 - part 2: allow the user to control this behaviour with a checkbox, r?mconley

Bug 332195 - part 2: allow the user to control this behaviour with a checkbox, r?mconley

	:Gijs (he/him) Assignee
	Comment 68 • 9 years ago

Comment on attachment 8678183 [details]
MozReview Request: Bug 332195 - part 4: add rudimentary test, r?mconley

Bug 332195 - part 4: add rudimentary test, r?mconley

	:Gijs (he/him) Assignee
	Comment 69 • 9 years ago

Bug 332195 - part 5: fix test for e10s, r?mconley

	:Gijs (he/him) Assignee
	Comment 70 • 9 years ago

Bug 332195 - part 6: make an exception for beforeunload events while tabview is open, r?mconley

	:Gijs (he/him) Assignee
	Comment 71 • 9 years ago

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

	Mike Conley (:mconley) (:⚙️)
	Comment 72 • 9 years ago

Comment on attachment 8678818 [details]
MozReview Request: Bug 332195 - part 5: fix test for e10s, r?mconley

https://reviewboard.mozilla.org/r/23255/#review20727

Good stuff, and nice addition to BTU.

	Mike Conley (:mconley) (:⚙️)
	Comment 73 • 9 years ago

Comment on attachment 8678819 [details]
MozReview Request: Bug 332195 - part 6: make an exception for beforeunload events while tabview is open, r?mconley

https://reviewboard.mozilla.org/r/23257/#review20729

::: browser/base/content/tabbrowser.xml:4349
(Diff revision 1)
> -          if (event.detail && event.detail.tabPrompt &&
> +          if (event.detail && event.detail.tabPrompt && 

Trailing ws

::: browser/base/content/tabbrowser.xml:4350
(Diff revision 1)
> +              (!event.detail.inPermitUnload || !("TabView" in window) || !TabView.isVisible()) &&

This is getting hard to parse.

For future readers, can you quickly explain what the contents of the parentheses is checking for in a comment above it?

::: toolkit/components/prompts/src/nsPrompter.js:440
(Diff revision 1)
> -    let messageManager = domWin.QueryInterface(Ci.nsIInterfaceRequestor)
> +    let docShell = domWin.QueryInterface(Ci.nsIInterfaceRequestor)
>                           .getInterface(Ci.nsIWebNavigation)
> -                         .QueryInterface(Ci.nsIDocShell)
> +                         .QueryInterface(Ci.nsIDocShell);

Can you not just getInterface the domWin directly to the nsIDocShell like you did in `openTabPrompt`?

	Matej Novak [:matej]
	Comment 74 • 9 years ago

(In reply to :Gijs Kruitbosch (at OSCON, limited availability) from comment #59)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #57)
> > Comment on attachment 8673013 [details]
> > MozReview Request: Bug 332195 - part 2: allow the user to control this
> > behaviour with a checkbox, r?mconley
> > 
> > https://reviewboard.mozilla.org/r/21833/#review20613
> > 
> > ::: browser/locales/en-US/chrome/browser/tabbrowser.properties:48
> > (Diff revision 2)
> > > +tabs.allowTabFocusByPromptForSite=Allow dialogs from %S to make their tab the selected tab
> > 
> > Maybe we should run this by Matej to make sure the language is as simple as
> > possible.
> 
> Hmm, probably:
> 
> Allow dialogs from %S to select their tab
> 
> is simpler and better.
> 
> Matej, do you have a different suggestion or does that sound OK to you?

This is definitely cleaner, but it introduces some ambiguity about who "their" refers to, whether it's the site or the user (though maybe it would be clearer in context).

Maybe it needs a "you" or something to help clarify:

"Allow dialogs from %S to take you to their tab"

Or maybe adding "open" helps?

"Allow dialogs from %S to select their open tab"

	Pulsebot
	Comment 75 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/19c3ac2af9d9
https://hg.mozilla.org/integration/fx-team/rev/96d3a95d7dc6
https://hg.mozilla.org/integration/fx-team/rev/96bb0f79639d
https://hg.mozilla.org/integration/fx-team/rev/e9d1a369d217
https://hg.mozilla.org/integration/fx-team/rev/a60f220db385
https://hg.mozilla.org/integration/fx-team/rev/31033f04e561

	:Gijs (he/him) Assignee
	Comment 76 • 9 years ago

(In reply to Matej Novak [:matej] from comment #74)
> This is definitely cleaner, but it introduces some ambiguity about who
> "their" refers to, whether it's the site or the user (though maybe it would
> be clearer in context).
> 
> Maybe it needs a "you" or something to help clarify:
> 
> "Allow dialogs from %S to take you to their tab"

I went with this because I think the "take" is helpful here in at least alluding to how intrusive this can be.

	Carsten Book [:Tomcat]
	Comment 77 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/96bb0f79639d
https://hg.mozilla.org/mozilla-central/rev/e9d1a369d217
https://hg.mozilla.org/mozilla-central/rev/a60f220db385
https://hg.mozilla.org/mozilla-central/rev/31033f04e561

	kud
	Comment 78 • 9 years ago

Thank you so much for this fix. It makes my days really better. ;)

	:Gijs (he/him) Assignee
	Comment 79 • 9 years ago

Note that for bug 1234936 I had to undo this for beforeunload dialogs - see that bug and some of the other deps for details about why. I'll likely request uplift there to address that in 44+. If we can't make that happen we might need to turn this off for 44. :-(

	pkay42
	Comment 80 • 8 years ago

That's a nice little fix!  How do you go about unchecking the checkbox if you decide you don't want that behavior any more?

	:Gijs (he/him) Assignee
	Comment 81 • 8 years ago

(In reply to pkay42 from comment #80)
> That's a nice little fix!  How do you go about unchecking the checkbox if
> you decide you don't want that behavior any more?

You can't right now. See bug 1224137. For a workaround, run:

Services.perms.removeFromPrincipal(gBrowser.selectedBrowser.contentPrincipal, "focus-tab-by-prompt");

in the (app-privileged) browser console ( https://developer.mozilla.org/en/docs/Tools/Browser_Console ).