1206560 - Show the site favicon in XUL web notifications/alerts, when available

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Matthew N. [:MattN]]

The WIP design for bug 1201397 plans to show the site favicon beside the Notification title. If the alert is for a web property (not a chrome caller), we should fetch and display the favicon.

Comment 1

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws

Bug 1206560 - Show the site favicon in XUL notifications.

Comment 2

[Lina Butler [:lina]]

Comment on attachment 8694489 [details]
MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws

Here's a first cut. Depends on the refactoring in bug 1227300. Once the new interface lands, we can give bug 1224785 the same treatment.

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8694489 [details]
MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws

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

Comment 4

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1206560, Part 2 - Show the site favicon in XUL notifications.

Bug 1206560, Part 2 - Show the site favicon in XUL notifications.

Comment 5

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/26781/#review24675

::: toolkit/components/alerts/AlertNotification.js:55
(Diff revision 2)
> +  showWithFavicon(service, alertListener) {

This API is a bit odd, since the service needs to pass itself. I thought of having this take an `nsIFaviconDataCallback` instead, but that still requires the callback to be implemented in C++, along with `#ifdef MOZ_PLACES`. It's shy of four dozen lines in C++, compared to 10 in JS. WDYT?

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8696152 [details]
MozReview Request: Bug 1206560, Part 2 - Show the site favicon in XUL notifications.

Splitting out the design from the backend. I haven't updated the dimensions yet, but is the general approach right?

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

https://reviewboard.mozilla.org/r/27287/#review25333

::: toolkit/components/alerts/resources/content/alert.xul:28
(Diff revision 1)
> +      <hbox class="alertIconBox" align="center" pack="center">

Why does this need to be in a separate hbox? The #alertTitleBox is already an hbox, it seems that maybe this was done to get the correct alignment and packing? Can you try setting -moz-box-pack:center; and -moz-box-align:center; on #alertTitleBox instead?

::: toolkit/themes/shared/alert-common.css:62
(Diff revision 1)
> +  padding: 6px 0 6px 8px;

This should use padding-inline-start and padding-inline-end so it will not look out of position in RTL builds.

::: toolkit/themes/shared/alert-common.css:66
(Diff revision 1)
> +  width: 16px;

How does the notification look if a favicon is provided that is 16px wide but much taller than 16px?

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8694489 [details]
MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws

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

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8694489 [details]
MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws

https://reviewboard.mozilla.org/r/26781/#review26917

Holding off r+ until the questions below are answered.

::: toolkit/components/alerts/nsXULAlerts.cpp:91
(Diff revision 3)
> +nsXULAlerts::ShowAlertWithIconURI(nsIAlertNotification* aAlert,
> +                                  nsIObserver* aAlertListener,
> +                                  nsIURI* aURI)

Please rename aURI to aIconURI since it is not obvious that aURI is referring to the icon here.

::: toolkit/components/alerts/nsXULAlerts.cpp:249
(Diff revision 3)
> +  nsCOMPtr<nsISupportsInterfacePointer> iconURI = do_CreateInstance(NS_SUPPORTS_INTERFACE_POINTER_CONTRACTID, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  nsCOMPtr<nsISupports> supportsIconURI(do_QueryInterface(aURI));
> +  iconURI->SetData(supportsIconURI);
> +  iconURI->SetDataIID(&NS_GET_IID(nsIURI));
> +  rv = argsArray->AppendElement(iconURI);
> +  NS_ENSURE_SUCCESS(rv, rv);

Please pass the spec here instead of the full nsIURI. alert.js only uses the spec and I would prefer if we keep the API surface as small as possible.

::: toolkit/components/alerts/resources/content/alert.css:21
(Diff revision 3)
> +#alertTitleBox {
> +  -moz-box-pack: center;
> +  -moz-box-align: center;
> +}

Did you test that this doesn't center the title text?

::: toolkit/themes/shared/alert-common.css:61
(Diff revision 3)
> +  width: 16px;

If width is specified here, should height be specified also? If an icon is provided that is 16px tall and 2px wide, this will probably distort the icon. You should use `object-fit: scale-down;` here, as it will contain the image to the box size but not increase the size if it is intrinsically smaller than the box.

Comment 10

[Lina Butler [:lina]]

Comment on attachment 8694489 [details]
MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26781/diff/3-4/

Comment 11

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/26781/#review26917

> Please pass the spec here instead of the full nsIURI. alert.js only uses the spec and I would prefer if we keep the API surface as small as possible.

Sounds good.

> Did you test that this doesn't center the title text?

Sure did. The title text is not centered, and looks the same as before for pages without an icon.

> If width is specified here, should height be specified also? If an icon is provided that is 16px tall and 2px wide, this will probably distort the icon. You should use `object-fit: scale-down;` here, as it will contain the image to the box size but not increase the size if it is intrinsically smaller than the box.

I didn't use `object-fit: scale-down;` to keep the appearance consistent with the tabs bar, bookmarks toolbar, and bookmarks menu, which will distort excessively tall icons. It looks like OS X does the same. I think the scaled icon looks cleaner, albeit less recognizable compared to the others. I'll upload some screenshots to compare. WDYT?

Comment 12

[Lina Butler [:lina]]

Attachment: With object-fit.png

Comment 13

[Lina Butler [:lina]]

Attachment: Without object-fit.png

Comment 14

[Lina Butler [:lina]]

Attachment: OS X native alerts.png

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Is an image supplied for the notification in your two XUL screenshots? There is a large blank area where the image should be, I'm not sure if a transparent image is used in your screenshots or if there is a bug with the changes.

Comment 16

[Lina Butler [:lina]]

Attachment: Without object-fit.png

Oof, yes. I specified an image with an invalid path, so it returns a 404. I thought we had a bug on file for that, but I can't seem to find it. Filed bug 1238240. Here's a better screenshot, too.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8694489 [details]
MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws

https://reviewboard.mozilla.org/r/26781/#review26943

Thanks for fixing and providing those screenshots. Sorry for not thinking of this earlier, but can you please update the browser_notification_close.js test to check that when the icon is provided it is correctly referenced in the notification?

r+ with the test updated

Comment 18

[Lina Butler [:lina]]

Comment on attachment 8694489 [details]
MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26781/diff/4-5/

Comment 19

[Pulsebot]

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

Comment 20

[Pulsebot]

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

Comment 21

[Lina Butler [:lina]]

Hmm, I'm seeing intermittent failures for the icon test on Windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc0a80cc9333 I backed out for failing M-e10s(bc4), but it's reproducible on M(bc2).

I wonder if this is caused by a race, where we're showing the notification before the icon has been downloaded and saved to Places. Maybe `mozIAsyncFavicons.setAndFetchFaviconForPage` can help here? (Or `mozIAsyncFavicons.replaceFaviconData{FromDataURL}`).

Though I wonder if that could race with adding the visit to Places, and if we should unconditionally add a visit for the page first.

Does that sound right, :jaws?

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Paolo knows a bit more about mozIAsyncFavicons than I do, perhaps he knows?

Comment 23

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws

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

Comment 24

[Lina Butler [:lina]]

Let's see if this works: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1aeca4693300

Comment 25

[Lina Butler [:lina]]

Attachment: interdiff.diff

Interdiff.

Comment 26

[Lina Butler [:lina]]

Comment on attachment 8723181 [details]
MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws

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

Comment 27

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8723181 [details]
MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws

https://reviewboard.mozilla.org/r/36389/#review32981

Comment 28

[Lina Butler [:lina]]

That appears to have done the trick: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c489416501b

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa740a3f7dc9

Comment 30

[Carsten Book [:Tomcat]]

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

Comment 31

[Bogdan Maris, Desktop Test Engineering]

Can you please provide a way I can make those web notification show up in order to reproduce the issue on old Nightly build and then verify on latest builds?

Comment 32

[Lina Butler [:lina]]

Hi! We disabled favicons in web notifications in bug 1250288. You can flip the "alerts.showFavicons" pref in about:config, but it needs some UX attention (bug 1250285) before we can ship it.