1573678 - Implement the Preferences Search Field

Status: RESOLVED FIXED

(Thunderbird :: Preferences, task, P2)

Description

[Alessandro Castellani [:aleca]]

Now that the new Preferences UI landed on trunk (thanks Richard!), it's time to implement the global Search Field like Firefox did to drastically improve its usability.

Search field: https://searchfox.org/mozilla-central/rev/c7e8bc4996f979e5876b33afae3de3b1ab4f3ae1/browser/components/preferences/in-content/preferences.xul#182

JS: https://searchfox.org/mozilla-central/rev/c7e8bc4996f979e5876b33afae3de3b1ab4f3ae1/browser/components/preferences/in-content/findInPage.js#38

It's currently still a textbox, but we should implement it directly as an html:input field.

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: 1573678-search-preferences.patch

I started working on this and I need a bit of feedback and help in figuring out the proper approach.
There are many differences between our implementation of the Preferences panel and m-c which will affect how we want to handle this search feature.

Here's a quick recap of what I discovered and implemented with questions alongside some important points.

html:template VS prefpane

m-c wraps each panel around an html:template element, while we use prefpane.
The template is used to select the entire content and generate a l10n fragment for indexing: https://searchfox.org/mozilla-central/rev/588814f2edddf0e132d77d326ddae50911e8bad1/browser/components/preferences/in-content/preferences.js#57

1. It doesn't seem to work the same with a prefpane, so I wonder, should we convert all our prefpane to html:template?

Initialization of each object

The preferences.js file waits for the DOM to be loaded before calling each panel's object and initialize it through a register_module() method: https://searchfox.org/mozilla-central/rev/588814f2edddf0e132d77d326ddae50911e8bad1/browser/components/preferences/in-content/preferences.js#76
This approach is reused multiple times during search and navigation to ensure that all the panels have been properly loaded and are properly refreshed while searching.
From our end, we're waiting for the paneload event to be triggered before initializing the panel's object.

2. This approach doesn't translate well with the modular approach of the search.
   Should we update our init() methods to emulate m-c?

All strings are managed with Fluent

All the strings are managed via fluent and this seems to be the exclusive way the search method finds its matches: https://searchfox.org/mozilla-central/rev/c7e8bc4996f979e5876b33afae3de3b1ab4f3ae1/browser/components/preferences/in-content/findInPage.js#555

3. Should we update all our strings from DTD to Fluent?

selected VS hidden

We're currently showing the various panels using the selected attribute triggered via a radiogroup.
In m-c, a richlistbox is used to display the various Panels' links, and the hidden attribute is used to display the currently selected section.

4. Should we follow this approach?

JS loaded after the content

We're currently loading our javascript after the content in the aboutPreferences.xul. I guess it's because some elements can't be selected before the page is rendered, like the prefPane.
This delayed loading is unusual and it doesn't play well with the search implementation.

5. Should we update the javascript in order to initialize an object after the DOM is loaded, therefore moving the JS scripts above the content?

---

I'm uploading this temp patch just as a visual reference for everything I talked, but please ignore it and don't review it as it's nowhere near working or even well done. It's a temporary patchwork of m-c code adapted to our base to see what works and what doesn't.

I'm pulling in also Richard because of his experience with the preferences section, and Paul because of his experience with the implementation of the m-c appmenu.

Comment 2

[Richard Marti (:Paenglab)]

