Closed Bug 883546 Opened 7 years ago Closed 7 years ago

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

Categories

(SeaMonkey :: Themes, defect)

defect
Not set

Tracking

(seamonkey2.22 fixed)

RESOLVED FIXED
seamonkey2.22
Tracking Status
seamonkey2.22 --- fixed

People

(Reporter: BinOC, Assigned: philip.chee)

References

(Blocks 1 open bug)

Details

(Keywords: modern)

Attachments

(3 files, 3 obsolete files)

Attached image 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.
Blocks: 526210
Keywords: modern
OS: Windows 8 → All
Hardware: x86_64 → All
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..
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.
Attached patch Patch v1.0 Proposed fix. (obsolete) — Splinter Review
This patch adds 2px padding on the left and right of the search widgets in the Addressbook and in the 3pane MailNews windows.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #763253 - Flags: feedback?(email)
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.
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 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.)
Ok.. That makes sense then. Sorry, not used to bugzilla with PRIVS yet :P
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.
> (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 [?] email@mattatobin.com
And change the [?] to [+] (approve) or [-] (disapprove)
Attachment #763253 - Flags: review?(neil)
Attachment #763253 - Flags: feedback?(email) → feedback+
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.
Attachment #763253 - Flags: review?(neil) → feedback+
>>+.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.
Attachment #763253 - Attachment is obsolete: true
Attachment #772073 - Flags: review?(neil)
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?
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.
> 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.
Attachment #772073 - Attachment is obsolete: true
Attachment #772073 - Flags: review?(neil)
Attachment #775340 - Flags: review?(neil)
Attachment #775340 - Flags: review?(neil)
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;
Attachment #775340 - Attachment is obsolete: true
Attachment #775684 - Flags: review?(neil)
Comment on attachment 775684 [details] [diff] [review]
Patch v1.2a merged tweaks for the navigator throbber [check-in Comment 17]

Very neat!
Attachment #775684 - Flags: review?(neil) → review+
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3ba0cc41732a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.22
Attachment #775684 - Attachment description: Patch v1.2a merged tweaks for the navigator throbber. → Patch v1.2a merged tweaks for the navigator throbber [check-in Comment 17]
You need to log in before you can comment on or make changes to this bug.