Closed Bug 1542720 Opened 8 months ago Closed 3 months ago

[de-xbl] convert the glodaSearch binding

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: mkmelin, Assigned: aleca)

References

Details

(Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test])

Attachments

(2 files, 10 obsolete files)

36.99 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
2.08 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review

The glodaSearch binding can be converted after bug 1534455 is taken care of. Exact direction still unknown.

https://searchfox.org/comm-central/rev/304e74d67edad6b5a27bb62338a36805243d3851/mail/base/content/search.xml#34

Type: defect → task

There is a patch removing the and autocomplete bindings in bug 1534455, not complete atm, but it might in a week or two(?), so we should get started with this soon.

Assignee: nobody → alessandro

Hey Tim, I've loaded the patch from bug 1534455 and I'm trying to extend the AutocompleteInput class in order to create the gloda-search CE.
Are you planning to make it accessible through MozElements or it can only be used as is="autocomplete-input"?
Is there a way for me to extend that class?

::EDIT::

I should add that I tried customElements.get("autocomplete-input")
and I'm getting this error: class heritage customElements.get(...) is not an object or null

Flags: needinfo?(ntim.bugs)

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

Hey Tim, I'm loaded the patch from bug 1534455 and I'm trying to extend the AutocompleteInput class in order to create the gloda-search CE.
Are you planning to make it accessible through MozElements or it can only be used as is="autocomplete-input"?
Is there a way for me to extend that class?

::EDIT::

I should add that I tried customElements.get("autocomplete-input")
and I'm getting this error: class heritage customElements.get(...) is not an object or null

I suspect the problem is that the referenced patch uses setElementCreationCallback for autocomplete-input so the element won't actually be registered if it has never been instantiated in your document. A really quick-and-dirty workaround would be to instantiate an <autocomplete-input> before trying to create the new element. Alternatively, you could directly load autocomplete-input.js with the subscript loader, I'm not sure if that would interact badly with the callback registered from customElements.js

Flags: needinfo?(ntim.bugs)

This is a WIP patch with the initial work.
It requires the loading of bug 1534455 in order to be tested.

It doesn't currently work and I'm uploading this to seek some feedback and guidance on what I'm doing the and proper approach to follow.

Attachment #9077186 - Flags: feedback?(mkmelin+mozilla)
Attachment #9077186 - Flags: feedback?(aswan)
Comment on attachment 9077186 [details] [diff] [review]
1542720-dexbl-glodasearch-WIP.patch

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

::: mail/base/content/search.js
@@ +230,5 @@
> +    }
> +  }
> +
> +  MozXULElement.implementCustomInterface(MozGlodaSearch, [Ci.nsIObserver]);
> +  customElements.define("gloda-search", MozGlodaSearch, { extends: "autocomplete-input" });

Here, you seem to be defining an element with the markup `<autocomplete-input is="gloda-search" />` which doesn't seem like how you're using it at the moment.

I think what you want to do is: `customElements.define("gloda-search", MozGlodaSearch, { extends: "input" });`
and then use it as: `<html:input is="gloda-search" />`
Blocks: 1554637
Attached patch 1542720-dexbl-glodasearch.patch (obsolete) — Splinter Review

Here's a first, partially working patch.
The search fields are visualized properly and no error is triggered.

The autocomplete popup doesn't appear because it's related to bug 1554637.
As usual, this patch can be tested by applying the wip patch from bug 1534455.

I'm asking an early review to tackle potential glaring issues and important code refactor.

Attachment #9077186 - Attachment is obsolete: true
Attachment #9077186 - Flags: feedback?(mkmelin+mozilla)
Attachment #9077186 - Flags: feedback?(aswan)
Attachment #9077233 - Flags: review?(mkmelin+mozilla)
Attachment #9077233 - Flags: review?(geoff)

For menulist-editable, we did this, which adapted for this bug looks like:

