Duplicate "Search" text in searchbox

RESOLVED FIXED in Thunderbird 45.0

Status

Thunderbird
Theme
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aleth, Assigned: Paenglab)

Tracking

({regression})

Trunk
Thunderbird 45.0
regression

Thunderbird Tracking Flags

(thunderbird45+ fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8670198 [details]
Searchbox.png

This is a regression (maybe OS X only?).
(Reporter)

Updated

2 years ago
Keywords: regression
Version: unspecified → Trunk
(Reporter)

Comment 1

2 years ago
Still seeing this.
status-thunderbird45: --- → affected
tracking-thunderbird45: --- → ?
(Reporter)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
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.
(Assignee)

Comment 4

2 years ago
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..."?
(Reporter)

Comment 5

2 years ago
Created attachment 8692683 [details]
Screenshot without search.label.base
(Reporter)

Comment 6

2 years ago
(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).
(Assignee)

Comment 7

2 years ago
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.
(Reporter)

Comment 8

2 years ago
(In reply to Richard Marti (:Paenglab) from comment #7)
> Can it be this is only visible in HiDPI mode?

no, that's not it.
(Assignee)

Comment 9

2 years ago
Created attachment 8693217 [details] [diff] [review]
OSXSearchbox.patch

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)
(Reporter)

Comment 10

2 years ago
Created attachment 8693237 [details]
Screenshot with patch
(Reporter)

Comment 11

2 years ago
Created attachment 8693238 [details]
Firefox for comparison
(Reporter)

Comment 12

2 years ago
(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?
(Reporter)

Comment 13

2 years ago
(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?
(Assignee)

Comment 14

2 years ago
Created attachment 8693332 [details] [diff] [review]
OSXSearchbox.patch v2

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)
(Reporter)

Comment 15

2 years ago
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.

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
(Reporter)

Comment 16

2 years ago
(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).
(Reporter)

Comment 17

2 years ago
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?
(Assignee)

Comment 18

2 years ago
Created attachment 8693343 [details] [diff] [review]
OSXSearchbox.patch v3

(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)
(Reporter)

Comment 19

2 years ago
(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" ;)
(Reporter)

Comment 20

2 years ago
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+
(Reporter)

Comment 21

2 years ago
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 ;)
(Reporter)

Updated

2 years ago
Keywords: checkin-needed
(Reporter)

Comment 22

2 years ago
https://hg.mozilla.org/comm-central/rev/6b6dec0b603df40f8b06f865fe2fbb0efb9b5d2f
Bug 1211864 - Use our own searchBox styling. r=aleth
(Reporter)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0

Comment 23

2 years ago
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! :-)

Updated

2 years ago
Blocks: 1236164

Updated

2 years ago
status-thunderbird45: affected → fixed

Updated

2 years ago
tracking-thunderbird45: ? → +
You need to log in before you can comment on or make changes to this bug.