1395732 - Replace Globe Icon G L O B A L L Y

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P2)

Description

[bbell]

Attachment: example.png

Replace the Existing Globe icon with a new "Photon" one.

Comment 1

[bbell]

Attachment: globe-16.svg

Icon required

Comment 2

[Dão Gottwald [:dao]]

This was just replaced recently in bug 1360891...

Comment 3

[Marco Bonardo [:mak]]

Also, I must note that bug 1360891 introduced an icon with a light border that works nicely on both dark and light backgrounds, and we should retain that, or we'll have regressions.

Unless there's a very compelling reason to replace the icon again, I'd ask to reconsider.

Comment 4

[Marco Bonardo [:mak]]

fwiw, I also think we may keep the bug in Theme, the Places part to allow having an SVG icon in bug 1360891 has been done, and we just need to replace the svg at this point at toolkit/themes/shared/places/defaultFavicon.svg

Comment 5

[u428464]

(In reply to Marco Bonardo [::mak] from comment #3)
> Also, I must note that bug 1360891 introduced an icon with a light border
> that works nicely on both dark and light backgrounds, and we should retain
> that, or we'll have regressions.
> 
> Unless there's a very compelling reason to replace the icon again, I'd ask
> to reconsider.

FWIW the inverted version of the new globe looks a lot better that the current light-bordered one.

Comment 6

[Marco Bonardo [:mak]]

The problem with inverted icon here is that we'll have to give a fill to all the svgs in places views, and if a remote svg uses fill, it will also pick up what we pass.
In short, the frontend doesn't know if it's showing the default favicon or a remote favicon, so it cannot tell when to invert the icon.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Stephen and Bryan, can you agree on the approach we should take here regarding Marco's comment 3 and comment 6?

Comment 8

[Marco Bonardo [:mak]]

Let me clarify my concern a last time.
The default favicon is shown in place of a missing remote favicon.
The UI has no way to know if what is showing is a remote favicon or the globe icon, thus we would have to provide a fill to every favicon in the ui, regardless of it being the globe or a remotely fetched favicon. While it's likely that most will be fine, we can't be sure.
The existing solution solves the problem with a tiny white border, that works more or less on any background.
I'm not going to blogk UX if you think this is of critical importance, but I'd like you to evaluate if we could somehow get the new icon maintaining the old approach.

Comment 9

[Marco Bonardo [:mak]]

Ok, after briefly discussing the thing with shorlander, I think we can proceed.
My concern can probably be answered by considering the remote icon itself broken in the cases where it may misbehave with our context-fill usage. The favicon owner will have to fix it in such a case, if he cares.

The only remaining concern is technical, context-fill works for chrome:// and resource://, I have no idea if it works for the page-icon protocol we use through the UI.
This can probably be verified quickly by replacing the current defaultFavicon.svg with the provided one and checking some page without an icon in the Address Bar or some other Places views.

We'll likely also need some css changes (for example we may want to restore the .ac-site-icon[selected] rule in autocomplete.css inverting the icon)

Comment 10

[Tim Nguyen :ntim]

(In reply to Marco Bonardo [::mak] from comment #9)
> Ok, after briefly discussing the thing with shorlander, I think we can
> proceed.
> My concern can probably be answered by considering the remote icon itself
> broken in the cases where it may misbehave with our context-fill usage. The
> favicon owner will have to fix it in such a case, if he cares.
> 
> The only remaining concern is technical, context-fill works for chrome://
> and resource://, I have no idea if it works for the page-icon protocol we
> use through the UI.

It would work if added here: https://dxr.mozilla.org/mozilla-central/rev/tip/layout/svg/SVGContextPaint.cpp#61
Though adding it here would expose an internal keyword (context-fill) to all webpage favicons.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Tim Nguyen :ntim from comment #10)
> Though adding it here would expose an internal keyword (context-fill) to all
> webpage favicons.

Right, the keyword would be exposed to remote favicons when they are shown in the UI. since page-icon: protocol is not exposed to content, there should be no other exposure. We may need to understand if there are unexpected consequences yet.

Comment 12

[Dão Gottwald [:dao]]

Attachment: Bug 1395732 - Implement new defaultFavicon.svg with context-fill.

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

Comment 13

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim from comment #10)
> (In reply to Marco Bonardo [::mak] from comment #9)
> > The only remaining concern is technical, context-fill works for chrome://
> > and resource://, I have no idea if it works for the page-icon protocol we
> > use through the UI.
> 
> It would work if added here:
> https://dxr.mozilla.org/mozilla-central/rev/tip/layout/svg/SVGContextPaint.cpp#61

This doesn't seem to work. Am I missing something?

Comment 14

[Dão Gottwald [:dao]]

D'oh, I was using an artifact build.

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 8944755 [details]
Bug 1395732 - Implement new defaultFavicon.svg with context-fill.

https://reviewboard.mozilla.org/r/214914/#review220796

::: layout/svg/SVGContextPaint.cpp:65
(Diff revision 1)
>    //
>    nsAutoCString scheme;
>    if (NS_SUCCEEDED(aURI->GetScheme(scheme)) &&
> -      (scheme.EqualsLiteral("chrome") || scheme.EqualsLiteral("resource"))) {
> +      (scheme.EqualsLiteral("chrome")
> +       || scheme.EqualsLiteral("resource")
> +       || scheme.EqualsLiteral("page-icon"))) {

It looks like this would allow any website to use context paint, so I think you should check that aURI is "page-icon:chrome://mozapps/skin/places/defaultFavicon.svg" if the scheme is "page-icon"...if that works. I'm not sure offhand how nested protocols look in an nsIURI.

Comment 16

[Dão Gottwald [:dao]]

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #15)
> It looks like this would allow any website to use context paint, so I think
> you should check that aURI is
> "page-icon:chrome://mozapps/skin/places/defaultFavicon.svg" if the scheme is
> "page-icon"...if that works. I'm not sure offhand how nested protocols look
> in an nsIURI.

That's not going to work as chrome://mozapps/skin/places/defaultFavicon.svg isn't part of the URI. page-icon:foo is used by history and bookmark items whereas foo is the _page_ URL, and then nsFaviconService picks defaultFavicon.svg if we don't have an icon stored.

Note that this only affects bookmark and history items, notably not the tab bar. I think we should just accept context paint capabilities leaking to the outside here. I don't know how else to address this.

Comment 17

[Jonathan Watt [:jwatt]]

Comment on attachment 8944755 [details]
Bug 1395732 - Implement new defaultFavicon.svg with context-fill.

https://reviewboard.mozilla.org/r/214914/#review220856

Okay, as long as this doesn't leak to the location bar I think that's an acceptable compromize. r=me on the SVGContextPaint.cpp changes.

Comment 18

[Jonathan Watt [:jwatt]]

However, could you add a new paragraph to the big comment above that section of SVGContextPaint noting that handling 'page-icon' does in principal leak context paint to page favicons, but only in history and bookmarks, not in the location bar. (And that we can't filter on defaultFavicon.svg, since the URL will be "page-icon:<site-url>".)

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8944755 [details]
Bug 1395732 - Implement new defaultFavicon.svg with context-fill.

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

Comment 20

[Dão Gottwald [:dao]]

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #18)
> However, could you add a new paragraph to the big comment above that section
> of SVGContextPaint noting that handling 'page-icon' does in principal leak
> context paint to page favicons, but only in history and bookmarks, not in
> the location bar. (And that we can't filter on defaultFavicon.svg, since the
> URL will be "page-icon:<site-url>".)

done

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 8944755 [details]
Bug 1395732 - Implement new defaultFavicon.svg with context-fill.

https://reviewboard.mozilla.org/r/214914/#review222742

Sorry if it took a while.
I found a couple issues:
1. with the dark theme, the back/forward popups are showing a white default favicon on grey, barely visible
2. The about:preferences homepage autocomplete popup shows dark favicons even when rows are selected (minor)

Since both are secondary UI, not blocking on them. But please either fix them or file follow-up bugs.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b394489fbb4
Implement new defaultFavicon.svg with context-fill. r=jwatt,mak

Comment 23

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6b394489fbb4

Comment 24

[Marco Bonardo [:mak]]

Dao, just to be sure you saw comment 21, since often it gets lost in bugmails.

Comment 25

[Mohammad Maruf Rahman]

I have reproduced this bug with  Nightly 57.0a1 (2017-08-31)on Windows 10, 64 Bit!

This Bug's fix is verified with latest Nightly!

Build ID   : 20180206220102
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0

Comment 26

[Dão Gottwald [:dao]]

Thanks.