883546 - Address Book Toolbar Search Bar visually overlaps line between icons and text labels in large icon mode

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Matt A. Tobin]

Attachment: Address Book Issue

In large icon mode and default toolbar arrangement for the customizable Address Book Toolbar the search bar does not nicely end the line between the icons and text labels for the buttons as the throbber does it simply overlaps them with a transparent background for the search bar. The line should terminate some pixels before the search bar as it does with the throbber in this view.

Image Attachment illustrates issue better.

Comment 1

[Matt A. Tobin]

This also seems to affect mailnews in the same way if the search bar is moved to the main toolbar as well. Likely to affect composer too but..

Comment 2

[Philip Chee]

Putting:
background: url("chrome://communicator/skin/toolbar/prtb-bg-noline.gif") #B1BDC9 repeat-x top; padding: 0px 5px;
on the <toolbaritem id="searchBox" ..> seems to work.

Comment 3

[Philip Chee]

Attachment: Patch v1.0 Proposed fix.

This patch adds 2px padding on the left and right of the search widgets in the Addressbook and in the 3pane MailNews windows.

Comment 4

[Matt A. Tobin]

Only issue I can forsee is the eventual switch to css generated backgrounds MAY impact the line entirely. We could just not do that then. Have to weigh the pros and cons of that decision.

Comment 5

[Matt A. Tobin]

Finally got a flat build going where I can test these things. This looks fantastic, Ratty. Seems to deal with it quite well in any position on the toolbars of mailnews and addressbook.

I'd say send it to Neal for review and inclusion whenever c-c reopens.

Comment 6

[Matt A. Tobin]

Comment on attachment 763253 [details] [diff] [review]
Patch v1.0 Proposed fix.

