352621 - phishing warning bubble uses wrong closebutton (on mac)

Status: RESOLVED INVALID

(Toolkit :: Safe Browsing, defect)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

The phishing warning bubble still uses the red closebutton instead of the new grey one from the tabstrip and pretty much everywhere else. Screenshot coming.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Attachment: screenshot

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

philor tells me that this might be intentional; if it is, feel free to WONTFIX, but with the understanding that I think it's pretty darned odd.

Comment 3

[Jay Goldman]

It's not intentional on our part if that helps anyone make a call on fixing this...

Comment 4

[Phil Ringnalda (:philor)]

I didn't mean to imply it was intentional, just that it wasn't _exactly_ something we forgot while changing the close button: safebrowsing uses its own fake of the closebutton, in browser/themes/(win|pin)stripe/browser/safebrowsing/close16x16.png, rather than using the images everyone else uses, so

 - it will always look wrong, because it doesn't have inactive/hover/active states
 - it will almost always be forgotten, because changing one image, and searching for references to that image name in CSS, will change every other instance of a close button in the tree, but not that one

And though the difference isn't as obvious in winstripe, it's wrong there too: compare http://lxr.mozilla.org/mozilla1.8/source/browser/themes/winstripe/browser/Close-tab.png and http://lxr.mozilla.org/mozilla1.8/source/browser/themes/winstripe/browser/safebrowsing/close16x16.png

Comment 5

[Tony Chang (Google)]

(In reply to comment #4)
> I didn't mean to imply it was intentional, just that it wasn't _exactly_
> something we forgot while changing the close button: safebrowsing uses its own
> fake of the closebutton, in
> browser/themes/(win|pin)stripe/browser/safebrowsing/close16x16.png, rather than
> using the images everyone else uses, so
> 
>  - it will always look wrong, because it doesn't have inactive/hover/active
> states
>  - it will almost always be forgotten, because changing one image, and
> searching for references to that image name in CSS, will change every other
> instance of a close button in the tree, but not that one

So it sounds like what should be done is to start using the same image as elsewhere and add the hover/active effect.

Comment 6

[Tony Chang (Google)]

Attachment: v1: use tab close icon, add css hover

Comment 7

[Phil Ringnalda (:philor)]

Comment on attachment 238964 [details] [diff] [review]
v1: use tab close icon, add css hover

Hahahaha, you asked mconnor for review.

Oh, wait, that's not funny.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 238964 [details] [diff] [review]
v1: use tab close icon, add css hover

>Index: browser/themes/*stripe/browser/safebrowsing/browser-protection.css
>===================================================================

> #safebrowsing-palm-close { 
>   height: 16px; 
>   width: 16px;

You can remove this now, I think.

r=mano otherwise.

Comment 9

[Philip Chee]

Phishing popup bubble has been replaced by an in-content error page. Closing as INVALID.