The default bug view has changed. See this FAQ.

Change popup blocker's color

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Annoyance Blocking
P1
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Håkan Waara, Assigned: Nick Kreeger)

Tracking

({fixed1.8.1})

Trunk
Camino1.5
PowerPC
Mac OS X
fixed1.8.1
Dependency tree / graph
Bug Flags:
camino1.5 +

Details

Attachments

(13 attachments, 4 obsolete attachments)

12.31 KB, image/png
Details
12.52 KB, image/png
Details
10.76 KB, image/png
Details
67.07 KB, image/png
Details
61.06 KB, image/png
Details
51.53 KB, image/png
Details
87.67 KB, image/png
Details
11.47 KB, application/zip
Smokey Ardisson (offline for a while; not following bugs - do not email)
: review+
Simon Fraser
: superreview+
Details
8.16 KB, patch
Simon Fraser
: superreview+
Details | Diff | Splinter Review
8.16 KB, patch
Simon Fraser
: superreview+
Details | Diff | Splinter Review
744 bytes, image/tiff
Details
744 bytes, image/tiff
Details
4.21 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 215889 [details]
Light blue
(Reporter)

Comment 2

11 years ago
Created attachment 215890 [details]
Slightly more blue variant
(Reporter)

Comment 3

11 years ago
Created attachment 215891 [details]
Light brown / beige
(Reporter)

Comment 4

11 years ago
So here are some ideas.

I kinda like one of the blue variants, they feel modern, and are more visible.
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

11 years ago
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

11 years ago
Created attachment 226108 [details]
"Extended tab" popup blocker with no tab bar

Comment 8

11 years ago
Created attachment 226109 [details]
"Extended tab" popup blocker with no tab or bookmarks bar

Comment 9

11 years ago
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.
(Reporter)

Comment 10

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

Comment 11

11 years ago
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
Flags: camino1.1?
Target Milestone: --- → Camino1.1
Blocks: 343938
Assignee: mikepinkerton → nobody
Component: Location Bar & Autocomplete → Annoyance Blocking
QA Contact: annoyance.blocking

Comment 12

11 years ago
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

11 years ago
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.
(Reporter)

Comment 14

11 years ago
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

11 years ago
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? 
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....
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

11 years ago
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)
(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!
Flags: camino1.1? → camino1.1+

Comment 20

11 years ago
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?
(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

11 years ago
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!
(Assignee)

Comment 23

11 years ago
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
Assignee: nobody → nick.kreeger
Status: NEW → ASSIGNED
Attachment #239554 - Flags: review?(stuart.morgan)
(Assignee)

Comment 24

11 years ago
(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.
(Assignee)

Comment 25

11 years ago
Created attachment 239560 [details]
Screen shot.

Here is a screen shot of the new nib/images/patch.
Priority: -- → P1

Comment 26

11 years ago
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?
Attachment #239554 - Flags: review?(stuart.morgan) → review-

Comment 27

11 years ago
(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.
(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.
Depends on: 353039
(Reporter)

Comment 29

11 years ago
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.
(Assignee)

Comment 30

11 years ago
Created attachment 240412 [details] [diff] [review]
Patch Version 2

Here is an updated patch.
Attachment #239554 - Attachment is obsolete: true
Attachment #240412 - Flags: review?(stuart.morgan)
(Assignee)

Comment 31

11 years ago
Created attachment 240413 [details]
Popup Novelties 

Nib and associated images (that have been renamed).
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.
Attachment #239547 - Attachment is obsolete: true
Attachment #239549 - Attachment is obsolete: true
Created attachment 240417 [details] [diff] [review]
Branch project patch

Ditto the above, but for the 1.8branch.
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

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

r=me
Attachment #240412 - Flags: review?(stuart.morgan) → review+
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.
Attachment #240412 - Flags: superreview?(sfraser_bugs)
Attachment #240413 - Flags: superreview?(sfraser_bugs)
Attachment #240413 - Flags: review?(alqahira)
Attachment #240416 - Flags: superreview?(sfraser_bugs)
Attachment #240417 - Flags: superreview?(sfraser_bugs)
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.
Attachment #240413 - Flags: review?(alqahira) → review+

Comment 38

11 years ago
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).
Attachment #240412 - Flags: superreview?(sfraser_bugs) → superreview+

Comment 39

11 years ago
Comment on attachment 240413 [details]
Popup Novelties 

OK, but I think you can make the buttons (esp. "Configure..." a little less wide).
Attachment #240413 - Flags: superreview?(sfraser_bugs) → superreview+

Updated

11 years ago
Attachment #240416 - Flags: superreview?(sfraser_bugs) → superreview+

Updated

11 years ago
Attachment #240417 - Flags: superreview?(sfraser_bugs) → superreview+

Comment 40

11 years ago
Created attachment 240715 [details]
Cleaned up popup-blocked icon

Comment 41

11 years ago
Created attachment 240717 [details]
Cleaned up pop up blocked icon

My apologies - this is the correct one.

Comment 42

11 years ago
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.
(Assignee)

Comment 43

11 years ago
(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.
(Assignee)

Comment 44

11 years ago
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.
Attachment #240412 - Attachment is obsolete: true
(Assignee)

Comment 45

11 years ago
 - (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.
(Assignee)

Comment 46

11 years ago
Landed on trunk and the MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED

Comment 47

11 years ago
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
You need to log in before you can comment on or make changes to this bug.