Last Comment Bug 331331 - Change popup blocker's color
: Change popup blocker's color
Status: RESOLVED FIXED
: fixed1.8.1
Product: Camino Graveyard
Classification: Graveyard
Component: Annoyance Blocking (show other bugs)
: Trunk
: PowerPC Mac OS X
P1 normal (vote)
: Camino1.5
Assigned To: Nick Kreeger
:
:
Mentors:
Depends on: 353039
Blocks: 343938
  Show dependency treegraph
 
Reported: 2006-03-22 05:33 PST by Håkan Waara
Modified: 2006-10-03 06:39 PDT (History)
8 users (show)
samuel.sidler+old: camino1.5+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Light blue (12.31 KB, image/png)
2006-03-22 06:17 PST, Håkan Waara
no flags Details
Slightly more blue variant (12.52 KB, image/png)
2006-03-22 06:18 PST, Håkan Waara
no flags Details
Light brown / beige (10.76 KB, image/png)
2006-03-22 06:19 PST, Håkan Waara
no flags Details
Mockup of the "extended tab" popup blocker (67.07 KB, image/png)
2006-06-18 21:27 PDT, froodian (Ian Leue)
no flags Details
"Extended tab" popup blocker with no tab bar (61.06 KB, image/png)
2006-06-18 21:28 PDT, froodian (Ian Leue)
no flags Details
"Extended tab" popup blocker with no tab or bookmarks bar (51.53 KB, image/png)
2006-06-18 21:29 PDT, froodian (Ian Leue)
no flags Details
Dark background image to be repeated (324 bytes, image/tiff)
2006-09-21 12:20 PDT, Jon Hicks
no flags Details
Special close buttons to sit on the dark strip (3.23 KB, application/zip)
2006-09-21 12:50 PDT, Jon Hicks
no flags Details
Patch 1 (2.26 KB, patch)
2006-09-21 13:44 PDT, Nick Kreeger
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
Screen shot. (87.67 KB, image/png)
2006-09-21 14:15 PDT, Nick Kreeger
no flags Details
Patch Version 2 (3.21 KB, patch)
2006-09-27 22:22 PDT, Nick Kreeger
stuart.morgan+bugzilla: review+
sfraser_bugs: superreview+
Details | Diff | Splinter Review
Popup Novelties (11.47 KB, application/zip)
2006-09-27 22:24 PDT, Nick Kreeger
alqahira: review+
sfraser_bugs: superreview+
Details
Trunk project patch (8.16 KB, patch)
2006-09-27 23:09 PDT, Smokey Ardisson (offline for a while; not following bugs - do not email)
sfraser_bugs: superreview+
Details | Diff | Splinter Review
Branch project patch (8.16 KB, patch)
2006-09-27 23:10 PDT, Smokey Ardisson (offline for a while; not following bugs - do not email)
sfraser_bugs: superreview+
Details | Diff | Splinter Review
Cleaned up popup-blocked icon (744 bytes, image/tiff)
2006-09-29 23:44 PDT, Jon Hicks
no flags Details
Cleaned up pop up blocked icon (744 bytes, image/tiff)
2006-09-29 23:48 PDT, Jon Hicks
no flags Details
Patch version 3 (4.21 KB, patch)
2006-10-02 14:05 PDT, Nick Kreeger
no flags Details | Diff | Splinter Review

Description User image Håkan Waara 2006-03-22 05:33:44 PST
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 User image Håkan Waara 2006-03-22 06:17:58 PST
Created attachment 215889 [details]
Light blue
Comment 2 User image Håkan Waara 2006-03-22 06:18:32 PST
Created attachment 215890 [details]
Slightly more blue variant
Comment 3 User image Håkan Waara 2006-03-22 06:19:01 PST
Created attachment 215891 [details]
Light brown / beige
Comment 4 User image Håkan Waara 2006-03-22 06:20:33 PST
So here are some ideas.

