530999 - Login manager UI should show site favicons

Status: VERIFIED FIXED

(Toolkit :: Password Manager, enhancement)

Description

[Justin Dolske [:Dolske]]

Finding logins in the preferences UI would be a lot easier if the site's favicon was shown in the list (using nsIFaviconService).

Might be worth doing this for other UI, like cookies, too.

Comment 1

[Matthew N. [:MattN]]

This probably requires getImageSrc:
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/content/passwordManager.js?rev=1249401fb8b0&mark=53#46
+
PlacesUtils.promiseFaviconLinkUrl:
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm?rev=802e3c169db8#1650
and dealing with the fact that the latter is async.

We should probably create a Map of distinct origins from logins to favicon URLs and when all of the promises resolve or reject then force a tree update. getImageSrc would probe the Map and fallback to the default globe if there is no favicon found.

Comment 2

[Matthew N. [:MattN]]

Perhaps due to the async nature of PlacesUtils.promiseFaviconLinkUrl, a better solution may be to dynamically append CSS rules using ::moz-tree-image(https://example.com) to a new stylesheet and have the origin added as a property in getCellProperties. https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Styling_a_Tree#Setting_Properties_for_the_Custom_View

Comment 3

[Joseph Yeh [:joseph]]

Hi Matthew,

I am interested in this bug, so I just take it and see how to deal with it.

This will be my very first bug for Firefox Desktop Browser and might need some mentoring on this.

Thanks :)

Comment 4

[Joseph Yeh [:joseph]]

Attachment: 0001-Bug-530999-Show-site-favicons-in-login-manager-UI.patch

Hi Matthew,

I implement the getImageSrc function in passwordManager.js with PlacesUtils.promiseFaviconLinkUrl as you have described in comment 1. And I also create a _faviconMap to store the favicon URLs.


My questions are listed below:

1. I am using ‘chrome://mozapps/skin/places/defaultFavicon.png’ as the default icon, but I’m not sure if this is the correct one.

2. The _faviconMap will be reset after the login manager dialog closed. Do we need to store it somewhere in order to keep the Map data?

3. The tree must update after the promise resolve or reject, but I just couldn’t find how to make this happen. Can you give me some hint about this?

4. From your comment 2, I try to add some rules like treechildren::moz-tree-image(https://twitter.com) and add the origin property in getCellProperties, but it didn’t work. I have no idea how to fix it, so I just skip this solution. Maybe you can give me more tips?

This is my first patch, please take a look and give me some feedback. If I missed something please point it out for me, thanks :)

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8739904 [details] [diff] [review]
0001-Bug-530999-Show-site-favicons-in-login-manager-UI.patch