// The autocomplete CE is defined lazily. Create one now to get
// autocomplete-input defined, allowing us to inherit from it.
if (!customElements.get("autocomplete-input")) {
  delete document.createXULElement("input", { is: "autocomplete-input" });
}
customElements.whenDefined("autocomplete-input").then(() => {

(In the place of the outer block that prevents leaking.)

Comment on attachment 9077233 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

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

Some work needed but it's minor. It's hard to check without the autocomplete working, so let's come back to this when it's all ready.

::: mail/base/content/glodaFacetTab.js
@@ +9,1 @@
>  var { GlodaMsgSearcher } = ChromeUtils.import("resource:///modules/gloda/msg_search.js");

Move this import into search.js with the others.

::: mail/base/content/mainMailToolbox.inc.xul
@@ -323,5 @@
>                   title="&glodaSearch.title;"
>                   align="center"
>                   flex="1"
>                   class="chromeclass-toolbar-additional">
> -        <textbox id="searchInput"

Where'd the ID go?

::: mail/base/content/messenger.xul
@@ +595,1 @@
>                      <box id="glodaSearchFacets"/>

A bit of digging reveals this box did something for about two months in 2009. Let's get rid of it and the incorrect comment about it.

::: mail/base/content/search.js
@@ +111,5 @@
> +              case "mailnews.database.global.indexer.enabled":
> +              this.inputSearch.glodaEnabled =
> +                Services.prefs.getBoolPref("mailnews.database.global.indexer.enabled");
> +              this.inputSearch.collapsed = !this.inputSearch.glodaEnabled;
> +              break;

Indent these four lines.

@@ +157,5 @@
> +        Services.prefs.addObserver("mailnews.database.global.indexer.enabled",
> +          this._prefObserver);
> +
> +        this.glodaCompleter = Cc["@mozilla.org/autocomplete/search;1?name=gloda"]
> +          .getService(Ci.nsIAutoCompleteSearch).wrappedJSObject;

Line up the dots with the [, like how it was.

@@ +175,5 @@
> +    }
> +
> +    // Remove???
> +    get menupopup() {
> +      return document.getAnonymousElementByAttribute(this, "anonid", "quick-search-menupopup");

Seems like we're using popup now, so yeah remove, I think.

::: mail/components/im/content/chat-messenger.inc.xul
@@ -60,5 @@
>                             title="&amp;glodaSearch.title;"
>                             align="center"
>                             flex="1"
>                             class="chromeclass-toolbar-additional">
> -                <textbox id="IMSearchInput"

This ID (and the other one I commented on) is still in use. There's also a mention in mail/components/im/content/chat.css that needs to be removed.

https://searchfox.org/comm-central/rev/bff4ee41a668a974944fe859a9517ece5987ddd3/mail/base/content/mailWindowOverlay.js#3153

::: mail/themes/osx/mail/compose/messengercompose.css
@@ -1036,5 @@
>    -moz-appearance: textfield !important;
>    margin: 3px;
>  }
>  
> -#searchInput > .textbox-input-box #sidebar {

Neither of these match anything in the compose window. I guess they must've, once. Take them away!

::: mail/themes/osx/mail/searchBox.css
@@ +33,5 @@
>    box-shadow: var(--focus-ring-box-shadow);
>  }
>  
> +.gloda-search,
> +.gloda-search,

Hmm…
Attachment #9077233 - Flags: review?(mkmelin+mozilla)
Attachment #9077233 - Flags: review?(geoff)
Attachment #9077233 - Flags: feedback+
Attached patch 1542720-dexbl-glodasearch.patch (obsolete) — Splinter Review

Thank you so much for the snappy review, I updated everything.

This is at a decent state where the 3 search fields we're using are visible and accessible.
The autocomplete doesn't work yet, so I'm gonna pause this for now and move to bug 1554637.

Attachment #9077233 - Attachment is obsolete: true
Attachment #9077307 - Flags: feedback+
Status: NEW → ASSIGNED
Attached patch 1542720-dexbl-glodasearch.patch (obsolete) — Splinter Review
Attachment #9077307 - Attachment is obsolete: true
Attachment #9077813 - Flags: review?(geoff)
Comment on attachment 9077813 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

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

::: mail/themes/osx/mail/compose/messengercompose.css
@@ +1034,1 @@
>    -moz-appearance: textfield !important;

This looks wrong since HTML inputs can't have children.

I don't know what this rule is about, but if `-moz-appearance: textfield` is supposed to style the input itself, I don't think it's still needed given that this is the default style on inputs.

::: mail/themes/osx/mail/searchBox.css
@@ +31,5 @@
>    border-color: -moz-mac-focusring;
>    box-shadow: var(--focus-ring-box-shadow);
>  }
> +.gloda-search,
> +.gloda-search,

Duplicated selector

::: mail/themes/shared/mail/searchBox.css
@@ +34,5 @@
>  .remote-gloda-search:-moz-lwtheme {
>    background-color: var(--lwt-toolbar-field-background-color, hsla(0,0%,100%,.8));
>    color: var(--lwt-toolbar-field-color, black);
>  }
> +.gloda-search:not([focused="true"]):-moz-lwtheme,

You probably want to switch `[focused="true"]` to `:focus` here and below since `[focused]` is specific to `textbox`

@@ +46,5 @@
>  .themeableSearchBox:-moz-lwtheme:not([disabled]):hover,
>  .remote-gloda-search:-moz-lwtheme:hover {
>    background-color: var(--lwt-toolbar-field-background-color, white);
>  }
> +.gloda-search:-moz-lwtheme[focused="true"],

