Closed Bug 878905 Opened 7 years ago Closed 6 years ago

improve camera button menuitem labels

Categories

(Firefox :: General, defect)

23 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Details

Attachments

(1 file, 1 obsolete file)

Started conversation in bug 871931, look there for previous details.

The issue is that urls in the button are not very good indicators to the user.  The document title would potentially be better.  Another option is to have both document title and url.  The favicon is probably a good idea as well.
Attached patch use favicon and document title (obsolete) — Splinter Review
Here's a quick patch that adds favicon and uses doc title for the label.  I think that the icon is quite useful for quick/easy identification if the user has streams open from more than one site.  The url is still in the tooltip.
Attachment #757632 - Flags: feedback?(dolske)
Boriss, the original mockups for the camera button in bug 799417 used the page url as the label of the menuitem.  The patch here would change that to favicon + document title.  For something like Talkilla, there are likely to be multiple entries with a very similar url, making the current menu (using urls) relatively ineffective.  What do you think of this change?
Flags: needinfo?(jboriss)
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Boriss, the original mockups for the camera button in bug 799417 used the
> page url as the label of the menuitem.  The patch here would change that to
> favicon + document title.  For something like Talkilla, there are likely to
> be multiple entries with a very similar url, making the current menu (using
> urls) relatively ineffective.  What do you think of this change?

Makes sense not to use URLs.  As Dolske notes in bug 871931#c6, users have already granted permission to the page on the page itself, so if a page is spoofing a page title it's hardly the most important security concern at that point.  Also, granting access to multiple URLs on the same domain would make this a nightmare.  

For something like Talkilla or a video chat in SocialAPI, what would "contentDocument.title" entail?  New strings may be needed for some of our projects that ask for video permission.  The title bar of a chat in SocialAPI currently just has the name of the person being contacted, where really we'd want something more like "Bill Hicks - chat" or "Bill Hicks - video chat."
Flags: needinfo?(jboriss)
(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #3)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #2)

> For something like Talkilla or a video chat in SocialAPI, what would
> "contentDocument.title" entail?  New strings may be needed for some of our
> projects that ask for video permission.  The title bar of a chat in
> SocialAPI currently just has the name of the person being contacted, where
> really we'd want something more like "Bill Hicks - chat" or "Bill Hicks -
> video chat."

The document title is defined by the web page loaded into the window.  We could append something to that, but I'm figuring the favicon and title is enough to identify what it is.  I'd prefer to stick to document title for now, and modify further later if there is a need.
Attachment #757632 - Flags: feedback?(dolske) → review?(dao)
@dao ping
Comment on attachment 757632 [details] [diff] [review]
use favicon and document title

>-      menuitem.setAttribute("label", streamData.uri);
>+      menuitem.setAttribute("class", "menuitem-iconic");
>+      menuitem.setAttribute("label", streamData.browser.contentDocument.title || pageURI.host);

Accessing browser.contentDocument.title is bad for e10s. You should use browser.contentTitle, which is managed by the remote-browser binding.

I'm not sure we should reduce the label to the host instead of the full URI in case there's no contentTitle...

>+      PlacesUtils.favicons.getFaviconURLForPage(pageURI, function (aURI) {
>+        if (aURI) {
>+          let iconURL = PlacesUtils.favicons.getFaviconLinkForIcon(aURI).spec;
>+          menuitem.style.listStyleImage = "url(" + iconURL + ")";

Setting menuitem.image should work here.
Attachment #757632 - Flags: review?(dao) → review-
updated with feedback.
Assignee: nobody → mixedpuppy
Attachment #757632 - Attachment is obsolete: true
Attachment #811347 - Flags: review?(dao)
Attachment #811347 - Flags: review?(dao) → review+
https://hg.mozilla.org/mozilla-central/rev/a4c2df11b142
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.