331331 - Change popup blocker's color

Status: RESOLVED FIXED

(Camino Graveyard :: Annoyance Blocking, defect, P1)

Description

[Håkan Waara]

Since no bug was filed for this, I'm filing this one. I'll try to do something constructive and come up with a few proposals.

Comment 1

[Håkan Waara]

Attachment: Light blue

Comment 2

[Håkan Waara]

Attachment: Slightly more blue variant

Comment 3

[Håkan Waara]

Attachment: Light brown / beige

Comment 4

[Håkan Waara]

So here are some ideas.

I kinda like one of the blue variants, they feel modern, and are more visible.

Comment 5

[Samuel Sidler (old account; do not CC)]

For the future (if we need more), I'd like to see these in context of the nav/bookmark/tab bar combo and actual content.

Comment 6

[froodian (Ian Leue)]

Attachment: Mockup of the "extended tab" popup blocker

On IRC we discussed the idea of making the popup blocker look more associated with the tab it belongs to.  This way, it would be seen as an extension of the chrome, rather than an invasion of the content pane, and it would be obvious that it "belongs" to the currnet tab, not the current web content.  Here's a mockup of what that could look like.

Comment 7

[froodian (Ian Leue)]

Attachment: "Extended tab" popup blocker with no tab bar

Comment 8

[froodian (Ian Leue)]

Attachment: "Extended tab" popup blocker with no tab or bookmarks bar

Comment 9

[froodian (Ian Leue)]

More notes (sorry for the bugspam):

A side effect of this is that we'd need to shuffle between a couple states here - drawing the regular tabs when there's no popup notification, but drawing them extended (ie without the three px "content pane starts here" border) when there is one.  Also, the items in the popup notification itself need to shift up two pixels if there's no tab bar.  All this is reflected in the mockups.  The upside is that it remains clear where the browser ends and the content begins - a line which is currently fuzzy.

Comment 10

[Håkan Waara]

I for one think that looks great!  Ideally the extended popup blocker would use the same gradient as the tab bar.

Comment 11

[froodian (Ian Leue)]

We get lots of feedback concerning the pop-up blocker, and almost all of it mentions the current color in a negative context.  Even if it ends up being just toning down the color, something should be done before we ship like this.  Requesting blocking for 1.1

Comment 12

[Jon Hicks]

Colour suggestions for the pop up blocker: http://www.hicksdesign.co.uk/clients/camino/PopUpBlocker.zip

Presumably we can use a background image for this? I'm thinking of  a repeating background gradient that will not only improve the colour but make it look more like a part of the browser (and therefore noticeable), rather than a part of the site.

I like the 'dark' and 'dark blue' background of these options. While the colours aren't typical 'attention' tones, they are noticeable, and more pleasant than yellow. I've done a second version of each of these using the /!\ icon from the error page.

Comment 13

[froodian (Ian Leue)]

I feel pretty strongly that the way to solve this is to go with the "extended tab" look like the mockups I posted here.

That said, I suppose in an absolute worst case scenario (since "extended tabs" will require code changes), just changing the color would be ok.  I really don't want to though.

Comment 14

[Håkan Waara]

One good thing about froodian's "extended tab" mockup is that the popup blocker and current tab are visually merged. Since the blocker is per-tab, this is a good thing.

But the gray look is quite dull, IMHO. Jon Hicks' mockups are colorful and make Camino look more like a year new-style Apple application.

Comment 15

[Jon Hicks]

Hmm, I thought I'd posted my reply to Froodian, but it looks like haven't.

Anyway, I agree that the extended tab makes the most sense, but I really don't like the way it looks with the current tab design. Not just the expanse of grey, but the double line at the bottom of the tab looks odd somehow. Perhaps we should look into a different tab style? 

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Per the weekly meeting, we're not going to do any changes in overall design (i.e., "extended tab" stuff) for 1.1, but we are entertaining additional color schemes from Jon for the current bar design....

Comment 17

[Mike Pinkerton (not reading bugmail)]

let me add that we like the idea of the extended tab design, but the work required is of lower priority to things we really should fix to get 1.1 out. I think that's a direction we should move in the future.

Comment 18

[Jon Hicks]

Further options: http://www.hicksdesign.co.uk/clients/camino/PopUpBlocker2.zip

Legibility has been improved, but I'm not sure whether they are any less overpowering (except for a new grey option)

Comment 19

[Samuel Sidler (old account; do not CC)]

