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

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[Bill Gianopoulos [:WG9s]]

The lock icon looks much better under beta 3 than with current trunk builds.

Comment 1

[Bill Gianopoulos [:WG9s]]

This is actually a regression from bug 381661.

This was verified via backout.

Comment 2

[Bill Gianopoulos [:WG9s]]

Attachment: screenshot of image before 381661

Comment 3

[Bill Gianopoulos [:WG9s]]

Attachment: screenshot of image with 381661

Comment 4

[Bill Gianopoulos [:WG9s]]

Attachment: workaround

Reverting this part of the code from bug 381661 is sufficient to avoid the issue.

Comment 5

[Bill Gianopoulos [:WG9s]]

Attachment: patch version 1

How about this?

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 7

[Bill Gianopoulos [:WG9s]]

(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.

Comment 8

[Bill Gianopoulos [:WG9s]]

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.

Comment 9

[Michael Monreal [:micmon]]

So, the dialog is using the stock icon "gtk-dialog-authentication"? How big is the target image supposed to be? Not 48x48?

Comment 10

[Bill Gianopoulos [:WG9s]]

(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.

Comment 11

[Michael Monreal [:micmon]]

(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.

Comment 12

[Bill Gianopoulos [:WG9s]]

Yes it is using Bluecurve icons.

Comment 13

[Michael Ventnor]

I ended up making the icons at 40px because some themes have their dialog icons at 32px.

Comment 14

[Bill Gianopoulos [:WG9s]]

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.

Comment 15

[Michael Ventnor]

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.

Comment 16

[Justin Dolske [:Dolske]]

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.

Comment 17

[Michael Monreal [:micmon]]

(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).

Comment 18

[[e-mail]]

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

Comment 19

[Reed Loden [:reed]]

(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."

Comment 20

[Michael Ventnor]

(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.

Comment 21

[Michael Monreal [:micmon]]

(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.

Comment 22

[[e-mail]]

(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?

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Shouldn't we still fix the underlying scaling bug?  Or is there no underlying bug?

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 26

[Michael Ventnor]

The earlier downscaling effect looked much nicer for any image, so we should definitely turn it back on.

Comment 27

[Michael Ventnor]

Attachment: Patch 2

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 ;)

Comment 28

[Michael Ventnor]

...plus the smooth downscaling produces much, much better results (obviously) and with no downsides.

Comment 29

[Bill Gianopoulos [:WG9s]]

Attachment: screenshot with Patch 2

With patch 2 applied, this is what my master password dialog box looks like.

So, this is not quite right.

Comment 30

[Boris Zbarsky [:bzbarsky]]

> and with no downsides.

Other than serious performance suck, of course... ;)

Comment 31

[Boris Zbarsky [:bzbarsky]]

Do we need a separate bug on reenabling downscaling?  If so, can someone please get that filed?

Comment 32

[Bill Gianopoulos [:WG9s]]

I will do that and post the bug number back here.

Comment 33

[Bill Gianopoulos [:WG9s]]

bug 419927 filed.

Comment 34

[Bill Gianopoulos [:WG9s]]

Changing summary as the summary will be incorrect once the code for bug 419927 is approved and checked-in.

Comment 37

[Merike Sell (:merike)]

(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.

Comment 38

[Bill Gianopoulos [:WG9s]]

If they are being scaled to a larger size, the issue could be Bug 422179.

Comment 39

[Dão Gottwald [:dao]]

Attachment: patch

Comment 40

[[e-mail]]

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?

Comment 41

[Dão Gottwald [:dao]]

(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 42

[[e-mail]]

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?

Comment 43

[Dão Gottwald [:dao]]

(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 44

[[e-mail]]

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.

Comment 45

[Dão Gottwald [:dao]]

Well, it looks misaligned, what else can I say?

Comment 46

[Dão Gottwald [:dao]]

Attachment: patch v2

other comments addressed

Comment 47

[Dão Gottwald [:dao]]

Attachment: HTTP auth / centered

Comment 48

[Dão Gottwald [:dao]]

Attachment: HTTP auth / left aligned

Comment 49

[Dão Gottwald [:dao]]

Attachment: HTTP auth / Epiphany

Comment 50

[Dão Gottwald [:dao]]

Attachment: password required / centered

Comment 51

[Dão Gottwald [:dao]]

Attachment: password required / left aligned

Comment 52

[[e-mail]]

(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.]

Comment 53

[[e-mail]]

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 .

Comment 54

[Dão Gottwald [:dao]]

Attachment: patch v3

!important dropped

Comment 55

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/693b2a27975a

Comment 56

[Merike Sell (:merike)]

32px icons are correctly displayed as 32px in Mozilla/5.0 (X11; Linux i686; rv:2.0b7pre) Gecko/20100916 Firefox/4.0b7pre.