Closed Bug 363130 Opened 13 years ago Closed 12 years ago

Search Engine dropdown icon ugliness where iconsize!=16x16

Categories

(Firefox :: Search, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 3 alpha8

People

(Reporter: mail, Assigned: Gavin)

References

()

Details

(Keywords: verified1.8.1.8)

Attachments

(2 files, 3 obsolete files)

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.
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...
*** Bug 300204 has been marked as a duplicate of this bug. ***
Assignee: nobody → gavin.sharp
Duplicate of this bug: 371506
Attached patch branch patch (obsolete) — Splinter Review
Attachment #257692 - Flags: review?(mano)
Attachment #257692 - Attachment is obsolete: true
Attachment #257692 - Flags: review?(mano)
We already do this on the mac, so we should do it consistently.
Attachment #257697 - Flags: review?(mano)
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.
Attachment #257697 - Flags: review?(mano)
Comment on attachment 257692 [details] [diff] [review]
branch patch

at least for the branch.
Attachment #257692 - Attachment is obsolete: false
Attachment #257692 - Flags: review+
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → Firefox 3 M7
Attachment #257692 - Attachment is obsolete: true
Attachment #257697 - Attachment is obsolete: true
Attachment #274199 - Flags: review?(mano)
Comment on attachment 274199 [details] [diff] [review]
patch, without inherit, with cleanup

r=mano without the dead code change.
Attachment #274199 - Flags: review?(mano) → review+
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.
Attachment #257692 - Attachment is obsolete: false
Attachment #257692 - Flags: approval1.8.1.7?
Keywords: checkin-needed
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Attached patch updated patchSplinter Review
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
Attachment #274199 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #257692 - Attachment description: patch → branch patch
Comment on attachment 257692 [details] [diff] [review]
branch patch

approved for 1.8.1.7, a=dveditz for release-drivers
Attachment #257692 - Flags: approval1.8.1.7? → approval1.8.1.7+
Keywords: checkin-needed
Whiteboard: [1.8 branch checkin]
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.
Attachment #257692 - Attachment is obsolete: true
mozilla/browser/themes/winstripe/browser/browser.css 	1.17.2.65
Whiteboard: [1.8 branch checkin]
(This won't cause bug 393804 on the branch because bug 337771 didn't land there.)
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
(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).
Litmus Triage Team: We are uncertain as to whether a Litmus test case is needed for this bug.
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
Status: RESOLVED → REOPENED
Keywords: verified1.8.1.8
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 13 years ago12 years ago
Keywords: verified1.8.1.8
Resolution: --- → FIXED
Summary: Search Engine icon ugliness where iconsize!=16x16 → Search Engine dropdown icon ugliness where iconsize!=16x16
Status: RESOLVED → VERIFIED
Litmus Triage team: Tomcat will create a testcase for this
created testcase of this ...setting in-litmus flag 
Flags: in-litmus? → in-litmus+
Depends on: 410672
Depends on: 419540
Duplicate of this bug: 419540
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
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.
Depends on: 411064
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.
Flags: in-testsuite?
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
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...
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.
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).
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?
Flags: in-litmus+ → in-litmus-
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?
You need to log in before you can comment on or make changes to this bug.