1) Go to http://popuptest.com/popuptest1.html 2) Resize window below about 500 pixels or so (depending on size of system font, etc.) 3) Watch notification text overlap buttons Firefox does the Right Thing™ here.

Not letting the text overlap is a simple nib change, however the blocker is currently hardcoded at a fixed height. That'll have to go away, allowing for 2+ lines of text, to fix this.

Gah, this should be 1.1(a1) We also need to make sure we're doing things flexibly enough for l10n.

This should be possible to do by asking the textfield's cell what its preferred size is (cellSizeForBounds:), and then setting the popup blocker's size to that in BrowserWrapper's setFrame:resizingBrowserViewIfHidden: but some NIB magic twiddling might be needed too.

Another note: the "resizing view also resizes subviews" API might also help here (after the nib changes have been done)

I've been thinking about this one all week... IMO truncating the text at the end, with an ellipsis indicating the truncation would look good here, but would you guys prefer the Firefox-way of wrapping the text into multiple lines when needed? I thought the truncating text was easier to wrap my head around, so I've been working at that for the past couple of days: (If you guys would rather the word-wrapping, that's fine too, and don't waste time reading this whole explanation of getting the truncating to work) We could've fixed this in about one sec thanks to a 10.4+ only NSCell method setLineBreakMode:. As for truncating text in 10.3+, NSTextField (its Cell really) does accept an NSAttributedString, which can be configured to contain a paragraph style which indicates line breaking. Handing the NSTextField this attributed string doesn't work, however, as it seems the NSCell never informs the attributed string that it's display size is actually going to be smaller than it's required width. (This kind of technique does work in table view columns) So, I just made a simple subclass of NSTextFieldCell which would just draw the attrib. string in the same rect as the NSTextFieldControl's frame, meaning it would realize the cut-off and the lineBreakMode would kick in. It turned out, the project already contained a TruncatingTextFieldCell by Simon, which I didn't realize until I compiled with my own implementation of a cell with the same name.. I couldn't get his version to accomplish the truncating, in this case. His subclass was not being used anywhere in the project, and was created in Dec'05, so it's not something just being added. I swapped mine in and the truncating worked as expected (and is l10n compatible also). I've attached a screenshot to visualize the behavior. Which style does everyone prefer, and if the truncating style, was it alright to substitute my custom cell instead of Simon's existing one? The wrapping style looks like it would take even more work to properly implement, and requires the coordination of multiple view objects..

We really need all the text to be visible IMO; truncating informational messages is a bad idea, especially given that depending on window width and language in use, one might end up only seing a word or two of the message.

Yeah, good point, and I see what you mean.. In that case: I'll work on the original idea of getting the text to wrap and resizing the blocker view to fit the message.

The shrink-wrap view types may be useful here (look at the Page Info impl.)

Alright.. I've been hacking around with this bug for most of the weekend. This is a solution which works great except the buttons (unblock, configure) don't handle the resizing of their superview (the blocked view) properly. By this, I mean they don't reposition themselves properly. This fix uses the same idea, but different technique, as CHShrinkWrapView, but is not a subclass of it, however. CHShrinkWrapView would not handle padding properly when any other view was inside it (meaning if we only had the NSTextField, it would resize the information view correctly). The reason I'm uploading only a partial fix is that I could use some help. I've been trying forever to get those buttons to re-align themselves after the information panel resizes. The springs/struts are (should) be set correctly in IB. (It seems like the info panel is moving upwards, which I'm pretty sure isn't the case). If anyone has any ideas to offer, that'd be great. Hopefully my implementation for resizing the information panel is sufficient, but if there's a better way, I wouldn't mind if it was done differently.

Comment on attachment 241025 [details] [diff] [review] Partial Fix - Text now wraps like it should, however Buttons don't autosize properly. Simple fix? If you need to hack the frame resizing code, you should probably hack BrowserWrapper's setFrame:resizingBrowser: method instead (apart from changing stuff in the NIB)

Resizing the blocker view is going to result in some trouble with the new background image pattern we're using. The image used to create the background pattern assumes the blocker view is always a fixed height. (Although, FWIW, I think the new view looks awesome). So, we can either re-institute the truncating style (and add a tooltip to display the full message upon mouse-over), or re-work the way the background image is drawn..

As a follow-up, I just read over Smokey's comments for bug 355323 and the image/pattern background is already a known problem. In that case, I'll keep trying to get the wrapping-style working properly.

