Closed Bug 1211864 Opened 4 years ago Closed 4 years ago

Duplicate "Search" text in searchbox

Categories

(Thunderbird :: Theme, defect)

defect
Not set

Tracking

(thunderbird45+ fixed)

RESOLVED FIXED
Thunderbird 45.0
Tracking Status
thunderbird45 + fixed

People

(Reporter: aleth, Assigned: Paenglab)

References

Details

(Keywords: regression)

Attachments

(4 files, 4 obsolete files)

Attached image Searchbox.png
This is a regression (maybe OS X only?).
Keywords: regression
Version: unspecified → Trunk
Still seeing this.
It looks like the <Cmd+K> part is not doubled, so there is likely both a standard text and a TB-specific text being displayed here.
Note the Firefox search box doesn't include the shortcut key.
I think this is because the searchbox uses -moz-appearance: searchfield; and then the standard text is shown. FX uses it's own binding and styles. If we could switch to such a binding we could also use our own stylings also with the text in it.
I don't see this duplicate text on my VMs "Mountain Lion" and "Yosemite".

Aleth, can you try to change https://dxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messenger.dtd#828 and check if the new text is duplicated or in the background there is still a "Search..."?
Attached image Screenshot without search.label.base (obsolete) —
(In reply to Richard Marti (:Paenglab) from comment #4)
> Aleth, can you try to change
> https://dxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/
> messenger/messenger.dtd#828 and check if the new text is duplicated or in
> the background there is still a "Search..."?

As you can see, setting search.label.base to "" reveals the underlying "Search...".

Note the problem also occurs in the filter box below.

Also, the cursor position is incorrect relative to the ligher "Search...".

Neither of the two stylings seems to match Firefox (including the icon, which is a little less thin in Firefox, but that hardly matters).
Then this is because of the use of -moz-appearance: searchfield;.

Can it be this is only visible in HiDPI mode?

I'll try if I can style this with -moz-appearance: none; like FX is doing.
(In reply to Richard Marti (:Paenglab) from comment #7)
> Can it be this is only visible in HiDPI mode?

no, that's not it.
Attached patch OSXSearchbox.patch (obsolete) — Splinter Review
This patch fixes the searchboxes for #searchInput and .remote-gloda-search with the stylings of FX.

I have already a patch to add a global searchBox class and then style all searchboxes in one file. I first created it for Windows where where the same stylings in many different files. But now its also handy for OS X.

I started a try build with this global class and also the centered date in today-button: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0b6b29ae1982

If you want you can test this build too. I haven't tested the try build but I hope it's working how it should.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8693217 - Flags: review?(aleth)
Attached image Screenshot with patch (obsolete) —
Attached image Firefox for comparison
(In reply to Richard Marti (:Paenglab) from comment #9)
> Created attachment 8693217 [details] [diff] [review]
> OSXSearchbox.patch
> 
> This patch fixes the searchboxes for #searchInput and .remote-gloda-search
> with the stylings of FX.

Looks pretty good already! As you can see from the Firefox screenshot, the margins and vertical centering is still a bit better there.

> I have already a patch to add a global searchBox class and then style all
> searchboxes in one file. I first created it for Windows where where the same
> stylings in many different files. But now its also handy for OS X.

I guess that would also cover the other searchboxes (filterboxes, chat, calendar) that are not touched by this patch?
(In reply to aleth [:aleth] from comment #12)
> (In reply to Richard Marti (:Paenglab) from comment #9)
> > Created attachment 8693217 [details] [diff] [review]
> > OSXSearchbox.patch
> > 
> > This patch fixes the searchboxes for #searchInput and .remote-gloda-search
> > with the stylings of FX.
> 
> Looks pretty good already! As you can see from the Firefox screenshot, the
> margins and vertical centering is still a bit better there.

Actually it might just be that the TB search bar height is incorrect, and that causes the rest?
Attached patch OSXSearchbox.patch v2 (obsolete) — Splinter Review
This fixes the text position. The box has now the same height as the FX box and this makes it align the same. I haven't touched the gap between the search glass and the text to be more like the native search field. Most obvious when you have opened the Quick Filter Bar with its search field.

(In reply to aleth [:aleth] from comment #12)
> > I have already a patch to add a global searchBox class and then style all
> > searchboxes in one file. I first created it for Windows where where the same
> > stylings in many different files. But now its also handy for OS X.
> 
> I guess that would also cover the other searchboxes (filterboxes, chat,
> calendar) that are not touched by this patch?

Yes. And I checked the try build and tweaked now some styles.
Attachment #8693217 - Attachment is obsolete: true
Attachment #8693217 - Flags: review?(aleth)
Attachment #8693332 - Flags: review?(aleth)
(In reply to Richard Marti (:Paenglab) from comment #14)
> This fixes the text position. The box has now the same height as the FX box
> and this makes it align the same. I haven't touched the gap between the
> search glass and the text to be more like the native search field. Most
> obvious when you have opened the Quick Filter Bar with its search field.

Personally, I think it looks better with a larger gap between search icon and search text, as in Firefox.

I'm not sure what you mean by "quick filter bar", as you can see on the screenshot it doesn't seem like the filter bar is touched by the patch yet.

At first I thought the search bar position was still wrong vertically, but as you can see from the screenshot, it's actually the toolbar height which is one or two pixels too big in TB, making it look more "bulky".


> (In reply to aleth [:aleth] from comment #12)
> > > I have already a patch to add a global searchBox class and then style all
> > > searchboxes in one file. I first created it for Windows where where the same
> > > stylings in many different files. But now its also handy for OS X.
> > 
> > I guess that would also cover the other searchboxes (filterboxes, chat,
> > calendar) that are not touched by this patch?
> 
> Yes. And I checked the try build and tweaked now some styles.

Great work, thanks!
Attachment #8692683 - Attachment is obsolete: true
Attachment #8693237 - Attachment is obsolete: true
(In reply to aleth [:aleth] from comment #15)
> At first I thought the search bar position was still wrong vertically, but
> as you can see from the screenshot, it's actually the toolbar height which
> is one or two pixels too big in TB, making it look more "bulky".

Unrelated, but also visible on the screenshot: it looks like the top border between toolbar and tabs is a bit darker in TB as well (but this might be a choice you made).
We could also drop the ellipses (...) from the strings in the searchboxes? I'm not sure what they are there for. It's not like you get a dialog or submenu if you click there?
(In reply to aleth [:aleth] from comment #15)
> Created attachment 8693341 [details]
> Search with patch v2.png
> 
> (In reply to Richard Marti (:Paenglab) from comment #14)
> > This fixes the text position. The box has now the same height as the FX box
> > and this makes it align the same. I haven't touched the gap between the
> > search glass and the text to be more like the native search field. Most
> > obvious when you have opened the Quick Filter Bar with its search field.
> 
> Personally, I think it looks better with a larger gap between search icon
> and search text, as in Firefox.
> 
> I'm not sure what you mean by "quick filter bar", as you can see on the
> screenshot it doesn't seem like the filter bar is touched by the patch yet.

The gap is 4px smaller than the FX search and we would have two search fields close together with different gaps. But you convinced me, I'm using now the FX gap in the hope the global searchBox patch can land in near future. ;)

> At first I thought the search bar position was still wrong vertically, but
> as you can see from the screenshot, it's actually the toolbar height which
> is one or two pixels too big in TB, making it look more "bulky".

Can we do this and the border color in a new bug?

(In reply to aleth [:aleth] from comment #17)
> We could also drop the ellipses (...) from the strings in the searchboxes?
> I'm not sure what they are there for. It's not like you get a dialog or
> submenu if you click there?

Good catch, done in this patch.
Attachment #8693332 - Attachment is obsolete: true
Attachment #8693332 - Flags: review?(aleth)
Attachment #8693343 - Flags: review?(aleth)
(In reply to Richard Marti (:Paenglab) from comment #18)
> > At first I thought the search bar position was still wrong vertically, but
> > as you can see from the screenshot, it's actually the toolbar height which
> > is one or two pixels too big in TB, making it look more "bulky".
> 
> Can we do this and the border color in a new bug?

Of course, I meant it in the sense of "actually this is not a searchbar issue" ;)
Comment on attachment 8693343 [details] [diff] [review]
OSXSearchbox.patch v3

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

Looks great, thanks!
Attachment #8693343 - Flags: review?(aleth) → review+
Comment on attachment 8693343 [details] [diff] [review]
OSXSearchbox.patch v3

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

::: mail/locales/en-US/chrome/messenger/messenger.dtd
@@ +812,5 @@
>  
>  <!-- Quick Search Bar -->
>  <!-- LOCALIZATION NOTE (quickSearchCmd.key):
>       This is actually the key used for the global message search box; we have
> +     not changed

I wonder if anyone remembers how this sentence was supposed to end ;)
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0
Oh, thanks. This has fixed Bug 1184769 for me, which I still was seeing if building with 10.11 SDK (no duplicate text with 10.10 SDK). With this fix I also don't see it anymore if building with 10.11 SDK. Nice! :-)
Blocks: 1236164
You need to log in before you can comment on or make changes to this bug.