363130 - Search Engine dropdown icon ugliness where iconsize!=16x16

Status: VERIFIED FIXED

(Firefox :: Search, defect, P2)

Description

[Mycroft Project (CC)]

Visit http://www.idealo.de/ and note the appearance of the autodiscovery bit in the searchbar dropdown.

Also add the engine (ignore the fact that the autodiscovery doesn't disappear due to the difference between the autodiscovery title and the engine name) and then go to Manage Search Engines and note that the icon is constrained to 16 pixels high but is still 47 wide.

Presume options are:
1. cater for any size icon
2. squash to 16x16

Consider this a vote for option 2.

I think this only occurs if it is picking up the favicon.ico rather than an icon specified in the OpenSearch description but I haven't tested exhaustively.

Comment 1

[Mycroft Project (CC)]

This may actually exist already as bug 300204 but I'm not sure if it's same cause or just same effect and I don't really feel like duping myself...

Comment 2

[:Gavin Sharp [email: [e-mail]]]

*** Bug 300204 has been marked as a duplicate of this bug. ***

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: branch patch

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Attachment: limit all menuitem icons to 16x16, for Linux and Windows

We already do this on the mac, so we should do it consistently.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 257697 [details] [diff] [review]
limit all menuitem icons to 16x16, for Linux and Windows

> +  list-style-image: inherit;

hrm?

If we take this change there are few places in winstripe to update.

Comment 7

[Asaf Romano (gone)]

Comment on attachment 257692 [details] [diff] [review]
branch patch

at least for the branch.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch, without inherit, with cleanup

Comment 9

[Asaf Romano (gone)]

Comment on attachment 274199 [details] [diff] [review]
patch, without inherit, with cleanup

r=mano without the dead code change.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 257692 [details] [diff] [review]
branch patch

It'd be good to get this fixed on the branch. This is the simple patch that touches the search dropdown specifically.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch

mozilla/toolkit/themes/gnomestripe/global/menu.css 	1.14
mozilla/browser/themes/winstripe/browser/browser.css 	1.72
mozilla/browser/themes/winstripe/browser/bookmarks/addBookmark.css 	1.5
mozilla/toolkit/themes/pinstripe/global/browser.css 	1.22
mozilla/browser/themes/winstripe/browser/places/bookmarkProperties.css 	1.10
mozilla/toolkit/themes/winstripe/global/browser.css 	1.33
mozilla/toolkit/themes/winstripe/global/menu.css 	1.14

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 257692 [details] [diff] [review]
branch patch

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated branch patch

The patch for bug 306330 hasn't landed on the branch, so the original patch didn't apply. Since it's a trivial change I'll just backport it.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/themes/winstripe/browser/browser.css 	1.17.2.65

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(This won't cause bug 393804 on the branch because bug 337771 didn't land there.)

Comment 16

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.7 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.7pre) Gecko/2007091204 BonEcho/2.0.0.7pre and with the steps to reproduce in this bug.

Adding verified keyword

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #15)
> (This won't cause bug 393804 on the branch because bug 337771 didn't land
> there.)

Also, because the patch is different! (I think I was tired when I wrote this).

Comment 18

[Marcia Knous [:marcia]]

Litmus Triage Team: We are uncertain as to whether a Litmus test case is needed for this bug.

Comment 19

[Mycroft Project (CC)]

2.0.0.8 fixes the dropdown issue but does not fix the display in the Manage Search Engines... list.

URL updated as idealo.de no longer suitable test.

TEST...
Add linked engine and witness display in MSE list

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Can you file a new bug on that issue, Charles? Easier to track that way. Seems to affect the trunk too, although it gets cut off vertically.

Comment 21

[Carsten Book [:Tomcat]]

Litmus Triage team: Tomcat will create a testcase for this

Comment 22

[Carsten Book [:Tomcat]]

created testcase of this ...setting in-litmus flag 

Comment 24

[carglue a t ya hoo do tcom]

Modifying the base-level menu.css file creates problems for extensions and applications which use XUL menus with icons because the rule inappropriately forces (or "squashes" as it was called) the icon's image to an specific size.

The original issue can still be fixed easily, but rather than hacking the underlying xul styling file (menu.css) upon which all XUL applications and extensions depend, it would be better to retain the specific styling rules for the Firefox browser, as it was done originally and as was done in the initial patch in comment #4, rather than forcing all XUL applications and extensions to override this (Firefox-specific) image size rule.

See related discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=419540

Comment 25

[:Gavin Sharp [email: [e-mail]]]

I've commented in bug 419540, but just to address the comments here:

There's nothing Firefox specific about this rule. The change was made to menu.css to match the platform convention of using 16px images in menus. It doesn't "force" anything on application authors, you're still able to override this default styling as you please.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Gavin, it should be easy to create an automated test for this bug. I ask because we would like to get rid-off the manual test on Litmus which is a bit of waste time for testers.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Further, the referenced search engine suppy a 16x16 icon now which makes the Litmus test useless.

See: http://mycroft.mozdev.org/installos.php/27213/btjunkie-def.xml

Comment 28

[:Gavin Sharp [email: [e-mail]]]

An automated test that checked computed style for the icon element could probably be written, I guess. It seems unlikely to be regressed given that it's now an explicit rule, though...

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Eventually I haven't found the correct line in the source but setIcon doesn't check for the size. Can you point me to the correct code? I've only found http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#1375. Thanks.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

The size of the source icon itself doesn't really matter (though we do have a check for absurdly large icons, in terms of file size: search for MAX_ICON_SIZE).

All that matters is that we display it at 16x16, and for that we use CSS (see this bug's patches).

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ah ok. Thanks for the clarification. In this case I have deleted the Litmus testcase. Gavin, I would leave it up to you if we wanna have an automated test or not. At least we would fetch CSS/theme regressions, right?

Comment 32

[[e-mail]]

Comment on attachment 275588 [details] [diff] [review]
updated patch

>+.menu-iconic-icon {
>+  width: 16px;
>+  height: 16px;
>+}
Maybe conditioning this on a [src] attribute would have reduced the amount of fallout?