367272 - make findBar (in suiterunner help viewer) look good in modern

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Robert Kaiser]

Bug 360109 added winstripe's findBar.css to modern, we probably want a real modern theme CSS file there for the future probably to make it fit in as well as possible.

Comment 1

[Manuel Reimer]

Attachment: Patch for the jar.mn file

Comment 2

[Manuel Reimer]

Attachment: And the CSS file

As we just have the text box, currently, the fix is pretty easy. I ripped down the file to only the -moz-binding rule, which makes the text box styled using the defult textbox rules. This patch will fix *this* bug. If we ever want to have more than just the textbox, this would require a "real" findBar.css file, but then we would also need several new icons, which curretly not exist for Classic and Modern.

Comment 3

[Manuel Reimer]

Attachment: Fixed CSS file

Added the color for "not found"

Comment 4

[[e-mail]]

(In reply to comment #2)
>As we just have the text box, currently, the fix is pretty easy.
We don't. Ctrl+F invokes the full find bar.

Comment 5

[[e-mail]]

Comment on attachment 269255 [details]
Fixed CSS file

>findbar > .find-field-container > textbox.findbar-textbox {
>  -moz-binding: url("chrome://global/content/bindings/findbar.xml#findbar-textbox");
>}
You don't need such a complex selector. I see pinstripe have simplified their selectors somewhat; I don't know why winstripe still has the complex ones.

Comment 6

[Stefan [:stefanh]]

(In reply to comment #5)
> (From update of attachment 269255 [details])
> >findbar > .find-field-container > textbox.findbar-textbox {
> >  -moz-binding: url("chrome://global/content/bindings/findbar.xml#findbar-textbox");
> >}
> You don't need such a complex selector. I see pinstripe have simplified their
> selectors somewhat; I don't know why winstripe still has the complex ones.
> 

Winstripe is also simplified now ;-)

Comment 7

[Ben Frisch]

Attachment: The Patch v.1 (CSS/Images from Kuden)

This is the Modern jar.mn changes, new images, and findBar.css that should make the find bar in the help viewer look good in Modern.

Comment 8

[Ben Frisch]

Attachment: The Patch v.1.1 (New Files Have 644 Permissions) (CSS/Images from Kuden)

Same code/images as before, but the new files now have 644 file permissions.

Comment 9

[[e-mail]]

Comment on attachment 372390 [details] [diff] [review]
The Patch v.1.1 (New Files Have 644 Permissions) (CSS/Images from Kuden)

Thanks for looking in to this. I don't know how much of this you can fix yourself.

>+.findbar-closebutton {
>+  list-style-image: url("chrome://global/skin/icons/close.gif");
>+  -moz-image-region: rect(0px, 15px, 16px, 0px);
The Modern closebutton doesn't work like this; instead there are separate images for the hover/active states.

>+  border-width: 0;
I'd prefer border: none;

>+findbar {
>+  min-width: 1px;
>+  background-color: #c7d0d9;
>+  background-image: url("chrome://global/skin/toolbar/findbar.png");
>+  background-position: center center;
>+  background-repeat: repeat-x;
>+  border-top: 1px solid #ebf4ff;
>+  border-right: 1px solid #95a0ad;
>+  border-bottom: 1px solid #494f5d;
>+  border-left: 1px solid rgba(225, 233, 244, 0.75);
I don't understand why there's a background image as well as background and border colours... also the border colours don't look quite right (I think the top border might be OK but Modern style is captial letters).

>+.findbar-find-next:hover,
>+.findbar-find-next:hover:active {
>+  -moz-image-region: rect(16px, 16px, 32px, 0px);
>+}
I don't think Modern goes in for hover effects that much.

>+.findbar-highlight[disabled="true"] .toolbarbutton-icon {
>+  opacity: 0.4;
I think that I would prefer this to use a separate image.

>+.findbar-highlight[disabled="true"][checked="true"] .toolbarbutton-text {
>+  color: #c7d0d9;
This should be unnecessary, since we want the colour to be the same as the next/previous buttons.

>+.find-status-icon {
>+  list-style-image: none;
>+  margin-top: 2px;
>+  margin-bottom: 0;
This makes the icon oddly vertically positioned.

>+.findbar-find-status {
>+  margin-top: 0;
>+  margin-bottom: 0;
>+  -moz-margin-start: 2px;
>+  -moz-margin-end: 0;
These don't take effect because the description element's specified margin takes priority.

>+.find-field-container > .findbar-textbox {
I don't see how this gets used.

Comment 10

[Ben Frisch]

Attachment: The Patch v.2

This patch should address most of the issues.  The only problem is that I wasn't sure where to go with the findbar's background except for removing the image and just using the borders in the patch.  Any thoughts?  Should it perhaps be similar to SeaMonkey's status bar or the bookmarks toolbar?

Also, Kuden has an image: close.png (with all of the close buttons in one file) that was used in his version of the patch, is there an interest in using that?

Comment 11

[[e-mail]]

Comment on attachment 372825 [details] [diff] [review]
The Patch v.2

>+.findbar-find-next[disabled="true"] {
>+  -moz-image-region: rect(16px, 16px, 32px, 0px) !important;

>+.findbar-find-previous[disabled="true"] {
>+  -moz-image-region: rect(16px, 32px, 32px, 16px) !important;
These probably don't need to be !important any more, but I guess highlight still needs it, so let's keep it for consistency.

>+.findbar-highlight:active {
I think this should be :hover:active

>+.findbar-find-status {
>+  padding: 2px;
Hmm, looks like this is relying on a layout bug ;-)

>+.find-field-container > .findbar-textbox {
No wonder this confused me - find-field-container was useless and got removed in mozilla-central; you shouldn't need it here either.

>+  -moz-binding: url("chrome://global/content/bindings/findbar.xml#findbar-textbox");
>+  -moz-box-align: center;
>+  min-height: 24px;
>+  padding-top: 0;
>+  padding-bottom: 0;
>+  margin-top: 3px;
>+  margin-bottom: 3px;
>+  -moz-margin-start: 0;
>+  -moz-margin-end: 3px;
And I'm really doubtful of the value of these anyway, but if you want, I'll have another look in your next patch (yes, sorry...).

>diff --git a/suite/themes/modern/global/icons/find.png b/suite/themes/modern/global/icons/find.png
Presumably the blank area isn't a problem because of compression.

>+  skin/modern/global/icons/notfoundpng                             (global/icons/notfound.png)
Typo :-(

Comment 12

[[e-mail]]

(In reply to comment #10)
> This patch should address most of the issues.  The only problem is that I
> wasn't sure where to go with the findbar's background except for removing the
> image and just using the borders in the patch.  Any thoughts?  Should it
> perhaps be similar to SeaMonkey's status bar or the bookmarks toolbar?
I didn't see anything wrong with the background without the image. I forgot to check the borders though...

> Also, Kuden has an image: close.png (with all of the close buttons in one file)
> that was used in his version of the patch, is there an interest in using that?
Probably, but in a separate bug, after this is fixed.

Comment 13

[[e-mail]]

Comment on attachment 372825 [details] [diff] [review]
The Patch v.2

>+  border-bottomtop: 1px solid #EBF4FF;
Typo ;-)

>+  border-right: 1px solid #95A0AD;
>+  border-bottom: 1px solid #494F5D;
I think that the right colour would be right for the bottom too ;-)

>+  border-left: 1px solid rgba(225, 233, 244, 0.75);
Not sure what (if any) the left colour should be but rgba is definitely wrong.

Comment 14

[Ben Frisch]

Attachment: The Patch v.3

I think the border color is best removed, so I removed it, and I should have addressed the other issues.

Also, it appears that the binding is necessary for the findbar to work, and the CSS basically controls the size and padding of the findbar.

Comment 15

[[e-mail]]

(In reply to comment #14)
> Also, it appears that the binding is necessary for the findbar to work
It seems to work fine with the previous version, which had the wrong rule (meaning that the binding wasn't actually applied...)

Comment 16

[[e-mail]]

Comment on attachment 373450 [details] [diff] [review]
The Patch v.3

>+.findbar-textbox {
>+  -moz-binding: url("chrome://global/content/bindings/findbar.xml#findbar-textbox");
>+  -moz-box-align: center;
>+  min-height: 24px;
>+  padding-top: 0;
>+  padding-bottom: 0;
>+  margin-top: 3px;
>+  margin-bottom: 3px;
>+  -moz-margin-start: 0;
>+  -moz-margin-end: 3px;
>+}
Sorry, but I don't like any of this. r=me with it removed.

Comment 17

[Ben Frisch]

Attachment: The Patch v.4

I removed that section in this patch.  The findbar still doesn't work for me without it, but maybe I'm doing something wrong in testing.

Comment 18

[[e-mail]]

Comment on attachment 373582 [details] [diff] [review]
The Patch v.4

Oops, my bad... bug 481427 isn't going to land on 1.9.1 so we need the binding after all.

Comment 19

[Ben Frisch]

Attachment: The Patch v.5

This version of the patch just has the binding without the margins and should be ready for check-in.  Otherwise you could check-in version 3 of the patch which has both the binding and the margins.

Comment 20

[Frank Wein [:mcsmurf]]

Checked into comm-central, changeset d4c0919e5a3f.
Benjamin: When you need a patch checked in, it's best to set "checkin-needed" as keyword in the bug.