The contextual dialog should not have a 3D border. Mockup available at: http://people.mozilla.com/~faaborg/files/granParadisoUI/places_NewBookmarkDialog_i9.png

this one appears to be a bug in XUL:Panel for windows I can't style this border at all, I can change the thickness of it, but no border colors.

-moz-border-*-colors seems to work for me.

Requesting blocking as this looks now unpolished when compared with the identity popup at the other end of the address bar (cf. <http://img219.imageshack.us/my.php?image=bevelproblemcu1.png>).

Simply setting the -moz-appearance to menupopup ought to make it have the correct 1px border on Aero/Luna and the correct 2px bevel on Classic... (though you'd probably have to set the child containers' backgrounds on Vista to get rid of the vertical rule that would be drawn, but that's trivial)

>Simply setting the -moz-appearance to menupopup ought to make it have the >correct 1px border on Aero/Luna and the correct 2px bevel on Classic... I recommend we do that for Luna and classic. For Aero we should use this: #identity-popup{ -moz-appearance: tooltip !important; } #identity-popup-container{ background-color: transparent !important; } Image shows what this looks like.

Sorry misread the bug, same thing that applies to the identity dialog should also apply to the bookmark contextual dialog, for all os themes.

(In reply to comment #6) > Sorry misread the bug, same thing that applies to the identity dialog should > also apply to the bookmark contextual dialog, for all os themes. > Uuuhhhh... that's actually very good-looking. Why not change the identity dialog to use a tooltip appearance on Vista Aero instead of making this conform to that? If it's too much hassle to reland that bug, we could just roll it up into this one (assuming of course that people agree with such a change, but I doubt anyone would argue with that)

>Why not change the identity dialog to use a tooltip appearance on Vista Aero >instead of making this conform to that? Regardless of which patch includes the change, I would like the bookmarks contextual dialog and the identity contextual dialog to both use menupopup on Luna and classic, and for both contextual dialogs to use tooltip on Aero. >but I doubt anyone would argue with that you would be surprised :)

FWIW, these two items on Mac have no border.

What this does: * For Classic, render bookmark & Larry popups as menupopup (2px bevel border) * For Luna, render bookmark & Larry popups as menupopup (1px flat border) * For Aero, render bookmark & Larry popups as tooltip (shiny and pretty!) * Remove hard-coded "color: black" from the Larry popup (it was making the text invisible in the black high-contrast mode, and it was redundant since color: MenuText is already inherited) Note that this patch touches both the bookmark and Larry popups. I rolled them up into one patch because they're closely related (we want both of them to have the same style, and they're both address bar popups) and because bug 414698 is already closed. If anyone thinks that this is a bad idea and that bug 414698 should be reopened and a separate patch made for that, let me know...

Larry and bookmark popups in Vista, Luna, Classic, and High Contrast.

(In reply to comment #11) > Created an attachment (id=316610) [details] > screenshot > > Larry and bookmark popups in Vista, Luna, Classic, and High Contrast. Looks like this has the effect, on Vista, of rounding the corners of the Larry popup, which seems to me to break the visual flow from the button into the popup, but having said all that, it looks pretty hot; a definite improvement. Will a -moz-border-radius-topleft directive let us re-point that one corner without looking bizarre?

(In reply to comment #12) > Looks like this has the effect, on Vista, of rounding the corners of the Larry > popup, which seems to me to break the visual flow from the button into the > popup, but having said all that, it looks pretty hot; a definite improvement. > > Will a -moz-border-radius-topleft directive let us re-point that one corner > without looking bizarre? > That is not possible. Once you set the -moz-appearance to tooltip, the rendering is handed over to uxtheme, and you have no say over how the border is drawn. It's the a trade-off that we can't avoid. My personal opinion is that it's a trade-off that's worth making. Would it help if the Larry panel was shifted to the left by 1px?

(In reply to comment #13) > (In reply to comment #12) > > Looks like this has the effect, on Vista, of rounding the corners of the Larry > > popup, which seems to me to break the visual flow from the button into the > > popup, but having said all that, it looks pretty hot; a definite improvement. > > > > Will a -moz-border-radius-topleft directive let us re-point that one corner > > without looking bizarre? > > > > That is not possible. Once you set the -moz-appearance to tooltip, the > rendering is handed over to uxtheme, and you have no say over how the border is > drawn. It's the a trade-off that we can't avoid. My personal opinion is that > it's a trade-off that's worth making. Would it help if the Larry panel was > shifted to the left by 1px? I was afraid of that. Yeah, played with it a little in image editing software, and I think you're right - 1px left looks slightly more natural, and any more finesse than that is letting perfect be the enemy of good. This is a definite improvement, and worth the trade-off, so please don't let me derail it more than that. :)

Comment on attachment 316609 [details] [diff] [review] patch >Index: browser/themes/winstripe/browser/browser-aero.css >+#identity-popup:-moz-system-metric(windows-default-theme) { >+ -moz-appearance: tooltip; >+} >+ >+/* bookmarking panel */ >+#editBookmarkPanel:-moz-system-metric(windows-default-theme) { >+ -moz-appearance: tooltip; >+} Why are these dependent on windows-default-theme? That is, why would menupopup be better than tooltip for non-default themes, if tooltip is what we like on Vista? (It's a bit weird that we're choosing different system styles on Vista vs. XP based solely on appearance, since a given -moz-appearance should have the same semantics regardless of OS version...) >Index: browser/themes/winstripe/browser/browser.css > #identity-popup-container { >- background-color: Menu; > color: MenuText; Why not remove both?

(In reply to comment #15) > Why are these dependent on windows-default-theme? That is, why would menupopup > be better than tooltip for non-default themes, if tooltip is what we like on > Vista? > > (It's a bit weird that we're choosing different system styles on Vista vs. XP > based solely on appearance, since a given -moz-appearance should have the same > semantics regardless of OS version...) > In general, I think these popups should have the menu panel look; the Vista-Aero case is the one and only exception to that rule. The tooltip appearance is, in general, not desirable. For example, in 2K/XP and in Vista Classic, it has a yellow background by default. Plus, in Vista, there is no telling what tooltips will look like with a third-party Vista theme. So since the use of the tooltip here is really just a special exception to the rule, I think it's prudent to be as conservative as possible in defining its scope and to limit its application to just the case where we know it looks fine--that of Vista using the built-in Microsoft appearance. As for semantics, I think it's imperfect that we are saying menu panel in one case and tooltip in another, but that is partly because Microsoft itself has been expanding on the use of the tooltip appearance in Vista and is now using it for things that didn't use to be tooltips. If we could create an "infopanel" -moz-appearance that rendered like a menu panel pre-Vista and that rendered like a tooltip in Vista, then that'd be ideal, but short of doing that, this CSS hack will have to do. > Why not remove both? > Without it, the panel will use the window text color, not the menu text color. While the two often are set to the same color, that is not necessarily always true, so it has to be set explicitly, either in #identity-popup or #identity-popup-container (that it was set in the latter in the patch was arbitrary).

Comment on attachment 316610 [details] screenshot looks great!

Not a blocker, but we'd take a patch - Gavin, did Kai answer your questions? FWIW, I agree with him, and we've been using -moz-system-metric(default-windows-theme) to mean "optimized for default Vista theme, which is itself pretty special and not adherent to windows theming"

Just a few minor changes: - Per comment 14, shift the Larry popup to the left by 1px so that it matches the curve better. - Moved "color: MenuText" from the child container up to the popup (makes more sense semantically) - Added "color: InfoText" to the Aero tooltip override (same color, just slightly better semantic consistency) - Added comments.

What comment 14 looks like...

Taking bug and changing OS to Windows (since everything about this bug thus far has been Windows-only)

Kai - please see bug 414700 comment 4, where Alex suggests #identity-popup-container{ background-color: transparent !important; } Dunno if that's needed, but since I said that bug will be fixed by this one, I wanted to pass it along. I don't actually see any substantial difference between the screenshot there and yours in comment 20. Thanks for taking this - ETA to patch?

Comment on attachment 317081 [details] [diff] [review] patch v1.1 >Index: browser/themes/winstripe/browser/browser-aero.css >+#identity-popup:-moz-system-metric(windows-default-theme), >+#editBookmarkPanel:-moz-system-metric(windows-default-theme) { >+ -moz-appearance: tooltip; >+ color: InfoText; Is InfoText garanteed to be compatible with "-moz-appearance: tooltip"'s background color? >Index: browser/themes/winstripe/browser/browser.css > #identity-popup { >- background-color: Menu; >+ -moz-appearance: menupopup; > color: MenuText; > } Specifying color without background-color worries me, because there's potential for a mismatch. Is "color: MenuText" necessary here?

(In reply to comment #23) > Is InfoText garanteed to be compatible with "-moz-appearance: tooltip"'s > background color? > It should be. The tooltip XUL widget uses both "-moz-appearance: tooltip" and "color: InfoText". InfoText is the text color used exclusively for tooltips in Windows. > Specifying color without background-color worries me, because there's potential > for a mismatch. Is "color: MenuText" necessary here? > The concern for a mismatch is precisely why I specified it. :) Since we are using a menupopup -moz-appearance, the background will be painted with the Windows system color for menu backgrounds, and the foreground text color to go with that is MenuText. -moz-appearance only paints the borders and the background--it doesn't change the foreground color, so by not setting the foreground color to MenuText, we'll basically be setting a background color without a corresponding foreground color. (In reply to comment #22) > #identity-popup-container{ > background-color: transparent !important; > } > That's not needed. :) The patch gets rid of identity-popup-container's background-color property (since the background is now painted by -moz-appearance), which has the same effect as setting it to transparent.

Comment on attachment 317081 [details] [diff] [review] patch v1.1 Thanks for bearing with me, you obviously know this stuff better than I do!

(In reply to comment #24) > so by not setting the > foreground color to MenuText, we'll basically be setting a background color > without a corresponding foreground color. > After writing those words, I realized that I had forgotten to do that for the bookmark popup... so this update adds a MenuText to the bookmark popup.

Comment on attachment 317170 [details] [diff] [review] patch v1.2 Carring over review from patch v1.1 (r+=gavin)

Comment on attachment 317170 [details] [diff] [review] patch v1.2 a=beltzner, thanks!

Checking in browser/themes/winstripe/browser/browser-aero.css; /cvsroot/mozilla/browser/themes/winstripe/browser/browser-aero.css,v <-- browser-aero.css new revision: 1.5; previous revision: 1.4 done Checking in browser/themes/winstripe/browser/browser.css; /cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v <-- browser.css new revision: 1.205; previous revision: 1.204 done

Alex, beside the 3D border the bookmark contextual dialog and the site identity dialog have a different border schema. The latter one has only a 1px border while the former one has an inset like border. That's a difference to what is shown in attachment 316610 [details]. Shall I file a new bug or do we want to reopen this one?

>The latter one has only a 1px border >while the former one has an inset like border. That's a difference to what is >shown in attachment 316610 [details] Not sure I follow, are you saying that attachment 316610 [details] doesn't match the results of applying the patch?

(In reply to comment #31) > Not sure I follow, are you saying that attachment 316610 [details] doesn't match the > results of applying the patch? Correct. When you open the attachment you will see that different border style is used for both dialogs on XP.

> (In reply to comment #31) > > Not sure I follow, are you saying that attachment 316610 [details] [details] doesn't match the > > results of applying the patch? > > Correct. When you open the attachment you will see that different border style > is used for both dialogs on XP. > Huh? I just downloaded the latest hourly build (the currently nightly doesn't have this patch yet) and ran it with a new profile, and I don't see the problem that you are describing in the screenshot...

I filed a follow up bug 430714 to move the identity dialog up three pixels to avoid a double line appearance.

(In reply to comment #33) > Huh? I just downloaded the latest hourly build (the currently nightly doesn't > have this patch yet) and ran it with a new profile, and I don't see the problem > that you are describing in the screenshot... My mistake. Thanks for clarification. It looks good with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008042407 Minefield/3.0pre => VERIFIED.

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv