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

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: MattN, Assigned: lina)

Tracking

unspecified
mozilla47
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

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.

Updated

3 years ago
Blocks: 1216585
No longer blocks: 1216585
(Assignee)

Updated

3 years ago
See Also: → bug 1224785
(Assignee)

Comment 1

3 years ago
Created attachment 8694489 [details]
MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws

Bug 1206560 - Show the site favicon in XUL notifications.
(Assignee)

Comment 2

3 years ago
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.
Attachment #8694489 - Flags: feedback?(MattN+bmo)
(Assignee)

Updated

3 years ago
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Depends on: 1227300
(Assignee)

Updated

3 years ago
Depends on: 1230212
(Assignee)

Comment 3

3 years ago
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/
Attachment #8694489 - Attachment description: MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. → MozReview Request: Bug 1206560, Part 1 - Implement alert favicons backend. r?MattN
Attachment #8694489 - Flags: feedback?(MattN+bmo) → review?(MattN+bmo)
(Assignee)

Comment 4

3 years ago
Created attachment 8696152 [details]
MozReview Request: Bug 1206560, Part 2 - Show the site favicon in XUL notifications.

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

Comment 5

3 years ago
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?
(Assignee)

Comment 6

3 years ago
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?
Attachment #8696152 - Flags: feedback?(jaws)
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?
Attachment #8696152 - Flags: feedback?(jaws) → feedback+
Attachment #8694489 - Flags: review?(MattN+bmo)
(Assignee)

Comment 8

3 years ago
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/
Attachment #8694489 - Attachment description: MozReview Request: Bug 1206560, Part 1 - Implement alert favicons backend. r?MattN → MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r?jaws
Attachment #8694489 - Flags: review?(jaws)
(Assignee)

Updated

3 years ago
Attachment #8696152 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 1224785
See Also: bug 1224785
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.
(Assignee)

Comment 10

3 years ago
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/
Attachment #8694489 - Flags: review?(jaws)
(Assignee)

Comment 11

3 years ago
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?
(Assignee)

Comment 12

3 years ago
Created attachment 8705889 [details]
With object-fit.png
(Assignee)

Comment 13

3 years ago
Created attachment 8705890 [details]
Without object-fit.png
(Assignee)

Comment 14

3 years ago
Created attachment 8705891 [details]
OS X native alerts.png
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.
Flags: needinfo?(kcambridge)
(Assignee)

Comment 16

3 years ago
Created attachment 8705950 [details]
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.
Attachment #8705890 - Attachment is obsolete: true
Flags: needinfo?(kcambridge)
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
Attachment #8694489 - Flags: review?(jaws) → review+
(Assignee)

Updated

3 years ago
Attachment #8694489 - Attachment description: MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r?jaws → MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws
(Assignee)

Comment 18

3 years ago
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/
(Assignee)

Comment 21

3 years ago
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?
Flags: needinfo?(jaws)
Paolo knows a bit more about mozIAsyncFavicons than I do, perhaps he knows?
Flags: needinfo?(jaws) → needinfo?(paolo.mozmail)
(Assignee)

Comment 23

3 years ago
Created attachment 8723181 [details]
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/
Attachment #8723181 - Flags: review?(jaws)
(Assignee)

Updated

3 years ago
Attachment #8694489 - Attachment is obsolete: true
(Assignee)

Comment 25

3 years ago
Created attachment 8723185 [details] [diff] [review]
interdiff.diff

Interdiff.
(Assignee)

Comment 26

3 years ago
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 on attachment 8723181 [details]
MozReview Request: Bug 1206560 - Show the site favicon in XUL notifications. r=jaws

https://reviewboard.mozilla.org/r/36389/#review32981
Attachment #8723181 - Flags: review?(jaws) → review+
Flags: needinfo?(paolo.mozmail)

Comment 30

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aa740a3f7dc9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Flags: qe-verify+
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?
Flags: needinfo?(kcambridge)
(Assignee)

Comment 32

2 years ago
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.
Flags: needinfo?(kcambridge)
You need to log in before you can comment on or make changes to this bug.