Per the meeting, we'll stop wrapping at a certain window size (perhaps 595 px); when a window is smaller than that, we'll go back to truncating. (We should also fix the minimum size of the browser window at that size, if possible--i.e., if it doesn't break the ability to have smaller-sized pop-ups.)

Alright, sorry this one took so long. Here is a real, hack-free, solution. The text field will now wrap when the window is resized or when the panel is displayed into an already-small browser window. InformationPanelView will automatically resize itself to contain the textField, preserving top/botttom padding. All other views inside the Information Panel are aligned vertically centered as well. The browser window is informed when the panel is resized, so the web content scrollbar, and other elements, are drawn correctly to accommodate the new size. I ended up doing the geometry myself inside of InformationPanelView, instead of relying on CHShrinkWrapView, since I couldn't adapt that class to work in this particular situation. Note: The panel's background image will have to be drawn differently, because of the dynamic height. (Known issue, bug is filed) The only thing left is the minimum point at which wrapping should stop. I didn't implement this yet, since I was still unsure as to whether it would be applied globally on the Browser window. Additionally, I thought that if the window was allowed to get very small and truncated some of the controls in InfoPanelView, we could use the ">>" button like NSToolbar does when items are clipped out of view.

Comment on attachment 241475 [details] [diff] [review] Patch: Information text wraps, InformationPanelView resizes to fit. I won't have time to review this until at the earliest 16/10.

Comment on attachment 241475 [details] [diff] [review] Patch: Information text wraps, InformationPanelView resizes to fit. Trying smorgan for this and the nib....

Not blocking 1.1a1, but we'd definitely take a patch. Håkan, Stuart: Either of you have time to look at this?

Comment on attachment 241475 [details] [diff] [review] Patch: Information text wraps, InformationPanelView resizes to fit. The InformationPanelView should not be calling up into BrowserWrapper, or have to know anything about it. I'm also not clear on why InformationPanelView is listening for resize notifications from one of its subviews; isn't a resize of InformationPanelView the only thing that's going to trigger a frame change of the text field? It seems like BrowserWrapper should, after telling the InformationPanelView to resize, find out its actual size and do the rest of its resizing appropriately. Similarly, the InformationPanelView should, in its setFrame:, tell the text view to resize then compute its own ideal size appropriately. General notes: - Make sure you have your editor set to use 2 spaces; this patch has a lot of tabs - Try to keep comments succinct, and eliminate comments that just restate the code itself (else means that the if condition false, describing a cast, etc.)

Comment on attachment 241476 [details] Nib to go along. Clearing review flags since the patch was r-minused.

Thanks Stuart for checking out the previous patch and for your help/corrections. How this patch works: When BrowserWrapper is resizing, it first sets the information panel's new width. The panel will then, as part of its setFrame method, wrap text if necessary and resize it's height to enclose the wrapping. BrowserWrapper then fetches the adjusted frame of the Information Panel, and sets its own frame accordingly. Notes: When centering subviews vertically, if their origin is not normalized and do not lie on integer values, the NSButtons inside InfoPanel are drawn incorrectly (visual screenshot if this attached). I use ceil() and #include math.h for that purpose. I know, I don't like doing that either, since the great advantage of Quartz is that it uses floating point values for all drawing. Anyone know what the deal is here? I appoligize for the messed up tabbing before, and I tried to correct that issue. Xcode did seem inconsistent again though, despite having the indentation settings correct. I hope to have fixed it up.

Added an IBOutlet to the message text field.

This happens when vertically centering the buttons on a non-integral location.

Yeah, there are still some problems with tab spacing. I will correct them if the patch passes review..

Comment on attachment 243385 [details] [diff] [review] Patch 2.0 Requesting another review from Stuart. (Thanks)

Comment on attachment 243385 [details] [diff] [review] Patch 2.0 Sorry it took me so long to get to this. The overall approach looks much better. General notes: - Regardless of what else we decide to do about browser window size, this view should enforce something sane on its own. My suggestion would be to allow no more than two lines of text. - The background issue needs to be addressed either in this bug or a bug blocking this one; we shouldn't land wrapping without that, since it looks really silly. - The comments are all very wide; try to wrap them at something closer to 80-100 characters. >+ float bottomPaddingOfPanel; Member variables should always be prefixed with 'm', and the OfPanel isn't necessary since it's a member variable of the panel, so call it mBottomPadding--or even better, mVerticalPadding, since that's how it's used. >+ // its setFrame method, wrap information text if necessary and adjust its own frame to enclose that text. >+ // Then, find out the actual (possibly adjusted) size of blockedPopupView and resize the browser view appropriately. You might want to use 'height' instead of size and/or frame here, just to be specific about what could change. >+ // Here we account for any resizing adjustment performed by the blockedPopupView. No need to re-explain >+ // and adjusts it to properly enclose the text, maintaining bottom padding. s/bottom/vertical/ >+ textFieldFrame.size.width -= oldPanelFrame.size.width - panelFrame.size.width; I think this would read a bit clearer as += new - old >+ [super setFrame:[self adjustedRectEnclosingMessageTextForFrame:panelFrame]]; >... >+- (NSRect)adjustedRectEnclosingMessageTextForFrame:(NSRect)panelFrame Is this method really needed? It seems like all you should need to do in each case is set |panelFrame.size.height| to |textFieldFrame.size.height + 2 * mVerticalPadding| before passing it to super. >+// Aligns all subviews so that their center points fall along a horizantal Horizontal is typo'd >+ float verticallyCenteredYLocationForSubview = 0; >+ NSRect currentSubviewFrame = NSZeroRect; These should be declared when they are needed, rather than up-front. >+ verticallyCenteredYLocationForSubview = ceil(panelFrame.size.height / 2.0) - ceil(currentSubviewFrame.size.height / 2.0); You want to round the total, not the intermediate values, so this should be ceilf((panelFrame.size.height - currentSubviewFrame.size.height) / 2.0) Use ceilf, since these are floats.

Flipping the dependancy per the last comment.

I have an updated patch just about ready to submit again, but I've been stuck for a few days trying to implement a way to stop wrapping at two lines. I'll keep working at it, but I wanted to explain where I'm at in case anyone has a suggestion.. The text field is an instance of AutoSizingTextField, which automatically determines a proper height based upon the wrapping resulting from its width. If the text starts to wrap into three lines, there's no easy way to transform it back into two lines, since it would require knowing which width would result in that scenario. Hard coding a minimum width for the panel (equal to the smallest width before three lines start warpping) does work, but isn't acceptable to me. This isn't just an easy way out, its also useless because that value only corresponds to the english-localized text's wrapping. It's simple to check whether the text has wrapped to three lines, but there's no easy way to force it back into two lines. Adding additional options/capabilities to AutoSizingTextField (a max lines to wrap) might be a way to go here.. I'll come up with something, but I wanted to throw this out there in case anyone has an idea.

It's also possible that in some languages more than two lines would be a better experience; perhaps we should just pick a reasonable min width after all?

Alright, this patch contains all of the fixes/enhancements suggested by Stuart from the last review as part of the text wrapping. Additionally, I also added code to draw a blue-gradient background. Even though the background color kinda has its own entry (bug 355323), the wrapping doesn't make sense without it and this way everything can be reviewed together. Some Notes: The gradient is drawn using Quartz 2D (Core Graphics API). The background image file are no longer needed and can be removed from the project file. (I'm using Xcode 2.4 here, so I can't edit the .xcode one). The popup-blocked icon was re-submitted by Jon and needs to be added as well. As per the previous discussion, there was no easy way to stop wrapping upon reaching a certain amount of lines. A minimum width for the panel is just #defined for now. The browser window is allowed to continue decreasing its size, which will then truncate the panel's controls. There is consequently no way to close the panel if that button is hidden out of the view area, so I've attached a mockup of what we could possibly do in this case. The behavior of clipped items matches that of NSToolbar. (The mockup was just done in Photoshop, I haven't written code for it yet). If the browser window happens to enforce a min width in agreement with the panel's, this isn't necessary.