I kinda like one of the blue variants, they feel modern, and are more visible.
Comment 5 User image Samuel Sidler (old account; do not CC) 2006-03-22 08:19:49 PST
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 User image froodian (Ian Leue) 2006-06-18 21:27:37 PDT
Created attachment 226107 [details]
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 User image froodian (Ian Leue) 2006-06-18 21:28:17 PDT
Created attachment 226108 [details]
"Extended tab" popup blocker with no tab bar
Comment 8 User image froodian (Ian Leue) 2006-06-18 21:29:15 PDT
Created attachment 226109 [details]
"Extended tab" popup blocker with no tab or bookmarks bar
Comment 9 User image froodian (Ian Leue) 2006-06-18 21:33:46 PDT
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 User image Håkan Waara 2006-06-19 01:14:18 PDT
I for one think that looks great!  Ideally the extended popup blocker would use the same gradient as the tab bar.
Comment 11 User image froodian (Ian Leue) 2006-06-30 13:51:32 PDT
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 User image Jon Hicks 2006-09-18 03:15:06 PDT
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 User image froodian (Ian Leue) 2006-09-18 05:31:13 PDT
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 User image Håkan Waara 2006-09-19 01:10:57 PDT
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 User image Jon Hicks 2006-09-19 01:45:43 PDT
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-19 02:07:07 PDT
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 User image Mike Pinkerton (not reading bugmail) 2006-09-19 09:10:16 PDT
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 User image Jon Hicks 2006-09-19 14:26:31 PDT
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 User image Samuel Sidler (old account; do not CC) 2006-09-21 09:06:03 PDT
(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 User image Jon Hicks 2006-09-21 12:20:56 PDT
Created attachment 239547 [details]
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 User image Samuel Sidler (old account; do not CC) 2006-09-21 12:23:38 PDT
(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 User image Jon Hicks 2006-09-21 12:50:00 PDT
Created attachment 239549 [details]
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 User image Nick Kreeger 2006-09-21 13:44:16 PDT
Created attachment 239554 [details] [diff] [review]
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 User image Nick Kreeger 2006-09-21 13:46:13 PDT
(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 User image Nick Kreeger 2006-09-21 14:15:20 PDT
Created attachment 239560 [details]
Screen shot.

Here is a screen shot of the new nib/images/patch.
Comment 26 User image Stuart Morgan 2006-09-21 21:15:53 PDT
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 User image Stuart Morgan 2006-09-21 21:27:46 PDT
(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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-21 21:45:49 PDT
(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 User image Håkan Waara 2006-09-22 01:23:25 PDT
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 User image Nick Kreeger 2006-09-27 22:22:14 PDT
Created attachment 240412 [details] [diff] [review]
Patch Version 2

Here is an updated patch.
Comment 31 User image Nick Kreeger 2006-09-27 22:24:49 PDT
Created attachment 240413 [details]
Popup Novelties 

Nib and associated images (that have been renamed).
Comment 32 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-27 23:09:29 PDT
Created attachment 240416 [details] [diff] [review]
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-27 23:10:35 PDT
Created attachment 240417 [details] [diff] [review]
Branch project patch

Ditto the above, but for the 1.8branch.
Comment 34 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-27 23:17:47 PDT
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 User image Stuart Morgan 2006-09-28 21:21:28 PDT
Comment on attachment 240412 [details] [diff] [review]
Patch Version 2

r=me
Comment 36 User image Samuel Sidler (old account; do not CC) 2006-09-29 07:03:29 PDT
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 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-09-29 15:14:36 PDT
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 User image Simon Fraser 2006-09-29 22:08:19 PDT
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 User image Simon Fraser 2006-09-29 22:09:29 PDT
Comment on attachment 240413 [details]
Popup Novelties 

OK, but I think you can make the buttons (esp. "Configure..." a little less wide).
Comment 40 User image Jon Hicks 2006-09-29 23:44:15 PDT
Created attachment 240715 [details]
Cleaned up popup-blocked icon
Comment 41 User image Jon Hicks 2006-09-29 23:48:50 PDT
Created attachment 240717 [details]
Cleaned up pop up blocked icon

My apologies - this is the correct one.
Comment 42 User image Jon Hicks 2006-09-29 23:59:48 PDT
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 User image Nick Kreeger 2006-09-30 00:02:31 PDT
(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 User image Nick Kreeger 2006-10-02 14:05:10 PDT
Created attachment 240978 [details] [diff] [review]
Patch version 3

Here is the patch I will check in, it is updated to meet smfr's comments.
Comment 45 User image Nick Kreeger 2006-10-02 14:07:17 PDT
 - (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 User image Nick Kreeger 2006-10-02 16:12:30 PDT
Landed on trunk and the MOZILLA_1_8_BRANCH.
Comment 47 User image Jon Hicks 2006-10-03 06:39:57 PDT
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

Note You need to log in before you can comment on or make changes to this bug.