Shane Caraveo (:mixedpuppy) Assignee
	Description • 12 years ago

The camera/mic toolbarbutton needs to support non-gBrowser iframes for social

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 1 • 11 years ago

Patch makes the toolbarbutton work with iframes/browsers that are not part of gBrowser.  That allows social chat windows to be listed and get properly focused.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 2 • 11 years ago

Using URLs for the camera menu seems awkward.  Is there a reason for URL vs. Document Title?

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 3 • 11 years ago

Comment on attachment 756732 [details]
cameramenu.png

dolske, since your in recent reviews in this area, thought I'd ask whether it is important to have URLs in this menu, or if document titles would be fine.  See attached image.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 4 • 11 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Created attachment 756732 [details]
> cameramenu.png
> 
> Using URLs for the camera menu seems awkward.  Is there a reason for URL vs.
> Document Title?

It's much harder to fake URLs than document titles.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 5 • 11 years ago

(In reply to Jared Wein [:jaws] from comment #4)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> > Created attachment 756732 [details]
> > cameramenu.png
> > 
> > Using URLs for the camera menu seems awkward.  Is there a reason for URL vs.
> > Document Title?
> 
> It's much harder to fake URLs than document titles.

So i've opened a tab, I've allowed that page to use video/audio via permissions panel, and we still need a url in that menu?

	Justin Dolske [:Dolske]
	Comment 6 • 11 years ago

The main purpose of the camera menu is to let you know a media device (cam/mic) is active, and to provide a way to go to the tab that's using it. I don't recall previous discussion on it, but I don't think there's any strict reason that it must be a URL. EG if the page title was used, the user would still have needed to previously grant permission to misleadingsite.com, and can still use this menu to switch to that tab to close it.

For things that are essentially already trusted by the browser (social API site or an addon), I think a URL is even less useful, and should just be a descriptive string ("Facebook" or "Facebook <mumble>").

That also raises the question of what selecting an item should do for those cases. Maybe it should be a mechanism that sends a message to the social provider / addon telling it to do some kind of appropriate UI notification/display so the user knows what's going on (eg raise the video chat window).

And maybe the camera menu needs to have a way to allow canceling a media stream, since "select item, close tab" method of forcing a thing to go away no longer applies.

Boriss -- UX thoughts?

	Nicolas Perriault (:NiKo`) — needinfo me if you need my attention
	Comment 7 • 11 years ago

Two cents: why do not use both title and url as a desription, like the awesome bar does? http://cl.ly/image/0y403B070s3W

	Justin Dolske [:Dolske]
	Comment 8 • 11 years ago

Hmm, interesting idea. I'd want to think about the potential for confusion (and I still don't think an URL is super helpful here), but it's worth considering. Favicon too.

	Dão Gottwald [:dao]
	Comment 9 • 11 years ago

Comment on attachment 756725 [details] [diff] [review]
camera button patch

>   menuCommand: function (aMenuitem) {
>     let streamData = this._menuitemData.get(aMenuitem);
>     if (!streamData)
>       return;
> 
>-    let tab = streamData.tab;
>-    let browserWindow = tab.ownerDocument.defaultView;
>-    browserWindow.gBrowser.selectedTab = tab;
>-    browserWindow.focus();
>+    if (streamData.tab) {
>+      let browserWindow = streamData.browser.ownerDocument.defaultView;
>+      browserWindow.gBrowser.selectedTab = streamData.tab;
>+      browserWindow.focus();
>+    } else if (streamData.browser) {
>+      streamData.browser.contentWindow.focus();
>+    }

You need to call browserWindow.focus outside of the if/else branches.

Also please call streamData.browser.focus rather than streamData.browser.contentWindow.focus.

>   get activeStreams() {
>     let contentWindowSupportsArray = MediaManagerService.activeMediaCaptureWindows;
>     let count = contentWindowSupportsArray.Count();
>     let activeStreams = [];
>     for (let i = 0; i < count; i++) {
>       let contentWindow = contentWindowSupportsArray.GetElementAt(i);
>-      let browserWindow = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+      let browser = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>                                        .getInterface(Ci.nsIWebNavigation)
>                                        .QueryInterface(Ci.nsIDocShell)
>-                                       .chromeEventHandler.ownerDocument.defaultView;
>+                                       .chromeEventHandler;

Please replace these lines with:

let browser = getBrowserForWindow(contentWindow);

>+      let browserWindow = browser.ownerDocument.defaultView;
>       let tab = browserWindow.gBrowser &&
>                 browserWindow.gBrowser._getTabForContentWindow(contentWindow.top);
>-      if (tab) {
>+      if (browser) {
>         activeStreams.push({
>           uri: contentWindow.location.href,
>-          tab: tab
>+          tab: tab,
>+          browser: browser
>         });
>       }

'browser' cannot be null here, so no need to check it.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 10 • 11 years ago

feedback added.  note, I have to call focus on the window then focus on the browser to correctly get focus into alternate browser elements such as the social chat windows.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 11 • 11 years ago

created bug 878905 for the button menuitem labels.

	Dão Gottwald [:dao]
	Comment 12 • 11 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> note, I have to call focus on the window then focus on the
> browser to correctly get focus into alternate browser elements such as the
> social chat windows.

I'm not sure what this means. What exactly didn't work correctly?

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 13 • 11 years ago

(In reply to Dão Gottwald [:dao] from comment #12)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> > note, I have to call focus on the window then focus on the
> > browser to correctly get focus into alternate browser elements such as the
> > social chat windows.
> 
> I'm not sure what this means. What exactly didn't work correctly?

browserWindow.focus will bring the firefox window forward, but the default is to focus into the tabbrowser.  So the second focus call on the browser element itself is necessary to ensure that the focus is correct (e.g. for social).

	Dão Gottwald [:dao]
	Comment 14 • 11 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #12)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> > > note, I have to call focus on the window then focus on the
> > > browser to correctly get focus into alternate browser elements such as the
> > > social chat windows.
> > 
> > I'm not sure what this means. What exactly didn't work correctly?
> 
> browserWindow.focus will bring the firefox window forward, but the default
> is to focus into the tabbrowser.

There's no such default that I know of.

> So the second focus call on the browser
> element itself is necessary to ensure that the focus is correct (e.g. for
> social).

But you're doing this unconditionally rather than just for the tab-less case.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 15 • 11 years ago

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> > (In reply to Dão Gottwald [:dao] from comment #12)
> > > (In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> > > > note, I have to call focus on the window then focus on the
> > > > browser to correctly get focus into alternate browser elements such as the
> > > > social chat windows.
> > > 
> > > I'm not sure what this means. What exactly didn't work correctly?
> > 
> > browserWindow.focus will bring the firefox window forward, but the default
> > is to focus into the tabbrowser.
> 
> There's no such default that I know of.

Right, sorry, to be more correct, the focus is where it was last.  When you call browserWindow.focus() you're only bringing the window to front, but not shifting focus.  With socialapi, there are more potential focus area's (sidebar and chat windows) and we need to be sure we set focus into the correct browser as selected by the user.

> > So the second focus call on the browser
> > element itself is necessary to ensure that the focus is correct (e.g. for
> > social).
> 
> But you're doing this unconditionally rather than just for the tab-less case.

Yes, it needs to be done, otherwise focus can be in the wrong place for the item you select.  But I can check focusedElement first to see if it is necessary.

Consider this:

2 browser windows.  In window #1, one tab connected to webrtc, and the social sidebar is open and has focus.  In window #2, I select the camera button, and choose the menuitem for the tab in window #1.  Window #1 is brought to front by the browserWindow.focus() call.

Without calling streamData.browser.focus(), the focus remains in the sidebar, even though I selected the tab.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 16 • 11 years ago

fix from previous patch: only focus to the browser element if necessary

	Dão Gottwald [:dao]
	Comment 17 • 11 years ago

Comment on attachment 757611 [details] [diff] [review]
camera button patch

>   menuCommand: function (aMenuitem) {
>     let streamData = this._menuitemData.get(aMenuitem);
>     if (!streamData)
>       return;
> 
>-    let tab = streamData.tab;
>-    let browserWindow = tab.ownerDocument.defaultView;
>-    browserWindow.gBrowser.selectedTab = tab;
>+    let browserWindow = streamData.browser.ownerDocument.defaultView;
>+    if (streamData.tab) {
>+      browserWindow.gBrowser.selectedTab = streamData.tab;
>+    }
>+    // bring window to front
>     browserWindow.focus();
>+    // focus on the correct browser element within the window (e.g. could be a
>+    // social chat window)
>+    let focused = document.commandDispatcher.focusedElement;
>+    if (focused.ownerDocument != streamData.browser.contentDocument)
>+      streamData.browser.focus();

Call streamData.browser.focus only if there's no streamData.tab.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 18 • 11 years ago

(In reply to Dão Gottwald [:dao] from comment #17)
> Comment on attachment 757611 [details] [diff] [review]
> camera button patch

> >+    // focus on the correct browser element within the window (e.g. could be a
> >+    // social chat window)
> >+    let focused = document.commandDispatcher.focusedElement;
> >+    if (focused.ownerDocument != streamData.browser.contentDocument)
> >+      streamData.browser.focus();
> 
> Call streamData.browser.focus only if there's no streamData.tab.

as I tried to explain, that doesnt work.  If the sidebar or a chat window has focus, and you selected a tab, we need to focus to the browser in the tab.

	Dão Gottwald [:dao]
	Comment 19 • 11 years ago

I see no reason why we should do that. Selecting a tab from the list is no indication that the user wants to interact with the content, it just means that that tab should be selected.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 20 • 11 years ago

I guess I see it as a change in behavior.  In a non-social world, the selection of the tab also results in that tab having focus again (assuming it previously had focus somewhere).  Once we add a social panel into the mix, the focus may no longer be in the tab.  I'll change the patch.

	Dão Gottwald [:dao]
	Comment 22 • 11 years ago

Comment on attachment 758302 [details] [diff] [review]
camera button patch

>+    let browserWindow = streamData.browser.ownerDocument.defaultView;
>+    if (streamData.tab) {
>+      browserWindow.gBrowser.selectedTab = streamData.tab;
>+    }
>+    // bring window to front
>     browserWindow.focus();
>+
>+    // focus on the correct social browser element within the window (e.g. could
>+    // be a social chat window)
>+    if (!streamData.tab) {
>+      streamData.browser.focus();
>+    }

This code is social-agnostic, so please drop that comment referring to social browser elements.

Also make the two 'if' statements an if/else one:

    if (streamData.tab)
      browserWindow.gBrowser.selectedTab = streamData.tab;
    else
      streamData.browser.focus();

r=me with that

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 24 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6725005dc27f

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/6725005dc27f