Here's how it looks now..

When the browser window gets too small, the panel stops resizing since its text will wrap to a ridiculous amount of lines. A side effect this is that controls are clipped out of the view area. This is a mockup of what we could do when that happens, mimicking NSToolbar.

Made a minor change to the nib with the newest patch..

It's a shame that this panel will push the content down, so every time a site trie s to show a popup, the content jiggles. Is that good?

(In reply to comment #37) We could smoothly animate the process, making it really easy for the user to visualize what's happening. I'd compare this to functionality added to system-wide scrolling in Panther. When hitting page-up/down (or home/end) the content isn't directly jumped to but smoothly scrolled towards, making it really easy on the eyes to see what happened and follow the content. Camino doesn't actually scroll this way (I looked into this before and appears to be more of a gecko issue, correct?). Open up Safari to a long page and hit page-down; it slides instead of jumps. So, each time the content must move down, a suggestion is to slide it smoothly. Firefox 2.0 already has this behavior with its popup blocker and Safari does it when showing/hiding its tab bar. I'd be glad to do the work implementing it if need be..

The page-moving-down issue isn't new to this bug, and has been discussed a lot; lets not turn this bug into a rehash of that. Specific thoughts for improving the experience should be new bugs.

(In reply to comment #37) > It's a shame that this panel will push the content down, so every time a site > tries to show a popup, the content jiggles. Is that good? The other thing ameliorating factor will be if we adopt bug 343937 comment 8, in which case you'd only ever see the blocker once (in most cases) per site; you'll either whitelist the popup in Gecko (and see the popups and not invoke the bar), or blacklist the site in Camino (and see neither the popups nor the bar ever again). Or you put the blocking notification back in the status bar altogether (if someone gets around to fixing bug 333531) ;)

Comment on attachment 245659 [details] [diff] [review] Updated text wrapping; Now draws a blue gradient background. Looks good functionally. Code nits: >+// with a blue-shaded background. ... >+// CalculateBlueShadingValues ... >+static void CalculateBlueShadingValues(void *info, const float *in, float *out) >+{ >+ static float lightBlue[4] = { 0.26666666667f, 0.4431372549f, 0.76470588235f, 1.0f }; >+ static float darkBlue[4] = { 0.16078431373f, 0.32156862745f, 0.61960784314f, 1.0f }; ... >+ out[i] = (1.0f - currentInterval) * lightBlue[i] + currentInterval * darkBlue[i]; ... >+// Draws a blue shading behind the view's contents. ... >+ struct CGFunctionCallbacks shadingCallback = { 0, &CalculateBlueShadingValues, NULL }; This is way too many places in the code that would have to change if we change the color of the bar. Remove all references to the color except in a comment above the values themselves (even they should just be called something like gradientTop and gradientBottom). >+ // Ensure all panel subviews are centered vertically, >+ // Then make note of the vertical padding. ... >+ [self verticallyCenterAllSubviews]; >+ mVerticalPadding = [mPopupBlockedMessageTextField frame].origin.y; Remove these comments; the code is just as clear. >+- (void)setFrame:(NSRect)newPanelFrame >+{ >+ // In addition to setting the panel's frame rectangle this method accounts >+ // for wrapping of the message text field in response to this new frame and >+ // adjusts it to properly enclose the text, maintaining vertical padding. This should probably go just before the method rather than inside it. >+ // Enforce a minimum width for the panel. >+ if (newPanelFrame.size.width < kInformationPanelMinWidth) >+ newPanelFrame.size.width = kInformationPanelMinWidth; Can we just enforce a min width on the text field directly? That way its effect wouldn't vary if we adjust the controls. >+ // Change the message text field's width first, so we can adapt if it wraps. >+ // The text wraps and resizes its height (if needed) when the new width is applied. >+ textFieldFrame.size.width += newPanelFrame.size.width - existingPanelFrame.size.width; >+ [mPopupBlockedMessageTextField setFrame:textFieldFrame]; >+ >+ // Fetch the new message text frame, since it may have wrapped & resized. >+ textFieldFrame = [mPopupBlockedMessageTextField frame]; >+ >+ // Adjust the panel height to enclose the message text field. >+ newPanelFrame.size.height = textFieldFrame.size.height + 2.0f * mVerticalPadding; It would be a bit clearer to use 2 instead of 2.0f >+ while (currentSubview = [subviewEnum nextObject]) { while ((foo = ...)) { (use double-parens when doing truth tests on an assignment) >+ static float lightBlue[4] = { 0.26666666667f, 0.4431372549f, 0.76470588235f, 1.0f }; >+ static float darkBlue[4] = { 0.16078431373f, 0.32156862745f, 0.61960784314f, 1.0f }; Why such incredibly precise values here (especially given that these values have more precision than floats can store)? >+ int i; >+ for(i = 0; i < 4; i++) Declare |i| in the loop >+ // The following is Quartz 2D (CoreGraphics) code which >+ // takes care of drawing the a shading. Remove this comment >+ if (shadingFunction == NULL) { if (!shadingFunction) >+ NS_ERROR("Failed to create a shading function."); Use NSLog >+ // Start at the top midpoint >+ float startX = NSMidX(bounds); >+ float startY = NSMaxY(bounds); >+ >+ // End at the bottom midpoint >+ float endX = NSMidX(bounds); >+ float endY = NSMinY(bounds); Use CGPointMake here, rather than storing the intermediate values. >+ if (colorspace == NULL) { >+ NS_ERROR("Failed to create a color space for the shading."); ... >+ if (shading == NULL) { >+ NS_ERROR("Failed to create the shading."); As above, !foo and NSLog >+ // Obtain the current graphics context and represent it as a CFContextRef ... >+ // Now draw the shading ... >+ // Finished drawing the Quartz shading. > > // Call our base class method to paint contents Remove these comments (again, the code reads pretty much like the comments) And just to toss this out there since it's ending up in this patch: after playing around with it, I'm not liking the blue. It makes it feel like popups are good and it's a friendly message about them or something. I actually prefer the pre-glassy color, but I know that wasn't making too many friends; I think next meeting we should maybe try to kick colors around one more time.

(In reply to comment #41) Thanks again for taking the time to review Stuart. I've incorporated all changes into the code with the exception of this one: >Can we just enforce a min width on the text field directly? That way its >effect wouldn't vary if we adjust the controls. If the text maintains a certain minimum width the panel must still maintain one for itself, otherwise the controls will eventually overlap the wrapped text just like the original bug. I do agree with this suggestion, however, so maybe you could elaborate on a way to make it work that I'm not thinking of..

(In reply to comment #42) > If the text maintains a certain minimum width the panel must still maintain one > for itself, otherwise the controls will eventually overlap the wrapped text > just like the original bug. But you'd have to resize it a heck of a lot more than before, and at the point where *something* bad has to happen overlapping seems somewhere between marginally better and no worse than clipping the controls.

This latest patch takes into account all comments from the last review. I was able to enforce wrapping on the text field directly. Additionally, the panel now draws a black shading consistent with the look of the panel from our 1.1a1 release. I'm kind of ashamed it took to many patches/comments to introduce text wrapping, but at least it shows we really care about the appearance of our software (and its underlying code)..

(In reply to comment #45) > Created an attachment (id=247194) [edit] > Screenshot of the black shading. This is not really a detraction from this patch, but I thought we agreed that we'd go back to the standard grey close button because the black was hard to see, didn't show state change well, and was inconsistent with the rest of the UI.

Comment on attachment 247193 [details] [diff] [review] Updated patch with black shading Looks great! Small stuff: There are a bunch of tabs in this patch; please convert them to spaces. There's also trailing whitespace on some blank lines, which should be removed. >+ mMessageTextRightStrutLength = [self frame].size.width - (textFieldFrame.origin.x + textFieldFrame.size.width); (textFieldFrame.origin.x + textFieldFrame.size.width) is just NSMaxX(textFieldFrame)? >+ float currentStrutLength = newPanelFrame.size.width - (textFieldFrame.origin.x + textFieldFrame.size.width); Same here. >+ static float beginTopHalf[4] = { 0.364706f, 0.364706f, 0.364706f, 1.0f }; >+ static float endTopHalf[4] = { 0.298039f, 0.298039f, 0.298039f, 1.0f }; >+ static float beginBottomHalf[4] = { 0.207843f, 0.207843f, 0.207843f, 1.0f }; >+ static float endBottomHalf[4] = { 0.290196f, 0.290196f, 0.290196f, 1.0f }; It would be a bit more readable if the value arrays were horizontally aligned (with spaces, not tabs). >+ if (!colorspace) { >+ NSLog(@"Failed to create a color space for the shading."); >+ return; >+ } This error case leaks shadingFunction. >+ if (!shading) { >+ NSLog(@"Failed to create the shading."); >+ return; >+ } And this one leaks shadingFunction and colorSpace. r=me with those things fixed.

Thanks again Stuart. I'm embarrassed I forgot to escape gracefully when an error was caught and had leaked those CG objects - sorry about that! Serves as an example for why multiple/early returns are sometimes cast in a bad light. The CG..Release functions are allowed to have a NULL parameter, unlike CFRelease, so they could all be grouped together and called only once, but I chose to take the slightly more redundant approach (without any crazy bail action) since there's only two objects to release at the most.

Comment on attachment 248171 [details] [diff] [review] small stuff fixed sr=pink

Removes project references to the background image since it won't be used anymore.

Same, but for the branch.

Checked in on trunk and MOZILLA_1_8_BRANCH