1373923 - Use SVG icons for the search glass and -close in textboxes

Status: RESOLVED FIXED

(Thunderbird :: Theme, enhancement)

Description

[Richard Marti (:Paenglab)]

FX uses a SVG icon in the searchbar. To be consistent we should use such a icon too. On Linux we still use a GTK icon which not really fits to our toolbar icons. On Windows the blue icon does also not match so good with our toolbar icons.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: searchIcons.patch

macOS needs no change as the system icons still fit (and the search glass is fix in the searchbox and can't be exchanged ;-) ).

Aceman can you check Linux and the CSS?
Jörg, can you check it on Windows?

Comment 2

[Richard Marti (:Paenglab)]

The opacity of 0.54 is used to be in sync with the placeholder text: https://dxr.mozilla.org/comm-central/source/mozilla/layout/style/res/forms.css#227

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8878774 [details] [diff] [review]
searchIcons.patch

When I hover the Gloda glass, it goes darker, the Quicksearch glass doesn't go darker. But then, the functionality is different, Quicksearch starts straight away, for Gloda you need to click or hit enter.

When the Gloda textbox is empty, the glass still goes darker but clicking it has no effect. That doesn't appear right, but I don't know whether you can change the style depending on whether the search is empty or not.

Nit: You've added empty lines to the end of the file. Please remove in the next loop or I remove at checkin, in case the Gloda thing can't be changed.

Comment 4

[Richard Marti (:Paenglab)]

The selector has .gloda-search-icon:not([disabled]):hover, but the icon is never disabled. Before my change the selector was .gloda-search-icon[searchbutton]:not([disabled]):hover where searchbutton was never defined and so this rule was never used. There was never a hovered state before my patch.

To fix this, we should add the [disabled] functionality in a followup bug in JS. But I look, if I can figure it out and add it here. But no promise.

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8878774 [details] [diff] [review]
searchIcons.patch

Clearing review for now as there might be a better solution coming. It's less then ideal that the glass appears to be active when the search box is empty.

Comment 6

[Richard Marti (:Paenglab)]

Jörg, if you can direct me where I can set the disabled state depending if the textfield is empty, I can try to add it in this bug. If not, there is no better solution for this bug and needs to be done in a new bug.

Comment 7

[Jorg K (CEST = GMT+2)]

I don't know, I'd have to dig through it. The surrounding textbox
https://dxr.mozilla.org/comm-central/rev/484beb60833c61561f1c447b972e7d4f55e52633/mail/base/content/extraCustomizeItems.xul#32
uses autocomplete, it somehow knows whether the search is empty or not, since <enter> behaves differently.

I hope Aceman can help when he comes to review this.

Comment 8

[Richard Marti (:Paenglab)]

When I set the gloda-search-icon to disabled="true" then it works as before my patch. A click on it still starts the search. Maybe adding oninput="enable the icon command" on the textbox could mimic the needed behavior.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: searchIcons.patch

This patch activates the search glass when you enter a text. But unfortunately I see no solution to deactivate it again when the field is empty.

Comment 10

[:aceman]

Comment on attachment 8880534 [details] [diff] [review]
searchIcons.patch

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

::: mail/base/content/extraCustomizeItems.xul
@@ +43,5 @@
>                   placeholder=""
>                   emptytextbase="&search.label.base1;"
>                   keyLabelNonMac="&search.keyLabel.nonmac;"
>                   keyLabelMac="&search.keyLabel.mac;"
> +                 oninput="document.getElementById('glodaSearchIcon').setAttribute('disabled', false);">

If you want to set back disabled=true if there is no text int he textbox, try:
oninput="document.getElementById('glodaSearchIcon').setAttribute('disabled', (document.getElementById('searchInput').value == '' ? 'true' : 'false'));">

But this still has a problem: if you click the searchglass icon to run gloda search a new tab is opened. When you click back to the original tab, the textbox is cleared by code. In that case oninput seems not to run thus disabled=true will not be set. Switching the tabs to the gloda tab fills the textbox again, seems to be a feature.

So if you want to toggle disabled too, you need to find an event to hook onto which runs also when textbox is touched by code, not user.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8880534 [details] [diff] [review]
searchIcons.patch

You can do:
oninput="document.getElementById('glodaSearchIcon').setAttribute('disabled',
  document.getElementById('searchInput').textLength==0 ? 'true' : 'false');">
<hbox>
<image id="glodaSearchIcon"
  class="gloda-search-icon"
  onclick="document.getElementById('glodaSearchIcon').setAttribute('disabled', 'true'); document.getElementById('searchInput').doSearch();"
  disabled="true"/>

I prefer .textLength.

Comment 12

[Jorg K (CEST = GMT+2)]

Grr, that's still not right. On the Gloda tab itself, no input has run yet, so the glass doesn't highlight although the box is populated.

Comment 13

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #12)
> Grr, that's still not right. On the Gloda tab itself, no input has run yet,
> so the glass doesn't highlight although the box is populated.

It shows the actual search. Without change it makes not much sense to search the same again.

Comment 14

[Jorg K (CEST = GMT+2)]

Sure, but the UI is still wrong.

Comment 15

[Richard Marti (:Paenglab)]

Attachment: searchIcons.patch with mutationObserver

Aceman proposed over IRC to use a mutationObserver. I tried this now only in main window.

The problem is, "textLength" isn't a attribute. What do I need to check, if the textLength changes?

To check my code I used "focused" as attribute and then it worked by changing the text and then defocus/focus the textfield, but this is clearly no solution.

Comment 16

[Jorg K (CEST = GMT+2)]

You'd need to check the value attribute.

Comment 17

[Richard Marti (:Paenglab)]

"value" doesn't work. Checking the textbox and it's children with DOMi shows never a value attribute.

Comment 18

[:aceman]

Ah yes, user's entered text is in the 'value' property, not attribute (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/textbox).

Can you try listening to the child element changes?
https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver, e.g. 'childList: true' and the other arguments you can listen to in mutation observer.

Comment 19

[Richard Marti (:Paenglab)]

Hmm, I can't make it work. I tried theis:

function checkGlodaInput() {
  // Observe if the textfield has content to enable the glodaSearchIcon
  let searchObserver = new MutationObserver(function(mutations) {
    mutations.forEach(function(mutation) {
      if (mutation.type == "attributes" && mutation.attributeName == "value") {
        document.getElementById('glodaSearchIcon').setAttribute('disabled',
        document.getElementById('searchInput').textLength==0 ? 'true' : 'false');
      }
    });
  });
  searchObserver.observe(document.getElementById("searchInput"), {
    childList: true,
    characterData: true,
    attributes: true,
    subtree: true,
    characterData: true });
}

What could I do?

Comment 20

[Jorg K (CEST = GMT+2)]

I did a bit of debugging. The mutation observer reports changes of three attributes:
focused, parentfocused and nomatch, none of which are useful.

Especially when I remove the last character from the search box, which would be the moment to disable the glass, nothing gets reported.

Comment 21

[:aceman]

Does it report any change if the textbox is cleared by code (switching away from the gloda tab)?

Comment 22

[Richard Marti (:Paenglab)]

Attachment: patch.png

Because the mutationObserver doesn't work, I implemented the proposals fron comment 10 and 11.

What do you think, is this patch landable like this or should I remove the hover state for the gloda search magnifier like we have now without the patch?

Comment 23

[Richard Marti (:Paenglab)]

Attachment: searchIcons.patch

Oops, this is the correct patch.

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8882877 [details] [diff] [review]
searchIcons.patch

As far as I understand, this solution is a compromise: At first the glass is not enabled, but when you type, it becomes enabled. If you delete all text, is doesn't become disabled again (or maybe it does).

In any case, once you trigger the search, a new tab is opened. Returning to the main tab, the glass remains active from the previous search, although the box was cleared.

I think this is inconsistent and confusing. If we can't get it to work 100% correctly, it should at least be simple and consistent. So the only consistent solution I can see so far is to have the glass always active. If the box is empty, clicking it won't start a search, and that case will be rare since the user will want to search for something.

So I suggest to go back to the very first patch in the series.

Comment 25

[Richard Marti (:Paenglab)]

Attachment: searchIcons.patch

This is v1 with a missing semicolon fixed.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8883516 [details] [diff] [review]
searchIcons.patch

OK, thanks. I'll land it with the last hunk removed since that adds two empty lines at the end of mail/themes/windows/mail/searchBox.css as pointed out in comment #3.

Comment 27

[Richard Marti (:Paenglab)]

Ah yes. Overseen this. Thanks

Comment 28

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/081f746eb2e888485b6c4a3cd9e96aa82969f7d0

Comment 30

[Paul Menzel]

Could this please be backported to Thunderbird 52.x? Should a new bug report be created for that?

Comment 31

[Richard Marti (:Paenglab)]

No, normally we don't do style changes in maintenance releases. Especially on Linux it's a change from Linux system theme icon to our own.

Comment 32

[Paul Menzel]

(In reply to Richard Marti (:Paenglab) from comment #31)
> No, normally we don't do style changes in maintenance releases. Especially
> on Linux it's a change from Linux system theme icon to our own.

Do you have a hint for users stuck with 52.x, what system theme is used. I configured Xfce 4.12 to use Adwaita, but Mozilla Thunderbird seems to use something else.

Comment 33

[Richard Marti (:Paenglab)]

It uses the find icon of the icon theme.

You could try to use following code in userChrome.css with a bigger icon which is then downscaled:

.gloda-search-icon {
  list-style-image: url(moz-icon://stock/gtk-find?size=toolbar") !important;
  width: 16px !important;
  height: 16px !important;
}

Not tried, only a proposal for testing.

Comment 34

[Paul Menzel]

I created `~/.thunderbird/ffh170i9.default/userChrome.css` with that content, and nothing changed.

```
$ more ~/.thunderbird/ffh170i9.default/userChrome.css
.gloda-search-icon {
  list-style-image: url(moz-icon://stock/gtk-find?size=toolbar") !important;
  width: 16px !important;
  height: 16px !important;
}
```

No idea, what I did wrong. After removing the package `gnome-icon-theme`, that icon shouldn’t even be displayed anymore.

Comment 35

[Richard Marti (:Paenglab)]

(In reply to Paul Menzel from comment #34)
> I created `~/.thunderbird/ffh170i9.default/userChrome.css` with that
> content, and nothing changed.

userChrome.css needs to be in ~/.thunderbird/ffh170i9.default/chrome

> No idea, what I did wrong. After removing the package `gnome-icon-theme`,
> that icon shouldn’t even be displayed anymore.

All other programs, which used the system icons, have now no icons? Or do they fall back to a default icon set, and TB too?