The default bug view has changed. See this FAQ.

Linux Theme should use moz-icon://stock/gtk-file for blank documents

RESOLVED FIXED in Firefox 4.0b10

Status

()

Firefox
Theme
--
enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: pascalc, Assigned: pascalc)

Tracking

Trunk
Firefox 4.0b10
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 499462 [details]
before/after screenshot

The Linux theme uses a png for blank document (chrome://global/skin/icons/folder-item.png) that looks a bit old and not very crisp, IMO it should be replaced by the standard gtk blank document which conveys better the concept of "blank, default" than the current icon and looks both more modern and less 'busy'.

The stock gtk icon is also almost identical to what we use for Windows XP and it is the icon used by Chrome on Linux.

I am attaching a screenshot before/after of this proposal.

Updated

6 years ago
Severity: normal → enhancement
Whiteboard: [good first bug]
(Assignee)

Comment 1

6 years ago
Created attachment 500195 [details] [diff] [review]
patch

Here is a patch fixing this issue, there is one context in which the older icon is used, panorama mode on the top left of thunbnails, but I couldn't find where this was defined.
Attachment #500195 - Flags: review?(dao)
Comment on attachment 500195 [details] [diff] [review]
patch

This makes folder-item.png unused in gnomestripe, doesn't it? In that case that file should be removed.

We should probably use size=menu in these cases at least:

> /* Bookmark items */
> .bookmark-item:not([container])  {
>-  list-style-image: url("chrome://global/skin/icons/folder-item.png");
>-  -moz-image-region: rect(0px, 16px, 16px, 0px)
>+  list-style-image: url("moz-icon://stock/gtk-file?size=toolbarsmall");
> }

> /* All tabs menupopup */
> .alltabs-item > .menu-iconic-left > .menu-iconic-icon {
>-  list-style-image: url("chrome://global/skin/icons/folder-item.png");
>-  -moz-image-region: rect(0px, 16px, 16px, 0px);
>+  list-style-image: url("moz-icon://stock/gtk-file?size=toolbarsmall");
> }
(In reply to comment #1)
> Created attachment 500195 [details] [diff] [review]
> patch
> 
> Here is a patch fixing this issue, there is one context in which the older icon
> is used, panorama mode on the top left of thunbnails, but I couldn't find where
> this was defined.

This is chrome://mozapps/skin/places/defaultFavicon.png, as defined in nsIFaviconService.idl. That file is used in gnomestripe/browser/places/places.css and gnomestripe/browser/aboutSessionRestore.css as well.
(Assignee)

Comment 4

6 years ago
(In reply to comment #2)
> Comment on attachment 500195 [details] [diff] [review]
> patch
> 
> This makes folder-item.png unused in gnomestripe, doesn't it? In that case that
> file should be removed.

The file is in hg/src/toolkit/themes/gnomestripe/global/icons 

is it ok to remove that icon that is in toolkit/ and not in browser/ ? Maybe it will impact applications such as Thunderbird or Seamonkey?

> 
> We should probably use size=menu in these cases at least:
> 

Is there any documentation somewhere about what parameters exists and how to use them? I couldn't find anything in MDC and Google, therefore I am mimicking what I see in other files.
(In reply to comment #4)
> The file is in hg/src/toolkit/themes/gnomestripe/global/icons 
> 
> is it ok to remove that icon that is in toolkit/ and not in browser/ ?

Yes.

> Maybe it will impact applications such as Thunderbird or Seamonkey?

Maybe. If so, they should start using the stock icon as well or, if there's a reason not to do so, package folder-item.png themselves.

> Is there any documentation somewhere about what parameters exists and how to
> use them? I couldn't find anything in MDC and Google, therefore I am mimicking
> what I see in other files.

nsIIconURI.idl lists the parameters:
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/decoders/icon/nsIIconURI.idl#63
(Assignee)

Comment 6

6 years ago
The default icon in panorama mode is actually used as an inline <img> here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/groupitems.js#1019

and is defined here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/modules/utils.jsm#480

I didn't find a syntax that would allow me to define the use of a stock gtk icon instead of an image  file in the jar manifest file, the manifest file currently has:

+ skin/classic/mozapps/places/defaultFavicon.png  (places/defaultFavicon.png)

(replacing the relative url places/defaultFavicon.png  by moz-icon://stock/gtk-file?size=menu does not work)
It can be done like this:

   defaultFaviconURL:
#ifdef MOZ_WIDGET_GTK2
     "moz-icon://stock/gtk-file?size=toolbarsmall",
#else
     "chrome://mozapps/skin/places/defaultFavicon.png",
#endif

However, I don't think utils.jsm is currently being preprocessed... So that would need to be changed first, or you could spin it off to a separate bug.
(Assignee)

Comment 8

6 years ago
I think that I will deal with the general case in this bug and the panorama one in a separate bug.
(Assignee)

Comment 9

6 years ago
Created attachment 500801 [details] [diff] [review]
updated patvch

Here is a new patch adressing the following issues:
- replaces all instances of the png icon by the stock gtk icon, sets opacity for disabled state when needed
- removes the now unused png icon and removes it from the jar manifest
- adds a #ifdef in nsIFaviconService.idl to use the stock icon as well
- adresses Dao's comment about the use of the menu attribute

This patch does not address the panorama case since no #ifdef can be used in this file yet, I will file a separate bug for that.

I compiled locally and everything works for me.
Assignee: nobody → pascalc
Attachment #500195 - Attachment is obsolete: true
Attachment #500801 - Flags: review?(dao)
Attachment #500195 - Flags: review?(dao)
Andreas if this lands we'll need something for TB.
Comment on attachment 500801 [details] [diff] [review]
updated patvch

>--- a/toolkit/components/places/public/nsIFaviconService.idl
>+++ b/toolkit/components/places/public/nsIFaviconService.idl
>@@ -357,7 +357,15 @@
>  * Notification sent when all favicons are expired.
>  */
> #define NS_PLACES_FAVICONS_EXPIRED_TOPIC_ID "places-favicons-expired"
>+
>+#ifdef MOZ_WIDGET_GTK2
>+#define FAVICON_DEFAULT_URL "moz-icon://stock/gtk-file?size=toolbarsmall"
>+#else
> #define FAVICON_DEFAULT_URL "chrome://mozapps/skin/places/defaultFavicon.png"
>+#endif
>+
> #define FAVICON_ERRORPAGE_URL "chrome://global/skin/icons/warning-16.png"
> 
> %}
>+
>+

nit: don't add these two lines at the bottom
Attachment #500801 - Flags: review?(dao) → review+
(Assignee)

Comment 12

6 years ago
Created attachment 500824 [details] [diff] [review]
updated patch (removed 2 empty lines)

here is the same patch without the 2 extra line to the file
Attachment #500801 - Attachment is obsolete: true
Attachment #500824 - Flags: review+
Hardware: x86 → All
(Assignee)

Updated

6 years ago
Attachment #500824 - Flags: approval2.0?
Status: NEW → ASSIGNED
If I'm reading this correctly, then when on GTK2:

- default favicon will be defined to use moz-icon://
- panorama code will get that value from the favicon service when asking what favicon to show for a tab when in panorama view (for tabs representing documents which don't define their own favicons)
- panorama code will set that value as explicit inline img src attribute
- pure CSS will be unable to apply a favicon because CSS list-style-image doesn't apply when img src is set
- when using an em:type=4 theme, the icon will come from the desktop rather than (unlike before this patch) from the theme
- leaving a high potential for mismatch between the style of the icon and the rest of the UI in that case
- and, the same thing will be true for all favicons gotten from the favicon service when on GTK2, i.e., for main browser tabs

Is that right?  And if so, could this be done in a way that preserves the current skinnability?  Like, if the favicon service tells you to fall back to the default favicon, then use in-theme CSS (chrome://browser/skin/tabview/tabview.css) to set it, allowing third-party themes to set the favicons they need?
Status: ASSIGNED → NEW
Hardware: All → x86
(Assignee)

Comment 14

6 years ago
@mcdavis
The patch in this bug does not affect panorama, I will file a separate bug to use the GTK default icon in panorama (and only the default icon, nothing else).

The moz-icon:// protocol allows setting a specific size in pixels (?size=16), not only proportional ones.
That bit I said about main browser tabs is wrong.  When there's no favicon, theme rules apply.
Comment on attachment 500824 [details] [diff] [review]
updated patch (removed 2 empty lines)

a=beltzner
Attachment #500824 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/22ea9b958dc7

I had to revert part of this (http://hg.mozilla.org/mozilla-central/rev/b521854c006d) because of test failures:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1295341987.1295343337.28935.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1295342114.1295343130.27623.gz
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
You need to log in before you can comment on or make changes to this bug.