(In reply to Joseph Yeh [:joseph] from comment #4)
> Created attachment 8739904 [details] [diff] [review]
> 0001-Bug-530999-Show-site-favicons-in-login-manager-UI.patch
> 
> Hi Matthew,
> 
> I implement the getImageSrc function in passwordManager.js with
> PlacesUtils.promiseFaviconLinkUrl as you have described in comment 1. And I
> also create a _faviconMap to store the favicon URLs.
> 
> 
> My questions are listed below:
> 
> 1. I am using ‘chrome://mozapps/skin/places/defaultFavicon.png’ as the
> default icon, but I’m not sure if this is the correct one.

Yes, I think that's correct but you also need at 1.1dppx media rule to use the @2x version for high DPI screens.

> 2. The _faviconMap will be reset after the login manager dialog closed. Do
> we need to store it somewhere in order to keep the Map data?

No

> 3. The tree must update after the promise resolve or reject, but I just
> couldn’t find how to make this happen. Can you give me some hint about this?

I think the best way is to use the approach from comment 2 using https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet/insertRule

> 4. From your comment 2, I try to add some rules like
> treechildren::moz-tree-image(https://twitter.com) and add the origin
> property in getCellProperties, but it didn’t work. I have no idea how to fix
> it, so I just skip this solution. Maybe you can give me more tips?

Can you try with a simple hard-coded example e.g. have a property of "foo" for all and see if the CSS rule will match it? MDN and https://mxr.mozilla.org/mozilla-central/search?string=cyclerState1 are simpler examples.

Comment 6

[Joseph Yeh [:joseph]]

Attachment: 0001-test-moz-tree-image-with-hostname.patch

Hi Matthew,

I just create a simple patch to test the rule of ::moz-tree-image.

As you can see in the patch, I add three properties (twitter, twitter.com and https://twitter.com) in getCellProperties and add three different rules corresponding to each properties.


treechildren::-moz-tree-image(twitter) {
  background-color: red;
}

treechildren::-moz-tree-image(twitter.com) {
  background-color: blue;
}

treechildren::-moz-tree-image(https://twitter.com) {
  background-color: green;
}


The result of the background color is red. Does it means that the character like '.' or ':' and '/' in the rules of ::-moz-tree-image is not a valid rule?

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8740863 [details] [diff] [review]
0001-test-moz-tree-image-with-hostname.patch

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

::: toolkit/themes/osx/global/passwordmgr.css
@@ +24,5 @@
> +treechildren::-moz-tree-image(twitter.com) {
> +  background-color: blue;
> +}
> +
> +treechildren::-moz-tree-image(https://twitter.com) {

When you dynamically append the rule you need to use https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape on the url argument then it works fine:

treechildren::-moz-tree-image(https\:\/\/twitter\.com)

Comment 8

[Joseph Yeh [:joseph]]

Attachment: 0002-Bug-530999-Show-site-favicons-in-login-manager-UI.patch

Hi Matthew,

Thanks for your tips.

I made another patch which insert the rules dynamically and also add media rules for @2x version.


Still have some follow-up questions:

1. I do all the stuff inside the getCellProperties function, is it appropriate to do so?

2. I use document.styleSheets[1] (passwordmgr.css) to insert css rules because I couldn't find the way to insert a new stylesheet in XUL. Do you have any example for this?

3. It looks like the tree still didn't update at the first place. How should I trigger a tree update?


Thanks!

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8741271 [details] [diff] [review]
0002-Bug-530999-Show-site-favicons-in-login-manager-UI.patch

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

Can you please obsolete the old attachments that you aren't going to use as I currently am confused about which ones you plan to land.

(In reply to Joseph Yeh [:joseph] from comment #8)
> 1. I do all the stuff inside the getCellProperties function, is it
> appropriate to do so?

I think that's fine for triggering the fetches and inserting the rules after getting a favicon. I think the default styles belong in the CSS file. Btw. if you want to de-dupe the two passwordmgr.css into toolkit/themes/shared/ you can do that in this or a dependency bug.

> 2. I use document.styleSheets[1] (passwordmgr.css) to insert css rules
> because I couldn't find the way to insert a new stylesheet in XUL. Do you
> have any example for this?

https://developer.mozilla.org/en-US/docs/Web/API/Document/createProcessingInstruction but I think you shouldn't do this programmatically, just add:

<!-- This stylesheet is used to dynamically add rules for login favicons. -->
<?xml-stylesheet href="data:text/css," type="text/css"?>

to the top of passwordManager.xul after existing styleSheets then add a lazy getter XPCOMUtils.defineLazyGetter(…) to iterate over document.styleSheets and find the one with the matching @href and append to it. e.g. https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-addons.js?rev=5e40adeb0332#657

> 3. It looks like the tree still didn't update at the first place. How should
> I trigger a tree update?

I'll test this now…

::: toolkit/components/passwordmgr/content/passwordManager.js
@@ +46,4 @@
>  var signonsTreeView = {
>    _filterSet : [],
>    _lastSelectedRanges : [],
> +  _faviconMap: {},

We use real `Map`s nowadays which has a nicer API for this (e.g. no hasOwnProperty): new Map(),

In this case I think you just need a `Set` though as you just need to keep track of whether you have requested the login favicon URL, we don't need to store the value.

@@ +96,5 @@
>    getCellProperties : function(row,column) {
> +    if (column.element.getAttribute("id") == "siteCol") {
> +      const signon = this._filterSet.length ? this._filterSet[row] : signons[row];
> +
> +      if (!signonsTreeView._faviconMap.hasOwnProperty(signon.hostname)) {

Please prefer the pattern of using early returns rather than nesting `if`:

let props = ["ltr"];
if (column.element.getAttribute("id") != "siteCol") {
  return props.join(" ");
}

if (this._faviconMap.has(signon.hostname)) {
  return props.join(" ");
}

// Fetch favicons for origins we haven't fetched for yet
…
this._faviconMap.set(hostname, faviconURL);

props.push(…)
…
return props.join(" ");

@@ +97,5 @@
> +    if (column.element.getAttribute("id") == "siteCol") {
> +      const signon = this._filterSet.length ? this._filterSet[row] : signons[row];
> +
> +      if (!signonsTreeView._faviconMap.hasOwnProperty(signon.hostname)) {
> +        const { PlacesUtils } = Cu.import("resource://gre/modules/PlacesUtils.jsm");

May as well put this at the top of the file but as a lazyModuleGetter:
XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                  "resource://gre/modules/PlacesUtils.jsm");

@@ +107,5 @@
> +            treechildren::-moz-tree-image(${CSS.escape(hostname)}) {
> +              list-style-image: url("${favicon}");
> +              width: 16px;
> +              height: 16px;
> +              margin-right: 5px;

Only the list-style-image property should be added dynamically, the rest should be inherited from the CSS file.

@@ +113,5 @@
> +          `, styleSheet.cssRules.length);
> +
> +          if (favicon.startsWith("chrome://")) {
> +            styleSheet.insertRule(`
> +              @media (min-resolution: 1.1dppx) {

This should just be added to passwordmgr.css as the 2x fallback image

@@ +126,5 @@
> +        }
> +
> +        PlacesUtils.promiseFaviconLinkUrl(signon.hostname)
> +          .then(function(favicon) {
> +            _insertRule(signon.hostname, favicon.path.replace(/^favicon:/, ""));

Do you actually need this .replace()? I think it should work fine without that e.g.:
moz-anno:favicon:https://mozorg.cdn.mozilla.net/media/img/favicon/favicon-196x196.cb79ae3b6daf.png

@@ +128,5 @@
> +        PlacesUtils.promiseFaviconLinkUrl(signon.hostname)
> +          .then(function(favicon) {
> +            _insertRule(signon.hostname, favicon.path.replace(/^favicon:/, ""));
> +          }, function() {
> +            _insertRule(signon.hostname, "chrome://mozapps/skin/places/defaultFavicon.png");

This should be made the default fallback in passwordmgr.css. Don't insert a specific rule for sites that don't have a favicon.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8741271 [details] [diff] [review]
0002-Bug-530999-Show-site-favicons-in-login-manager-UI.patch

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

(In reply to Joseph Yeh [:joseph] from comment #8)
> 3. It looks like the tree still didn't update at the first place. How should
> I trigger a tree update?

The following seems to work:

          signonsTree.treeBoxObject.invalidate();
          signonsTree.treeBoxObject.clearStyleAndImageCaches();

In my testing invalidateRow or invalidateColumn didn't seem to work until I hovered or clicked on the tree.

To avoid too much flickering it may be good to use a DeferredTask perhaps with a 10ms delay to batch up the invalidations: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/DeferredTask.jsm

::: toolkit/components/passwordmgr/content/passwordManager.js
@@ +107,5 @@
> +            treechildren::-moz-tree-image(${CSS.escape(hostname)}) {
> +              list-style-image: url("${favicon}");
> +              width: 16px;
> +              height: 16px;
> +              margin-right: 5px;

You also should avoid *-right properties and use -moz-*-end instead to automatically handle RTL layouts. In this case I believe you want -moz-margin-end: 5px;

Comment 11

[Joseph Yeh [:joseph]]

Attachment: MozReview Request: Bug 530999 - Show site favicons in login manager UI; r?MattN

Review commit: https://reviewboard.mozilla.org/r/47087/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47087/

Comment 12

[Joseph Yeh [:joseph]]

Hi Matthew,

Thanks for your last feedback, they are very helpful :)

I would like to use MozReview for reviewing this patch as I haven't try it before.
Please feel free to correct me if I did something wrong.

Thank you very much for your help!

Comment 13

[Matthew N. [:MattN]]

We should get QE verification to make sure the flickering of the login list isn't a problem on some OS.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8742226 [details]
MozReview Request: Bug 530999 - Show site favicons in login manager UI; r?MattN

https://reviewboard.mozilla.org/r/47087/#review43973

Great job, we're very close now. I'm excited for this to land! I'll take one more look at it with the suggested changes:

::: toolkit/components/passwordmgr/content/passwordManager.js:16
(Diff revision 1)
> +                                  "resource://gre/modules/PlacesUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "DeferredTask",
> +                                  "resource://gre/modules/DeferredTask.jsm");
> +
> +XPCOMUtils.defineLazyGetter(this, "styleSheet", function() {

Nit: "faviconStyleSheet" seems better since there is more than one we could be talking about.

::: toolkit/components/passwordmgr/content/passwordManager.js:66
(Diff revision 1)
>    _filterSet : [],
>    _lastSelectedRanges : [],
> +  _faviconMap: new Set(),

Nit: Keep these sorted alphabetically

::: toolkit/components/passwordmgr/content/passwordManager.js:68
(Diff revision 1)
>  }
>  
>  var signonsTreeView = {
>    _filterSet : [],
>    _lastSelectedRanges : [],
> +  _faviconMap: new Set(),

`_faviconMap` is no longer a Map so should be renamed :) `_faviconSet` also isn't that descriptive but I'm not sure of a better name so can you add a JSDoc comment above it to explain what it's for:

/**
 * Keep track of which favicons we've appended to the favicon stylesheet.
 */

::: toolkit/components/passwordmgr/content/passwordManager.js:137
(Diff revision 1)
> +        if (!signonsTreeView._invalidate) {
> +          signonsTreeView._invalidate = new DeferredTask(function() {
> +            signonsTree.treeBoxObject.invalidate();
> +            signonsTree.treeBoxObject.clearStyleAndImageCaches();
> +          }, 10);
> +        }

Please define this near the top of `signonsTreeView` instead of mixing it into the logic of this function.

Then:
* use `this` instead of `signonsTreeView`
* Rename the member to `_invalidateTask` to make it more clear what it is
* just call .arm() in this callback
* Add a comment about why we're using a DeferredTask e.g. "Coalesce invalidations to avoid repeated flickering."

::: toolkit/components/passwordmgr/content/passwordManager.js:144
(Diff revision 1)
> +        signonsTreeView._invalidate.arm();
> +      }, function() {
> +        signonsTreeView._faviconMap.add(signon.hostname);

Use `this` instead of `signonsTreeView` by switching the callback to a fat arrow function: `favicon => {…`

::: toolkit/components/passwordmgr/content/passwordManager.js:146
(Diff revision 1)
> +          }, 10);
> +        }
> +
> +        signonsTreeView._invalidate.arm();
> +      }, function() {
> +        signonsTreeView._faviconMap.add(signon.hostname);

The main point of the set is to avoid doing multiple promiseFaviconLinkUrl requests (and therefore multiple CSS rules) for the same origin so this .add needs to be done synchronously in getCellProperties (e.g. right before promiseFaviconLinkUrl).

::: toolkit/themes/shared/passwordmgr.css:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

`hg move`

::: toolkit/themes/shared/passwordmgr.css:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Nit: Can you represent this as an `hg move` from the Windows file to preserve VCS history? I think you can use `hg move --after` after the fact.

Comment 15

[Joseph Yeh [:joseph]]

Comment on attachment 8742226 [details]
MozReview Request: Bug 530999 - Show site favicons in login manager UI; r?MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47087/diff/1-2/

Comment 16

[Joseph Yeh [:joseph]]

https://reviewboard.mozilla.org/r/47087/#review43973

> The main point of the set is to avoid doing multiple promiseFaviconLinkUrl requests (and therefore multiple CSS rules) for the same origin so this .add needs to be done synchronously in getCellProperties (e.g. right before promiseFaviconLinkUrl).

I move the .add before promiseFaviconLinkUrl and so there is nothing to do in the reject function of promiseFaviconLinkUrl. Should I keep an empty function here or just remove it and let it throw an error in the console when the favicon is not found? (I do the latter in the updated patch)

> Nit: Can you represent this as an `hg move` from the Windows file to preserve VCS history? I think you can use `hg move --after` after the fact.

I am using Git (with git-cinnabar) right now as my VCS tools. Do you know whether this can be done with Git?

Comment 17

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/47087/#review43973

> I move the .add before promiseFaviconLinkUrl and so there is nothing to do in the reject function of promiseFaviconLinkUrl. Should I keep an empty function here or just remove it and let it throw an error in the console when the favicon is not found? (I do the latter in the updated patch)

Removing it (like you did) is fine.

> I am using Git (with git-cinnabar) right now as my VCS tools. Do you know whether this can be done with Git?

I'm not sure about that. I think git does its own move detection automatically so I don't know how that works with git-cinnabar.

I'll do this now before I push it.

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8742226 [details]
MozReview Request: Bug 530999 - Show site favicons in login manager UI; r?MattN

https://reviewboard.mozilla.org/r/47087/#review44291

::: toolkit/components/passwordmgr/content/passwordManager.js:137
(Diff revision 2)
> +        faviconStyleSheet.insertRule(`
> +          treechildren::-moz-tree-image(${CSS.escape(signon.hostname)}) {
> +            list-style-image: url("moz-anno:${favicon.path}");

I realized that this is introducing a potential security issue as we may not be able to fully trust the URL e.g. if it contains `[ ")\]` etc. A malicious favicon URL could insert other CSS rules as-is. Since this was my fault for telling you to go in this direction I ended up going back to using getImageSrc myself and used the Map to cache values. This also allowed me to do a smaller invalidation which should be good for reducing flicker. Take a look at the change I land to see what I did. Thanks for your patience!

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f9fc0c367d89

Comment 20

[Joseph Yeh [:joseph]]

https://reviewboard.mozilla.org/r/47087/#review44291

> I realized that this is introducing a potential security issue as we may not be able to fully trust the URL e.g. if it contains `[ ")\]` etc. A malicious favicon URL could insert other CSS rules as-is. Since this was my fault for telling you to go in this direction I ended up going back to using getImageSrc myself and used the Map to cache values. This also allowed me to do a smaller invalidation which should be good for reducing flicker. Take a look at the change I land to see what I did. Thanks for your patience!

Great! Thanks for your mentoring :)

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 22

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I managed to test this bug on 
- 50.0a1 (2016-06-12)
- 49.0a2 (2016-06-13)
- 48.0b1 build 2 (20160606200529)
using 
- Windows 10 x64
- Mac OS X 10.11
- Ubuntu 14.04 x86
The fix has landed and it works as expected, so I am marking this issue as Verified Fixed.