(In reply to Alessandro Castellani (:aleca) from comment #1)

html:template VS prefpane

m-c wraps each panel around an html:template element, while we use prefpane.
The template is used to select the entire content and generate a l10n fragment for indexing: https://searchfox.org/mozilla-central/rev/588814f2edddf0e132d77d326ddae50911e8bad1/browser/components/preferences/in-content/preferences.js#57

1. It doesn't seem to work the same with a prefpane, so I wonder, should we convert all our prefpane to html:template?

I planned to move to the FX implementation. That's also why I added the data-category="" in the prefs re-shuffling bug.

All strings are managed with Fluent

All the strings are managed via fluent and this seems to be the exclusive way the search method finds its matches: https://searchfox.org/mozilla-central/rev/c7e8bc4996f979e5876b33afae3de3b1ab4f3ae1/browser/components/preferences/in-content/findInPage.js#555

3. Should we update all our strings from DTD to Fluent?

The FX search also worked before the Fluent conversion. But we should convert too, then it could be easier to use the FX JS for the search.

selected VS hidden

We're currently showing the various panels using the selected attribute triggered via a radiogroup.
In m-c, a richlistbox is used to display the various Panels' links, and the hidden attribute is used to display the currently selected section.

4. Should we follow this approach?

Following was my plan. Great when you have time to start with.

Comment 3

[Alessandro Castellani [:aleca]]

Thank you so much for the feedback Richard.
It seems like we both agree we should follow the FX implementation.

How do you want to handle this?
I can take care of it in this bug if you're busy with other tasks, or if you want to take care of it I can definitely wait.

Comment 4

[Richard Marti (:Paenglab)]

Actually I have not much time for such a project. It's okay for me when you can do it.

Comment 5

[Alessandro Castellani [:aleca]]

Sounds good, I'll take care of it right away.
I'll use this bug to track it and upload separated patches for:

• UI update
• Fluent update
• Search implementation

Wish me luck!

Comment 6

[Paul Morris [:pmorris]]

I think following the m-c approach on all these makes sense -- html:template, init(), fluent, hidden, js loading. It will be really nice to have a searchable prefs UI.

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: 1573678-preferences-search-strings.diff

This mini patch implements only the necessary strings for the search field and search results page.
This patch is build on top of bug 1615501, and should land only after the migration of bug 1615501.

Uploading this in order to land strings before the June 1st deadline.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9152836 [details] [diff] [review]
1573678-preferences-search-strings.diff

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

Looks like the Firefox one. r=mkmelin

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/71fc60874af7
Implement the search strings in the Preferences Tab. r=mkmelin

Comment 11

[Magnus Melin [:mkmelin]]

Alex, did you already have some code for this part? (Or pointers)
Maybe this bug should cover converting to <html:template> too, and we can close bug 1615501?

I think Khushil could work on this to free you up for the pgp work.

Comment 12

[Alessandro Castellani [:aleca]]

I have some old patches but they all bitrotted.
Sounds good to me with letting Khushil taking care of this, and indeed we can do the html:template conversion here and close the other bug.

Comment 13

[Khushil Mistry [:khushil324]]

I see some of the DTDs left in general.inc.html: https://searchfox.org/comm-central/search?q=update.checkForUpdatesButton.label&path=
Did we leave them on purpose?

Comment 14

[Alessandro Castellani [:aleca]]

Those strings are also used in the About Dialog and I didn't want to touch that file in the same patch.
I think it's fine to leave them out for now as they're not vital for search purposes, we already have an Updates string in fluent that can be searched, and that whole section is wrapped in the MOZ_UPDATER condition, so not always visible.

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: Bug-1573678_convert-prefpane-to-template-0.patch

There are still a few test failures. Since it is a bigger patch, it will take some time to review so submitting it for the basic code review. Functionality is also working fine but since we have updated the logic, mochitests also need to be updated. I have worked on some of the failures and a few of them are remaining.

Comment 16

[Richard Marti (:Paenglab)]

Khushil, why do you remove all this <html:div>? They are needed to make overflow/expand the <html:fieldset>.

Comment 17

[Khushil Mistry [:khushil324]]

(In reply to Richard Marti (:Paenglab) from comment #16)

Khushil, why do you remove all this <html:div>? They are needed to make overflow/expand the <html:fieldset>.

Ohh, I didn't know that. We can add them back with adding data-category attribute to each <html:div>, functionality will remain the same.

Comment 18

[Alessandro Castellani [:aleca]]

Comment on attachment 9158103 [details] [diff] [review]
Bug-1573678_convert-prefpane-to-template-0.patch

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

Thank you so much for working on this, really massive work.
There are some fixes and changes to do, but overall it looks and works pretty good, great stuff.

Visual issues:
- The width of the main container changes rapidly when searching something, making the whole content and search field move left and right. The width should always be fixed and never move during search.
- Inconsistent width of the various templates, changing section makes the content width jump. The width of all the sections should always be consistent.

The beginning of a search is very slow. When I type the first letter it takes almost 2 seconds to see any visual response and the appearing of the typed letter.

::: calendar/base/content/preferences/alarms.inc.xhtml
@@ -1,4 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, you can obtain one at http://mozilla.org/MPL/2.0/.
> -    <html:div>

Indeed, as pointed by Richard, please don't remove the DIVs around the FIELDSET as we need those to handle overflows and text wrapping. This issue will most likely be solved once we completely drop XUL elements.

::: calendar/test/modules/CalendarUtils.jsm
@@ +630,3 @@
>    utils.waitFor(
> +    () => tab.browser.contentWindow.getCurrentPaneID() == "paneCalendar",
> +    "Timed out waiting for prefpane paneCalendar to load."

We can remove the `prefpane` here since it's not that type of element anymore.

::: mail/components/preferences/general.inc.xhtml
@@ +443,5 @@
>                    data-l10n-id="mail-play-sound-label"
>                    oncommand="gGeneralPane.updatePlaySound();"/>
>          <spacer flex="1"/>
> +        <button is="highlightable-button"
> +                id="playSound"

All these buttons with IDs should follow the the structure `<button id="" is=""..` for greppability

::: mail/components/preferences/preferences.js
@@ +309,5 @@
>  }
>  
>  /**
> + * If there is no visible second level header it will return first level header,
> + * otherwise return second level header.

indent by 2 shitespaces and leave a blank row before the @returns

Comment 19

[Khushil Mistry [:khushil324]]

(In reply to Alessandro Castellani (:aleca) from comment #18)

The beginning of a search is very slow. When I type the first letter it
takes almost 2 seconds to see any visual response and the appearing of the
typed letter.

I don't see this problem. On my machine, it's coming very quickly. Have you tried this with Linux?

All these buttons with IDs should follow the the structure <button id="" is="".. for greppability

We always put is="" first and then id="". Should I put them in one line?

Comment 20

[Alessandro Castellani [:aleca]]

(In reply to Khushil Mistry [:khushil324] from comment #19)

The beginning of a search is very slow. When I type the first letter it
takes almost 2 seconds to see any visual response and the appearing of the
typed letter.

I don't see this problem. On my machine, it's coming very quickly. Have you tried this with Linux?

Yes, I'm on Linux, Ubuntu 18.04
I'll try also on macOS.

All these buttons with IDs should follow the the structure <button id="" is="".. for greppability
We always put is="" first and then id="". Should I put them in one line?

Yes, sorry, I inverted the attributes, please put them inline.

Comment 21

[Khushil Mistry [:khushil324]]

Try the patch from here: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9ba3aa5efda79f0db5b0ac1cfe75e025a98d444c
It has everything except is="" and id="" in one line.
All the test cases are also passing.

Comment 22

[Khushil Mistry [:khushil324]]

Attachment: Bug-1573678_convert-prefpane-to-template-1.patch

Comment 23

[Alessandro Castellani [:aleca]]

Attachment: pref-search.webm

A screencast to show you the issue I'm experiencing.
As you can see at the beginning, when I hit the letter F, it takes almost 2 seconds to show the letter and start the search.
You can see the time difference from when the character appears on screen (meaning the key was pressed) and the character appearing in the search field.

Another issue you can see later in the video is a slight shift of a few pixels depend on the result. That shouldn't happen.

Comment 24

[Alessandro Castellani [:aleca]]

Comment on attachment 9158667 [details] [diff] [review]
Bug-1573678_convert-prefpane-to-template-1.patch

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

Way better than before, thanks for this.

Still, a couple of shifting and timing issues that should be fixed, as highlighted in the previous comment.

Comment 25

[Richard Leger]

(In reply to Alessandro Castellani (:aleca) from comment #23)

Created attachment 9158718 [details]
pref-search.webm

A screencast to show you the issue I'm experiencing.
As you can see at the beginning, when I hit the letter F, it takes almost 2 seconds to show the letter and start the search.

You could use DevTool > Performance to record Performance Report while your reproduce the issue... that may help identify what's causing the 2s delay possibly...

Comment 26

[Richard Marti (:Paenglab)]

Attachment: 1573678-jumping-fix.patch

This should fix the jumping.

This fixes also the different width of the panes and makes them shrink when the window is narrower.

Comment 27

[Alessandro Castellani [:aleca]]

Comment on attachment 9158817 [details] [diff] [review]
1573678-jumping-fix.patch

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

This is good, thanks.

Another small thing we're missing from m-c is the margin-inline variation for first and last child buttons.
If you take a look at the right edge of our prefs, the search field is not perfectly aligned with the buttons as they have an extra 4px.

We should follow what m-c did here: https://searchfox.org/mozilla-central/rev/a87a1c3b543475276e6d57a7a80cb02f3e42b6ed/browser/themes/shared/preferences/preferences.inc.css#416-422

Khushil, do you want to merge these changes into the main patch?

Comment 28

[Khushil Mistry [:khushil324]]

I will merge both the patches including the changes you have suggested for buttons.
About the performance issue, I am not finding any lead. We are following the m-c Implementation. Is this because we are loading too many fluent files and scanning them on each search?

Comment 29

[Alessandro Castellani [:aleca]]

I'm investigating to see if I can find the section impacting the performance.

Heads up, there's a small typo in the comment of the async handleEvent(event) { method.

// Ensure categories are initialized if idle callback didn't run sooo enough.

It should be soon enough

Comment 30

[Khushil Mistry [:khushil324]]

I also see the Performance Report in the DevTool > Performance but I didn't find anything unusual. All the function calls were necessary. Alex, can you inspect the performance report and have the guess? My guess is too many fluent files inspecting the Performance Report.

Comment 31

[Richard Marti (:Paenglab)]

We have not only the buttons but also menulists.

Removing on .sticky-container the margin-inline: -4px; and change the width: calc(100% + 8px); to width: 100%; would also align the searchfield.

Comment 32

[Khushil Mistry [:khushil324]]

In the matchesSearchL10nIDs function in the findInPage.js, we are using the following:

const messages = await document.l10n.formatMessages(
  refs.map(ref => ({ id: ref[0] }))
);

This can impact performance.

Comment 33

[Alessandro Castellani [:aleca]]

I suspect it might be the hiding and revealing of all our child elements.
We're collecting all those elements with

let rootPreferencesChildren = [
        ...document.querySelectorAll(
          "#paneDeck > *:not([data-hidden-from-search]):not(script):not(stringbundle)"
        ),
      ];

And we're looping through them twice, once before the search starts, and again when searching within all the strings.

for (let child of rootPreferencesChildren) {
        if (child.hidden) {
          child.classList.add("visually-hidden");
          child.hidden = false;
        }
      }

I think it might depend on the amount of elements and sections we're dealing with, which is larger than what FF has.

Comment 34

[Alessandro Castellani [:aleca]]

Comment on attachment 9158667 [details] [diff] [review]
Bug-1573678_convert-prefpane-to-template-1.patch

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

::: mail/components/preferences/findInPage.js
@@ +283,5 @@
> +      let ts = performance.now();
> +      let FRAME_THRESHOLD = 1000 / 60;
> +
> +      // Showing or Hiding specific section depending on if words in query are found
> +      for (let child of rootPreferencesChildren) {

Since we're looping all the child elements here, we could potentially remove the previous loop and add the condition
```
if (child.hidden) {
  child.hidden = false;
}
```
As the "visually-hidden" class gets applied and removed based on the results.

Comment 35

[Alessandro Castellani [:aleca]]

Nevermind, ignore my suggestion as that change causes results to not show up if the user types too fast.

Comment 36

[Khushil Mistry [:khushil324]]

Attachment: Bug-1573678_convert-prefpane-to-template-2.patch

I have combined Richard's patch.
Richard, can you also try out the patch?

Comment 37

[Richard Marti (:Paenglab)]

Comment on attachment 9159211 [details] [diff] [review]
Bug-1573678_convert-prefpane-to-template-2.patch

Looks good now. I see no big delay when searching.

Comment 38

[Alessandro Castellani [:aleca]]

Comment on attachment 9159211 [details] [diff] [review]
Bug-1573678_convert-prefpane-to-template-2.patch

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

Amazing work, thank you so much.
I'm still experiencing a little bit of delay when typing the first letter, but it's manageable and we can iterate on this in a follow up bug.

r+ with just a few nits.

::: calendar/base/content/preferences/categories.inc.xhtml
@@ +17,5 @@
> +                          oncommand="gCategoriesPane.addCategory()"
> +                          search-l10n-ids="
> +                            category-name-label,
> +                            category-color-label.label
> +                          "/>

nit: close the tag inline, like we're doing for the checkDefaultButton

::: mail/components/preferences/findInPage.js
@@ +407,5 @@
> +        searchPhrase
> +      );
> +
> +      // Searching some elements, such as xul:label, store their user-visible text in a "value" attribute.
> +      // Value will be skipped for menuitem since value in menuitem could represent index number to distinct each item.

nit: return the comment to not exceed the character limit.

::: mail/components/preferences/general.inc.xhtml
@@ +235,5 @@
> +                      text-encoding-description,
> +                      font-outgoing-email-label.value,
> +                      font-incoming-email-label.value,
> +                      default-font-reply-checkbox.label
> +                    "/>

nit: close tag inline

@@ +256,5 @@
> +                      override-color-label.value,
> +                      override-color-always.label,
> +                      override-color-auto.label,
> +                      override-color-never.label
> +                    "/>

also here

::: mail/components/preferences/privacy.inc.xhtml
@@ +85,5 @@
> +                      permission-can-access-first-party-label,
> +                      permission-can-session-label,
> +                      permission-cannot-label,
> +                      invalid-uri-message,
> +                      invalid-uri-title"/>

nit: reduce indentation by 2

@@ +205,5 @@
> +                    no-master-password-prompt,
> +                    password-os-auth-dialog-message,
> +                    password-os-auth-dialog-message-macosx,
> +                    password-os-auth-dialog-caption
> +                  "/>

nit: close tag inline

Comment 39

[Khushil Mistry [:khushil324]]

Attachment: Bug-1573678_convert-prefpane-to-template-3.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2f55ba03e52475342c9bf1cb685089f470b03fda

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/50d5257d6037
Implement the search strings in the Preferences Tab. r=aleca

Comment 41

[Alessandro Castellani [:aleca]]

Comment on attachment 9159529 [details] [diff] [review]
Bug-1573678_convert-prefpane-to-template-3.patch

[Approval Request Comment]
User impact if declined: High usability impact as we need the search field in the Preferences for 78.
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low

Comment 42

[Wayne Mery (:wsmwk)]

Comment on attachment 9159529 [details] [diff] [review]
Bug-1573678_convert-prefpane-to-template-3.patch

Approved for beta

Comment 43

[Rob Lemley [:rjl]]

Thunderbird 78.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/ab8ec6b48b88

Comment 44

[Rob Lemley [:rjl]]

On c-c:
backout https://hg.mozilla.org/comm-central/rev/50d5257d6037 (landed twice)

On c-b:
backout https://hg.mozilla.org/releases/comm-beta/rev/ab8ec6b48b88 (same thing)

Then....

land on c-c: attachment 9159529 [details] [diff] [review] and uplift to c-beta 79 and c-esr78.

Comment 45

[Jorg K (CEST = GMT+2)]

Wow, so the string got landed again in comment #40 and the main patch never landed :-(

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/610e9db6a88d
Backed out changeset 50d5257d6037 r=rjl
https://hg.mozilla.org/comm-central/rev/1aa606a44d29
Implement the Preferences Search Field. r=aleca

Comment 47

[Rob Lemley [:rjl]]

Thunderbird 79.0b1:
https://hg.mozilla.org/releases/comm-beta/rev/e5261df82711

Comment 48

[Rob Lemley [:rjl]]

Thunderbird 79.0b1:
https://hg.mozilla.org/releases/comm-beta/rev/2649b703cb39

Comment 49

[Rob Lemley [:rjl]]

Comment on attachment 9159529 [details] [diff] [review]
Bug-1573678_convert-prefpane-to-template-3.patch

[Triage Comment]
First the backout:
https://hg.mozilla.org/releases/comm-beta/rev/e5261df82711
	
Then the correct patch:
https://hg.mozilla.org/releases/comm-beta/rev/2649b703cb39

Comment 50

[Alex Ihrig [:Thunderbird_Mail_DE]]

Will this feature be implemented to account settings, too?

Comment 51

[Wayne Mery (:wsmwk)]

(In reply to Alex Ihrig [:Alex_Ihrig] from comment #50)

Will this feature be implemented to account settings, too?

That would be in a completely different component - account manager - and there is no such bug report to implement it https://mzl.la/2VxtAR6

Comment 52

[Richard Marti (:Paenglab)]

And this will be probably not so easy as the pages are loaded dynamically when accessed.

Comment 53

[Wayne Mery (:wsmwk)]

Discussed in chat with Rob and Alex - at this stage to be conservative we'll do this for beta 79 and then eventually 78.something, but skipping 78 beta and 78.0.

Comment 54

[Rob Lemley [:rjl]]

Comment on attachment 9159529 [details] [diff] [review]
Bug-1573678_convert-prefpane-to-template-3.patch

[Approval Request Comment]
Resetting to need approval to match reality.

Comment 55

[Alessandro Castellani [:aleca]]

Bug-1573678_convert-prefpane-to-template-3.patch should have landed on 78.
We don't have a search field in there

Comment 56

[Rob Lemley [:rjl]]

See comment 53. Should be able to get this into 78.0.1.

(In reply to Alessandro Castellani (:aleca) from comment #55)

Bug-1573678_convert-prefpane-to-template-3.patch should have landed on 78.
We don't have a search field in there

Comment 57

[Wayne Mery (:wsmwk)]

I feel bad about deferring this further but unfortunately we didn't get the beta out until today. So sticking with previous rationale, differing until it's actually been in the hands of users for a bit.

Comment 58

[Wayne Mery (:wsmwk)]

Comment on attachment 9159529 [details] [diff] [review]
Bug-1573678_convert-prefpane-to-template-3.patch

Approved for esr78

Comment 59

[Rob Lemley [:rjl]]

Thunderbird 78.1.0:
Backout of duplicate Fluent strings mentioned in comment 49
https://hg.mozilla.org/releases/comm-esr78/rev/8ebf01c70dc3

Comment 60

[Rob Lemley [:rjl]]

Thunderbird 78.1.0:
https://hg.mozilla.org/releases/comm-esr78/rev/19bbf85b853b