Closed Bug 1373923 Opened 4 years ago Closed 4 years ago

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

Categories

(Thunderbird :: Theme, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 56.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
Attached patch searchIcons.patch (obsolete) — Splinter Review
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?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8878774 - Flags: review?(jorgk)
Attachment #8878774 - Flags: review?(acelists)
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 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.
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 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.
Attachment #8878774 - Flags: review?(jorgk)
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.
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.
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.
Attached patch searchIcons.patch (obsolete) — Splinter Review
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.
Attachment #8878774 - Attachment is obsolete: true
Attachment #8878774 - Flags: review?(acelists)
Attachment #8880534 - Flags: review?(acelists)
Attachment #8880534 - Flags: feedback?(jorgk)
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.
Attachment #8880534 - Flags: review?(acelists) → feedback+
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.
Attachment #8880534 - Flags: feedback?(jorgk)
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.
(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.
Sure, but the UI is still wrong.
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.
Attachment #8880534 - Attachment is obsolete: true
Attachment #8881019 - Flags: feedback?(jorgk)
Attachment #8881019 - Flags: feedback?(acelists)
You'd need to check the value attribute.
"value" doesn't work. Checking the textbox and it's children with DOMi shows never a value attribute.
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.
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?
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.
Does it report any change if the textbox is cleared by code (switching away from the gloda tab)?
Attached image patch.png (obsolete) —
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?
Attachment #8881019 - Attachment is obsolete: true
Attachment #8881019 - Flags: feedback?(jorgk)
Attachment #8881019 - Flags: feedback?(acelists)
Attachment #8882876 - Flags: feedback?(jorgk)
Attachment #8882876 - Flags: feedback?(acelists)
Attached patch searchIcons.patch (obsolete) — Splinter Review
Oops, this is the correct patch.
Attachment #8882876 - Attachment is obsolete: true
Attachment #8882876 - Flags: feedback?(jorgk)
Attachment #8882876 - Flags: feedback?(acelists)
Attachment #8882877 - Flags: feedback?(jorgk)
Attachment #8882877 - Flags: feedback?(acelists)
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.
Attachment #8882877 - Flags: feedback?(jorgk)
This is v1 with a missing semicolon fixed.
Attachment #8882877 - Attachment is obsolete: true
Attachment #8882877 - Flags: feedback?(acelists)
Attachment #8883516 - Flags: review?(jorgk)
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.
Attachment #8883516 - Flags: review?(jorgk) → review+
Ah yes. Overseen this. Thanks
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/081f746eb2e888485b6c4a3cd9e96aa82969f7d0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 56.0
Duplicate of this bug: 1421006
Could this please be backported to Thunderbird 52.x? Should a new bug report be created for that?
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.
(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.
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.
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.
(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?
You need to log in before you can comment on or make changes to this bug.