(In reply to comment #18)
> Further options: http://www.hicksdesign.co.uk/clients/camino/PopUpBlocker2.zip

Discussing with Mike, and based on the conversation we had at our meeting on Monday, we'd like to try the "dark_with_icon" version and, depending on feedback, possibly switch to the "toned_down_blue" (but with an icon).

Also, we're going to go ahead and try the different colored close buttons and, again, depending on feedback, might remove it in favor of the current close buttons we have now.

Jon, can you attach the following:
1. Enough of the background of the latest "dark" mockup so we can repeat it
2. Three versions of the close button (off, hover, onclick; note that hover doesn't yet work)

The icon is available at attachment 239250 [details].

Thank you!

Comment 20

[Jon Hicks]

Attachment: Dark background image to be repeated

Close icons to follow. Do you need the pop up blocked icon as a separate file too Samuel?

Comment 21

[Samuel Sidler (old account; do not CC)]

(In reply to comment #20)
> Do you need the pop up blocked icon as a separate file too Samuel?

I don't think so. Unless attachment 239250 [details] is something different, we don't.

Comment 22

[Jon Hicks]

Attachment: Special close buttons to sit on the dark strip

Ah yes, I'd forgotten that I'd already posted that icon in another bug. Thats OK then!

Comment 23

[Nick Kreeger]

Attachment: Patch 1

Here is the patch to draw the image.

Smokey, I will need a project file change, please add these images (from the attached resources here):
? resources/images/chrome/popclose.tif
? resources/images/chrome/popclose_click.tif
? resources/images/chrome/popclose_hover.tif
? resources/images/chrome/popup-blocked.tif
? resources/images/chrome/popup_blocked_back.tif

Comment 24

[Nick Kreeger]

(In reply to comment #23)
> Created an attachment (id=239554) [edit]
> Patch 1
> 
> Here is the patch to draw the image.
> 
> Smokey, I will need a project file change, please add these images (from the
> attached resources here):
> ? resources/images/chrome/popclose.tif
> ? resources/images/chrome/popclose_click.tif
> ? resources/images/chrome/popclose_hover.tif
> ? resources/images/chrome/popup-blocked.tif
> ? resources/images/chrome/popup_blocked_back.tif
> 

Smokey: notice that I renamed "Dark background image to be represented" (dark_strip.tif) to popup_blocked_back.tif.

Comment 25

[Nick Kreeger]

Attachment: Screen shot.

Here is a screen shot of the new nib/images/patch.

Comment 26

[Stuart Morgan]

Comment on attachment 239554 [details] [diff] [review]
Patch 1

>+static NSImage*  gPopupGradient;  // popup view gradient image.

Declare this in the function that uses it, since it's only needed in one place (and call it sPopupGradient)

>+                        operation:NSCompositeSourceOver];

Use NSCompositeCopy, since there's nothing underneath we care about.

The next round may as well wait until it's a complete patch, I think.


Some general notes about all the above:

> ? resources/images/chrome/popclose.tif
> ? resources/images/chrome/popclose_click.tif
> ? resources/images/chrome/popclose_hover.tif
> ? resources/images/chrome/popup-blocked.tif
> ? resources/images/chrome/popup_blocked_back.tif

These need consistent names; right now the prefix and the separators vary from file to file. How about:
popup_close.tif
popup_close_click.tif
popup_close_hover.tif
popup_blocked_icon.tif
popup_blocked_background.tif

Visually, I'm a bit concerned about the rounded look this background has. Putting Aqua buttons designed to look like they are on/in a flat surface on top of something that looks very 3D feels wrong to me, because the place where they join doesn't have the depth or shadowing I would expect to see. Could the depth be toned down by making the highlight/shadow disparity more subtle?

Comment 27

[Stuart Morgan]

(In reply to comment #26)
> Could the depth be toned down by making the highlight/shadow disparity
> more subtle?

In case anyone else is concerned, I'm not suggesting that we hold the bug for such a mockup if there isn't one ready by the time the code and nib are done.

Comment 28

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #26)
> These need consistent names; right now the prefix and the separators vary from
> file to file. How about:
> popup_close.tif
> popup_close_click.tif
> popup_close_hover.tif
> popup_blocked_icon.tif
> popup_blocked_background.tif

While we're renaming, can we go with 

popup_close.tif
popup_close_hover.tif
popup_close_pressed.tif

so that we're in sync with the other close button?

Also, once everything is ready, I can do the branch patch, but I'd rather not do the trunk patch until bug 353039 has landed, because those project changes were such a PITA to do.

Comment 29

[Håkan Waara]

For whoever is collecting comments:

* The stop-button looks a bit "jagged" (not smooth) in the screenshot?
* I think the aqua button looks better on a slightly lighter background. (Also, the focus ring will be hard to see on the black background.)

Nice to see some action in this bug.

Comment 30

[Nick Kreeger]

Attachment: Patch Version 2

Here is an updated patch.

Comment 31

[Nick Kreeger]

Attachment: Popup Novelties

Nib and associated images (that have been renamed).

Comment 32

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Trunk project patch

This also removes the unused "popup-blocked.png" from the project, but it still needs to be cvs removed.  RIP splat.

Comment 33

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Branch project patch

Ditto the above, but for the 1.8branch.

Comment 34

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

In addition to Stuart's and Håkan's comments, here's my 2¢ from the peanut gallery:

* The close button's pressed state seems indistinguishable from the hover state
* The "edges" on the popup_blocked_icon seem a little "rough", like the white band is not a unified width or something (may just be an optical illusion)

Ian also mentioned we're going to have issues with tiling when we fix the text wrapping.

Again, none of this should stall the landing of the patch (it'll be much easier to try things out and fix graphics once it's landed) when the patch passes code muster.

Comment 35

[Stuart Morgan]

Comment on attachment 240412 [details] [diff] [review]
Patch Version 2

r=me

Comment 36

[Samuel Sidler (old account; do not CC)]

Comment on attachment 240412 [details] [diff] [review]
Patch Version 2

Simon, this, and the rest of the review requests in this bug are for changing the color and style of the popup blocker.

There are still a view concerns over the design (as mentioned in this bug and elsewhere), but Mike wanted to try this overall design and work with changing it later after we get feedback.

Comment 37

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 240413 [details]
Popup Novelties 

There are known problems with this nib, but they are covered by other follow-up bugs (e.g., bug 341967, bug 346803) or by comments here we've agreed to fix as follow-up to landing.

Otherwise, this all works as expected, so r=ardissone.

Comment 38

[Simon Fraser [no longer active]]

Comment on attachment 240412 [details] [diff] [review]
Patch Version 2

>? resources/images/chrome/popup_blocked_background.tif
>? resources/images/chrome/popup_blocked_icon.tif
>? resources/images/chrome/popup_close.tif
>? resources/images/chrome/popup_close_hover.tif
>? resources/images/chrome/popup_close_pressed.tif

Don't forget to cvs add -kb these before checkin.

>Index: src/browser/BrowserWrapper.mm
>===================================================================

> - (void)drawRect:(NSRect)aRect
> {
>-  // draw background color
>-  [[NSColor colorWithCalibratedRed:1.0 green:0.9 blue:0.58 alpha:1.0] set];
>-  NSRectFill([self frame]);
>-  
>-  // draw shadowed border
>-  [[NSColor controlShadowColor] set];
>-  NSRectFill(NSMakeRect(aRect.origin.x, 0.0, aRect.size.width, 1.0));
>-
>+  static NSImage* sPopupGradient;

It's an image, so call it one. Also don't make presuppositions about the image
contents ("gradient"). So call it sPopupBlockedBackgroundImage.

>+  if (!sPopupGradient)
>+    sPopupGradient = [[NSImage imageNamed:@"popup_blocked_background"] retain];

I'm not huge on the static; it would be cleaner to make this a member var in InformationPanel (which really needs to be called InformationPanelView).

Comment 39

[Simon Fraser [no longer active]]

Comment on attachment 240413 [details]
Popup Novelties 

OK, but I think you can make the buttons (esp. "Configure..." a little less wide).

Comment 40

[Jon Hicks]

Attachment: Cleaned up popup-blocked icon

Comment 41

[Jon Hicks]

Attachment: Cleaned up pop up blocked icon

My apologies - this is the correct one.

Comment 42

[Jon Hicks]

Regarding other feedback on the graphics, would it be OK to leave these until there is a build available with the patch? It will easier to judge and test when I can swap image resources and see the results.

Comment 43

[Nick Kreeger]

(In reply to comment #42)
> Regarding other feedback on the graphics, would it be OK to leave these until
> there is a build available with the patch? It will easier to judge and test
> when I can swap image resources and see the results.
> 

I'll land this later today so nightlies starting tonight will have this.

Comment 44

[Nick Kreeger]

Attachment: Patch version 3

Here is the patch I will check in, it is updated to meet smfr's comments.

Comment 45

[Nick Kreeger]

 - (void)drawRect:(NSRect)aRect
 {
-  // draw background color
-  [[NSColor colorWithCalibratedRed:1.0 green:0.9 blue:0.58 alpha:1.0] set];
-  NSRectFill([self frame]);
-  
-  // draw shadowed border
-  [[NSColor controlShadowColor] set];
-  NSRectFill(NSMakeRect(aRect.origin.x, 0.0, aRect.size.width, 1.0));
+  static NSImage* sPopupGradient;
 

The static NSImage* will not be in the patch I check in, sorry about that.

Comment 46

[Nick Kreeger]

Landed on trunk and the MOZILLA_1_8_BRANCH.

Comment 47

[Jon Hicks]

Great, I now have a build with the patch, thanks! I can see the problem with the close buttons easily now, I'll revise these.

Also the popup_blocked_icon.tif is an older version, and needs to be replaced with the new on from comment #41  https://bugzilla.mozilla.org/show_bug.cgi?id=331331#c41