Closed Bug 419298 Opened 16 years ago Closed 13 years ago

Dialog icons should be displayed in the native OS theme size without scaling

Categories

(Toolkit :: Themes, defect)

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla2.0b7

People

(Reporter: wgianopoulos, Assigned: dao)

References

Details

Attachments

(7 files, 7 obsolete files)

The lock icon looks much better under beta 3 than with current trunk builds.
This is actually a regression from bug 381661.

This was verified via backout.
Blocks: 381661
Component: Password Manager → GFX: Thebes
Keywords: regression
Product: Firefox → Core
QA Contact: password.manager → thebes
Attached image screenshot of image before 381661 (obsolete) —
Attached image screenshot of image with 381661 (obsolete) —
Flags: blocking1.9?
Attached patch workaround (obsolete) — Splinter Review
Reverting this part of the code from bug 381661 is sufficient to avoid the issue.
Attached patch patch version 1 (obsolete) — Splinter Review
How about this?
Assignee: nobody → bill
Status: NEW → ASSIGNED
Attachment #305357 - Flags: review?(vladimir)
Er, can you fix the lock icon usage so that it's not being scaled?  It shouldn't be getting scaled in the first place.
(In reply to comment #6)
> Er, can you fix the lock icon usage so that it's not being scaled?  It
> shouldn't be getting scaled in the first place.
> 
That is what I thought originally.  It would make sense if this were an icon that is part of the gnomestripe theme.  Just crate an icon of the correct size instead of trying to scale it.  Unfortunately, this is one of the cases where it is using an icon from the native OS theme so is kind of stuck with the size of the icon provided in the OS theme.
In any event, my patch here permits bilinear scaling under windows without altering the previous behavior under Linux.  It also makes the behavior under Linux match the comments.

The comments say "This effectively disables smooth upscaling for images.", when the post 381661 code disables smooth scaling in all cases, up and down.
So, the dialog is using the stock icon "gtk-dialog-authentication"? How big is the target image supposed to be? Not 48x48?
(In reply to comment #9)
> So, the dialog is using the stock icon "gtk-dialog-authentication"? How big is
> the target image supposed to be? Not 48x48?
> 

I hope you are not expecting an answer from me on this.  I really have no idea where this code even is.  I only figured out it is using a stock icon because:

1.  I could not find the icon being used in the gnomestripe icons

2.  So, I tried changing my OS theme form Clearlooks to Crux to prove my assumption and a completely different icon was then displayed.
(In reply to comment #10)
> I hope you are not expecting an answer from me on this.  I really have no idea
> where this code even is.  
;)

> 2.  So, I tried changing my OS theme form Clearlooks to Crux to prove my
> assumption and a completely different icon was then displayed.
Hmm... clearlooks is a gtk widget theme (the meta theme uses the default gnome icon theme) and the crux icon theme does not provide this icon. The icon you use seems to be the redhat bluecurve icon?

Anyway, I tested this here and the icon seems to be scaled to a very strange size... 37px or something like that. For dialogs like this we should always use 48x48. Actually I think firefox/windows does the same so I don't see the point of having this dialog being different.

Yes it is using Bluecurve icons.
I ended up making the icons at 40px because some themes have their dialog icons at 32px.
Attachment #305354 - Attachment is obsolete: true
Perhaps in cases where the native theme icons are not universally available across themes we should just not try to use native icons so we don't run into these issues.  I think scaling chrome icons is just a bad idea.
The problem is part us, part them. It kind of sucks that the size is up to them, but in the dialog, without explicit CSS height declarations, the image scales with the height of the paragraph of text.
Ugh. I think I've avoided XUL image problems like this in the past by sprinkling <spacer flex="1"/> around the images to eat up the extra space without hard-coding an image size. In the worst case, this becomes:

<vbox>
  <hbox>
    <image/>
    <spacer flex="1"/>
  </hbox>
  <spacer flex="1"/>
<vbox>

Not sure if there's a cleaner way.
(In reply to comment #13)
> I ended up making the icons at 40px because some themes have their dialog icons
> at 32px.
> 

Some themes? GTK only ships 48x48 and freedesktop icon thems ship 16x16, 22x22, 24x24, 32x32 and 48x48/scalable. On Linux, no theme should only ship them in 32x32 (or it is broken by design).
That's irrelevant. For some reason, the icon dimensions are being overridden:
http://mxr.mozilla.org/mozilla/source/toolkit/themes/gnomestripe/global/global.css#93
(In reply to comment #18)
> That's irrelevant. For some reason, the icon dimensions are being overridden:
> http://mxr.mozilla.org/mozilla/source/toolkit/themes/gnomestripe/global/global.css#93

Bug 402839, comment #7 states: "We need to specify those [width and height] because without them the image height flexes with the text of the dialog, and we can't have that."
(In reply to comment #17)
> (In reply to comment #13)
> > I ended up making the icons at 40px because some themes have their dialog icons
> > at 32px.
> > 
> 
> Some themes? GTK only ships 48x48 and freedesktop icon thems ship 16x16, 22x22,
> 24x24, 32x32 and 48x48/scalable. On Linux, no theme should only ship them in
> 32x32 (or it is broken by design).
> 

Redmond and GTK-QT give you 32x32 icons when you request size=dialog, so I imagine more themes do too.
(In reply to comment #20)
> Redmond and GTK-QT give you 32x32 icons when you request size=dialog, so I
> imagine more themes do too.

Sorry, I was speaking of *icon* themes. Sure GTK themes are free to re-define the size constants (something most avoid because many apps hard-code the size in numbers...).

Forcing the icon to be 40x40 is a very bad idea because no theme will ever ship that size, so it will look broken for everyone. Better make it 32x32, which at least in theory every icon theme should ship.
(In reply to comment #19)
>Bug 402839, comment #7 states: "We need to specify those [width and height]
>because without them the image height flexes with the text of the dialog
Ah, we size the dialog incorrectly because we don't know the image size yet.

Perhaps we could set a max-size on the image, and a min-size on the hbox?
The easiest thing here would be to put in a 48x48 block element (e.g. <div> or <hbox> or something with fixed height/width), and use the image as a non-repeating background; that would ensure that there would be no resizing involved no matter what size is used.  If it happens to be greater than 48x48, then we'd get clipping, but things shouldn't be > 48x48.
Shouldn't we still fix the underlying scaling bug?  Or is there no underlying bug?
I don't think there's an underlying bug -- just the type of scaling that was in effect.  1.8 didn't do smooth scaling on linux either (for either upscaling or downscaling); up until my change, 1.9 was doing smooth downscaling which was hiding this bug.  I disabled both smooth upscaling and downscaling on Linux -- upscaling due to correctness issues with the X server (EXTEND_PAD not implemented), and downscaling for consistency and potential correctness issues.

We can certainly turn smooth downscaling back on (which is what Bill's patch does), but we should fix the icon issue.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
The earlier downscaling effect looked much nicer for any image, so we should definitely turn it back on.
Attached patch Patch 2 (obsolete) — Splinter Review
Huh, no idea what got fixed elsewhere, but removing this CSS no longer shows the problem, so we should do that to get back native sizes.

We must also turn back on smooth downscaling for the sake of clean image rendering everywhere, not just this icon. A lot of images that are downscaled are unreadable in Firefox 2 because the author only tests in Firefox 3 with smooth downscaling, looking at you Ryan Paul of Arstechnica ;)
Attachment #305994 - Flags: review?(vladimir)
...plus the smooth downscaling produces much, much better results (obviously) and with no downsides.
Attached image screenshot with Patch 2 (obsolete) —
With patch 2 applied, this is what my master password dialog box looks like.

So, this is not quite right.
> and with no downsides.

Other than serious performance suck, of course... ;)
Do we need a separate bug on reenabling downscaling?  If so, can someone please get that filed?
I will do that and post the bug number back here.
Assignee: bill → nobody
Status: ASSIGNED → NEW
Component: GFX: Thebes → Theme
Flags: blocking1.9+
Product: Core → Firefox
QA Contact: thebes → theme
Attachment #305357 - Attachment is obsolete: true
Attachment #305357 - Flags: review?(vladimir)
bug 419927 filed.
Changing summary as the summary will be incorrect once the code for bug 419927 is approved and checked-in.
Summary: [gnomestripe] appearance of lock icon on master password dialog has regressed since beta3 → [gnomestripe] dialog icons should be displayed in the native OS theme size without scaling
Attachment #305994 - Attachment is obsolete: true
Attachment #305994 - Flags: review?(vladimir)
No longer blocks: 381661
Keywords: regression
(In reply to comment #15)
> The problem is part us, part them. It kind of sucks that the size is up to
> them, but in the dialog, without explicit CSS height declarations, the image
> scales with the height of the paragraph of text.

Can somebody explain this further? I was trying to figure out why icons in Oxygen theme (used with Oxygen-Molecule theme applied to gtk apps in kde) look terrible and after removing width and height rule and changing paragraph line-height it still didn't scale and looked perfectly fine. Default size for these icons is 32x32 with that theme and they look awful when scaled up.
If they are being scaled to a larger size, the issue could be Bug 422179.
Component: Theme → Themes
Priority: P3 → --
Product: Firefox → Toolkit
QA Contact: theme → themes
Summary: [gnomestripe] dialog icons should be displayed in the native OS theme size without scaling → Dialog icons should be displayed in the native OS theme size without scaling
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → dao
Attachment #305349 - Attachment is obsolete: true
Attachment #305350 - Attachment is obsolete: true
Attachment #306001 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #472299 - Flags: review?(neil)
Comment on attachment 472299 [details] [diff] [review]
patch

>+#iconContainer {
>+  -moz-box-pack: center;
[Tricky one, this... I'm worried about the password dialog in particular.]

>+  height: 55px; /* maximum icon height + padding */
>+  width: 58px; /* maximum icon width + padding */
>+  padding: 3px 5px 4px;
>+}
>+
>+#info\.icon {
>+  margin: 0 !important; /* overriding the 'spaced' class */
What was the reasoning behind the conversion of margin to padding?
(In reply to comment #40)
> >+  height: 55px; /* maximum icon height + padding */
> >+  width: 58px; /* maximum icon width + padding */
> >+  padding: 3px 5px 4px;
> >+}
> >+
> >+#info\.icon {
> >+  margin: 0 !important; /* overriding the 'spaced' class */
> What was the reasoning behind the conversion of margin to padding?

I didn't like the dependency of the height and width values on margin values buried somewhere in global.css...
Comment on attachment 472299 [details] [diff] [review]
patch

>+#iconContainer {
>+  -moz-box-pack: center;
Do you have a screenshot of a native Gnome authentication dialog showing that the icon should not be at the top left? (The allowance for 48px icons has no effect in this case as the box is already 72px wide.)

>+  height: 55px; /* maximum icon height + padding */
>+  width: 58px; /* maximum icon width + padding */
These should be minimum dimensions. Other constraints may want the size of this box to exceed the stated dimensions. (It lives in a grid, remember?)

>+  padding: 3px 5px 4px;
>+}
>+
>+#info\.icon {
>+  margin: 0 !important; /* overriding the 'spaced' class */
Rather than using padding, possibly duplicate the 'spaced' margin here, with a comment explaining why?
(In reply to comment #42)
> >+#iconContainer {
> >+  -moz-box-pack: center;
> Do you have a screenshot of a native Gnome authentication dialog showing that
> the icon should not be at the top left? (The allowance for 48px icons has no
> effect in this case as the box is already 72px wide.)

top left doesn't look right to me... Epiphany's HTTP auth dialog is different in that it doesn't have the labels under the icon, so it doesn't help much.
Comment on attachment 472299 [details] [diff] [review]
patch

(In reply to comment #43)
>top left doesn't look right to me...
Sorry, but that's insufficient for my r+, even with the other fixes.
Attachment #472299 - Flags: review?(neil) → review-
Well, it looks misaligned, what else can I say?
Attached patch patch v2Splinter Review
other comments addressed
Attachment #472299 - Attachment is obsolete: true
Attached image HTTP auth / centered
Attached image HTTP auth / Epiphany
Attachment #472387 - Flags: ui-review?(faaborg)
(In reply to comment #47)
> Created attachment 472387 [details]
> HTTP auth / centered
Maybe you should try right aligned too just for completeness ;-)

[I don't expect to see much difference in the master password dialog.]
Attachment #472387 - Flags: ui-review?(faaborg) → ui-review+
Comment on attachment 472383 [details] [diff] [review]
patch v2

>+#info\.icon {
>+  max-width: 48px;
>+  max-height: 48px;
>+  /* The 'spaced' class does this already. It's repeated here to make it clearer
>+     what margin values the #iconContainer's min-height and max-height depend on: */
>+  margin: 3px 5px 4px !important;
The !important seems unnecessary as a # rule is already more specific than a .
Attachment #472383 - Flags: review+
Attached patch patch v3Splinter Review
!important dropped
Attachment #472563 - Flags: approval2.0?
Attachment #472563 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/693b2a27975a
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
32px icons are correctly displayed as 32px in Mozilla/5.0 (X11; Linux i686; rv:2.0b7pre) Gecko/20100916 Firefox/4.0b7pre.
Status: RESOLVED → VERIFIED
Depends on: 625814
You need to log in before you can comment on or make changes to this bug.