same here.

@@ +54,5 @@
>    background-color: var(--lwt-toolbar-field-focus, var(--lwt-toolbar-field-background-color, white));
>    color: var(--lwt-toolbar-field-focus-color, var(--lwt-toolbar-field-color, black));
>    border-color: var(--toolbar-field-focus-border-color);
>  }
> +:root[lwt-selection] .gloda-search .textbox-input:-moz-lwtheme::selection,

This selector is wrong. <textbox> used to have anonymous XBL children (including .textbox-input), but that's no longer the case with the HTML input.

`:root[lwt-selection] .gloda-search::selection` should just be fine.

::: mail/themes/windows/mail/searchBox.css
@@ +62,5 @@
>      border-color: ThreeDShadow;
>    }
>  }
>  /* Add margins to show the hover box-shadow */
> +.gloda-search

I think you forgot `,` at the end of line.

@@ +72,5 @@
>  .themeableSearchBox[focused="true"] {
>    border-color: Highlight;
>  }
>  /* special treatment because these boxes are on themable toolbars */
> +.gloda-search

same here.
Comment on attachment 9077813 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

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

::: mail/base/content/mainMailToolbox.inc.xul
@@ +325,5 @@
>                   flex="1"
>                   class="chromeclass-toolbar-additional">
> +        <html:input type="text"
> +                    is="gloda-search"
> +                    id="searchInput"

would put is and id on the first line

::: mail/base/content/search.js
@@ +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/. */

please add a blank line after this