Review of attachment 763253 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. More in comment 5. (Yeah, I really don't know what to type here... sorry.)

Comment 7

[Matt A. Tobin]

Ok.. That makes sense then. Sorry, not used to bugzilla with PRIVS yet :P

Comment 8

[Matt A. Tobin]

Attachment: Real World Screenshot Shows Fixed as Described

As stated in Comment 1 this fix is the same in mailnews as it is in addressbook. Even though in mailnews the searchbox is not by default on the primaryToolbar.

Still it does the job and helps make modern that much better as all these enhancements and fixes will.

Comment 9

[Philip Chee]

> (Yeah, I really don't know what to type here... sorry.)
You go here:
https://bugzilla.mozilla.org/attachment.cgi?id=763253&action=edit
Scroll down to:
feedback [?] [e-mail]
And change the [?] to [+] (approve) or [-] (disapprove)

Comment 10

[[e-mail]]

Comment on attachment 763253 [details] [diff] [review]
Patch v1.0 Proposed fix.

>+.toolbar-primary > #searchBox,
>+.toolbar-primary > #wrapper-searchBox > #searchBox {
[brand.css just uses .toolbar-primary #throbber-box]

>+  background: url("chrome://communicator/skin/toolbar/prtb-bg-noline.gif") #B1BDC9 repeat-x top;
>+  padding: 0px 2px;
I don't see the benefit of the padding.

>+.toolbar-primary > #search-container,
>+.toolbar-primary > #wrapper-search-container > #search-container {
What about the other controls? After all, they could theoretically be customised on to the primary toolbar. I think the best thing to do here is to define a class for all of the throbbers and search fields and any other miscellaneous item and then we can fix their styling all in one fell swoop.

Comment 11

[Philip Chee]

Attachment: Patch v1.1 Use .toolbaritem-noline class.

>>+.toolbar-primary > #searchBox,
>>+.toolbar-primary > #wrapper-searchBox > #searchBox {
> [brand.css just uses .toolbar-primary #throbber-box]
[That's rather non-performat CSS isn't it?]

>>+  background: url("chrome://communicator/skin/toolbar/prtb-bg-noline.gif") #B1BDC9 repeat-x top;
>>+  padding: 0px 2px;
> I don't see the benefit of the padding.
The textbox in the toolbaritem has a right/left margin of 4px. Adding an additional 2px padding makes it look better subjectively. I've removed the padding though. If someone complains I'll add them back.

>>+.toolbar-primary > #search-container,
>>+.toolbar-primary > #wrapper-search-container > #search-container {
> What about the other controls? After all, they could theoretically be 
> customised on to the primary toolbar. I think the best thing to do here is to 
> define a class for all of the throbbers and search fields and any other 
> miscellaneous item and then we can fix their styling all in one fell swoop.

I'm now using a class of "toolbaritem-noline":

+.toolbar-primary > .toolbaritem-noline,
+.toolbar-primary > toolbarpaletteitem > .toolbaritem-noline {
+  background: url("chrome://communicator/skin/toolbar/prtb-bg-noline.gif") #B1BDC9 repeat-x top;
+}

The items affected are mostly toolbaritems containing textboxes or menulists. There are other toolbaritems that shouldn't have this (e.g. the Junk button) so I've applied styles to only those that need it.

Throbbers don't need any tweaks. Bug 882178 was to fix the navigator throbber when in smalll icons mode.

Comment 12

[Philip Chee]

Comment on attachment 772073 [details] [diff] [review]
Patch v1.1 Use .toolbaritem-noline class.

>      <toolbaritem id="button-search-container"
>                   title="&searchButton.title;"
>                   align="center"
> -                 class="chromeclass-toolbar-additional">
> +                 class="toolbaritem-noline chromeclass-toolbar-additional">
>        <button id="button-search"
>                label="&searchButton.label;"
>                accesskey="&searchButton.accesskey;"
>                tooltiptext="&advancedButton.tooltip;"
>                observes="button_search"
>                oncommand="goDoCommand('button_search')"/>
>        <button id="button-advanced"

Button-search and button-advanced have only 2px margins left and right. It think they could do with 2px more. What do you think?

Comment 13

[[e-mail]]

communicator/toolbar.css would probably be a better place for the CSS.

(In reply to Philip Chee from comment #11)
> Throbbers don't need any tweaks.
Well, it might be possible to merge the tweaks together.

(In reply to Philip Chee from comment #12)
> Button-search and button-advanced have only 2px margins left and right. It
> think they could do with 2px more. What do you think?
They look fine to me.

Comment 14

[Philip Chee]

Attachment: Patch v1.2 Move CSS to communicator/toolbar.css

> communicator/toolbar.css would probably be a better place for the CSS.
Done.

>> Throbbers don't need any tweaks.
> Well, it might be possible to merge the tweaks together.
I'll address this in another bug if possible.

>> Button-search and button-advanced have only 2px margins left and right. It
>> think they could do with 2px more. What do you think?
> They look fine to me.
Ok leaving at 2px.

Comment 15

[Philip Chee]

Attachment: Patch v1.2a merged tweaks for the navigator throbber [check-in Comment 17]

Changes since Patch v1.1
1. Added class="toolbaritem-noline" to various throbbers-box[es] except the one in navigator.xul.
2. Consolidated the toolbaritem-noline CSS to communicator/toolbar.css and merge the navigator-throbber fix.
3. Remove some unnecessary rules made possible with (1) and (2)

> communicator/toolbar.css would probably be a better place for the CSS.
Done.

>> Throbbers don't need any tweaks.
> Well, it might be possible to merge the tweaks together.
I eventually figured out how to do this...

>> Button-search and button-advanced have only 2px margins left and right. It
>> think they could do with 2px more. What do you think?
> They look fine to me.
Ok leaving at 2px.

brand.css:
> #wrapper-throbber-box > #throbber-box > #navigator-throbber,
> #navigator-throbber {

....

This rule is needed to override the rule further up in brand.css:

> +toolbar[mode="icons"] #wrapper-throbber-box > #throbber-box > #navigator-throbber,
>  toolbar[mode="icons"] #navigator-throbber {
>    margin: 4px 8px 2px;

Comment 16

[[e-mail]]

Comment on attachment 775684 [details] [diff] [review]
Patch v1.2a merged tweaks for the navigator throbber [check-in Comment 17]

Very neat!

Comment 17

[Philip Chee]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3ba0cc41732a