@@ +38,5 @@
>          event.preventDefault();
>          event.stopPropagation();
> +      });
> +
> +      this.addEventListener("keypress", (event) => {

please combine the keypress handlers

@@ +51,5 @@
> +    }
> +    connectedCallback() {
> +      super.connectedCallback();
> +      // @implements {nsIObserver}
> +      this._prefObserver = {

this is named with underscore, and the other observer isn't. For consistency, maybe both without?

@@ +74,1 @@
>          inputSearch: this,

use the => way to declare the methods utilizing inputSearch and you can drop the inputSearch and simply access this

::: mail/base/jar.mn
@@ +71,5 @@
>      content/messenger/aboutDialog.css               (content/aboutDialog.css)
>      content/messenger/converterDialog.css           (content/converterDialog.css)
>      content/messenger/notification.css              (content/notification.css)
>  *   content/messenger/messenger.css                 (content/messenger.css)
> +    content/messenger/search.js                     (content/search.js)

can we name it gloda-search.js
Attached patch 1542720-dexbl-glodasearch.patch (obsolete) — Splinter Review

Here's an updated version.
I'm asking for feedback since a full review it's not possible until bug 1534455 lands.

Attachment #9077813 - Attachment is obsolete: true
Attachment #9077813 - Flags: review?(geoff)
Attachment #9078230 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9078230 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

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

Didn't try it but looks alright to me

::: mail/base/content/gloda-search.js
@@ +170,5 @@
> +      if (this.value) {
> +        let tabmail = document.getElementById("tabmail");
> +        // If the current tab is a gloda search tab, reset the value
> +        //  to the initial search value.  Otherwise, clear it.  This
> +        //  is the value that 3is going to be saved with the current

remove the 3

there is also some strange spacing in here

@@ +171,5 @@
> +        let tabmail = document.getElementById("tabmail");
> +        // If the current tab is a gloda search tab, reset the value
> +        //  to the initial search value.  Otherwise, clear it.  This
> +        //  is the value that 3is going to be saved with the current
> +        //  tab when we switch back to it next.

T

@@ +178,5 @@
> +        if (tabmail.currentTabInfo.mode.name == "glodaFacet") {
> +          // we'd rather reuse the existing tab (and somehow do something
> +          // smart with any preexisting facet choices, but that's a
> +          // bit hard right now, so doing the cheap thing and closing
> +          // this tab and starting over

Please make the sentences capitalized, and dot at the end.

@@ +204,5 @@
> +    }
> +  }
> +
> +  MozXULElement.implementCustomInterface(MozGlodaSearch, [Ci.nsIObserver]);
> +  customElements.define("gloda-search", MozGlodaSearch, { extends: "input" });

Thinking more about it, it should be gloda-autocomplete-input.
It's generally nice when the child element name ends with -parent. At least it makes things more clear.

Would also affect the file name and class naming.
Attachment #9078230 - Flags: feedback?(mkmelin+mozilla) → feedback+
Attached patch 1542720-dexbl-glodasearch.patch (obsolete) — Splinter Review

Thanks for the quick feedback.
I updated the patch, waiting for D33250 to land before asking for a full review.

Attachment #9078230 - Attachment is obsolete: true
Attachment #9078470 - Flags: feedback+

Here's a temporary patch to re-enable the gloda search in the UI.
As per bug 1565075, also this autocomplete suffers from the click issue.

Attachment #9078470 - Attachment is obsolete: true
Attachment #9093461 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9093461 [details] [diff] [review]
1542720-dexbl-glodasearch-Part1.patch

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

::: mail/base/content/gloda-autocomplete-input.js
@@ +18,2 @@
>  
> +customElements.whenDefined("autocomplete-input").then(() => {

you shouldn't need the whenDefined, since above one is created

@@ +66,3 @@
>  
> +    connectedCallback() {
> +      super.connectedCallback();

please set the "is" attribute to make sure it's there.

this element should have some protection from running conectedCallback and setting up observers many times

@@ +69,3 @@
>  
> +      // @implements {nsIObserver}
> +      this.prefObserver = {

Please switch this over to using  defineLazyPreferenceGetter
Attachment #9093461 - Flags: review?(mkmelin+mozilla) → feedback+
Attached patch 1542720-dexbl-glodasearch.patch (obsolete) — Splinter Review

This is ready for a full review.

I fixed the click to search issue by removing the aSubject == this condition as it was always false:
https://searchfox.org/comm-central/rev/db40ceae271610e72a19e9fdcd47e2a74538a2e8/mail/base/content/search.xml#172

Is this correct?

Attachment #9093461 - Attachment is obsolete: true
Attachment #9093670 - Flags: review?(mkmelin+mozilla)

Does mozmill/quick-filter-bar/test-keyboard-interface.js | test-keyboard-interface.js::test_escape_does_not_reach_us_from_gloda_search pass?

Attached patch 1542720-dexbl-glodasearch.patch (obsolete) — Splinter Review

Fixed the test failure, sorry about that.

Attachment #9093670 - Attachment is obsolete: true
Attachment #9093670 - Flags: review?(mkmelin+mozilla)
Attachment #9093677 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9093670 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

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

::: mail/base/content/gloda-autocomplete-input.js
@@ +76,2 @@
>  
> +      XPCOMUtils.defineLazyPreferenceGetter(

you should import XPCOMUtils (or lint isn't happy either?)

@@ +82,5 @@
> +        (pref, oldVal, newVal) => {
> +          if (newVal) {
> +            this.removeAttribute("hidden");
> +          } else {
> +            this.setAttribute("hidden", "hidden");

please use this.toggleAttribute("hidden", !newVal);

@@ +111,5 @@
>  
> +      // @implements {nsIObserver}
> +      this.textObserver = {
> +        observe: (aSubject, aTopic, aData) => {
> +          if (aTopic == "autocomplete-did-enter-text") {

The subject == this check is supposed to make sure this widget doesn't do anything for when text is entered in other autocomplete widgets on the same page. So you should add some check that does that. How, depends on what aSubject is now.
Attachment #9093670 - Attachment is obsolete: false
Attached patch 1542720-dexbl-glodasearch.patch (obsolete) — Splinter Review

The linter wasn't complaining about the missing XPCOMUtils import.
Weird.

Attachment #9093670 - Attachment is obsolete: true
Attachment #9093677 - Attachment is obsolete: true
Attachment #9093677 - Flags: review?(mkmelin+mozilla)
Attachment #9093723 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9093723 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

Not ready for review as the gloda search is triggered from other autocomplete inputs.
Attachment #9093723 - Flags: review?(mkmelin+mozilla)

This works, but I'm not sure it's the right solution.
The subject variable is not the input anymore but a XPCWrappedNative_NoHelper, which it seems to not offer anymore a direct access to the input itself.
The workaround I found is to compare the popup.

Attachment #9093723 - Attachment is obsolete: true
Attachment #9093734 - Flags: review?(mkmelin+mozilla)
Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/95e390183284
temporarily disable Gloda subtest of test-keyboard-interface.js. rs=bustage-fix
Comment on attachment 9093734 [details] [diff] [review]
1542720-dexbl-glodasearch.patch

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

Looks good to me! r=mkmelin
Attachment #9093734 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6308e5a5c75b
Backed out changeset 95e390183284 to re-enable test. a=jorgk
https://hg.mozilla.org/comm-central/rev/95aa8237c44e
[de-xbl] convert the glodaSearch binding. r=mkmelin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0

Kindly run the linter before submitting a patch :-(

Attachment #9093837 - Flags: review?(mkmelin+mozilla) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/395ad4f5654a
Follow-up: Fix linting errors. r=mkmelin
You need to log in before you can comment on or make changes to this bug.