1534345 - [de-xbl] Migrate searchvalue binding to custom element.

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, task)

Comment 1

[Arshad Khan [:arshad]]

Attachment: searchvalue.patch

Comment 2

[:aceman]

Comment on attachment 9050032 [details] [diff] [review]
searchvalue.patch

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

Thanks:)

::: mailnews/base/search/content/searchTerm.js
@@ +35,5 @@
>          var searchAttribute=this.searchattribute;
>          var searchOperator=this.searchoperator;
>          var searchValue=this.searchvalue;
>  
> +        customElements.upgrade(searchValue);

Is this needed? So far I only had to use .upgrage inside a xbl binding.

::: mailnews/base/search/content/searchWidgets.js
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* global MozXULElement, gFilter, gFilterList */
>  
> +var { MailUtils } = ChromeUtils.import("resource:///modules/MailUtils.jsm");

So which version is preferred? We seem to use {var} and { var } randomly.

@@ +86,5 @@
>  
>      this.appendChild(menulist);
>  
>      document.getAnonymousElementByAttribute(this.closest(".ruleaction"), "class", "ruleactiontype")
> +      .getTemplates(true, menulist);

I think this was OK.

@@ +298,5 @@
> +    // Force initialization of the menulist custom elements first.
> +    const items = this.querySelectorAll("menulist");
> +    for (let item of items) {
> +      customElements.upgrade(item);
> +    }

I think .upgrade() is not needed now that the whole binding is a CE. Also, I found it is wrong at this place in https://bugzilla.mozilla.org/attachment.cgi?id=9050154. So if it makes no problems to you being placed here, it may confirm it has no effect.

@@ +459,5 @@
> +      this.setAttribute("selectedIndex", "2");
> +    } else if (val == Ci.nsMsgSearchAttrib.Date) {
> +      this.setAttribute("selectedIndex", "3");
> +    } else if (val == Ci.nsMsgSearchAttrib.Sender) {
> +      // Since the internalOperator is null, this is the same as the initial state. 

Trailing space.

@@ +626,5 @@
> +
> +  fillInTags() {
> +    let menulist = this.childNodes[5];
> +    // Force initialization of the menulist custom element first.
> +    customElements.upgrade(menulist);

Possibly unneeded.

Comment 3

[Arshad Khan [:arshad]]

Comment on attachment 9050032 [details] [diff] [review]
searchvalue.patch

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

::: mailnews/base/search/content/searchTerm.js
@@ +36,5 @@
>          var searchOperator=this.searchoperator;
>          var searchValue=this.searchvalue;
>  
> +        customElements.upgrade(searchValue);
> +

yes it is needed. The lifecycle evvent of xbl and ce is different. For ce, connectedCallback is called when ce is appended to dom. searchvalue is created by js so calling a method of search value before appending it to dom might need an upgrade call first.

::: mailnews/base/search/content/searchWidgets.js
@@ +9,1 @@
>  

Let Magnus decide.

@@ +91,1 @@
>  

Not sure what you meant here?

@@ +299,5 @@
> +    const items = this.querySelectorAll("menulist");
> +    for (let item of items) {
> +      customElements.upgrade(item);
> +    }
> +

I ll change upgrade call position.

Comment 4

[Arshad Khan [:arshad]]

Comment on attachment 9050032 [details] [diff] [review]
searchvalue.patch

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

::: mailnews/base/search/content/searchTerm.js
@@ +36,5 @@
>          var searchOperator=this.searchoperator;
>          var searchValue=this.searchvalue;
>  
> +        customElements.upgrade(searchValue);
> +

yes it is needed. The lifecycle evvent of xbl and ce is different. For ce, connectedCallback is called when ce is appended to dom. searchvalue is created by js so calling a method of search value before appending it to dom might need an upgrade call first.

::: mailnews/base/search/content/searchWidgets.js
@@ +9,1 @@
>  

Let Magnus decide.

@@ +91,1 @@
>  

Not sure what you meant here?

@@ +299,5 @@
> +    const items = this.querySelectorAll("menulist");
> +    for (let item of items) {
> +      customElements.upgrade(item);
> +    }
> +

I ll change upgrade call position.

Comment 5

[Arshad Khan [:arshad]]

So which version is preferred? We seem to use {var} and { var } randomly.

my linter preferes { var }.

Comment 6

[Arshad Khan [:arshad]]

Attachment: searchvalue.patch

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9050222 [details] [diff] [review]
searchvalue.patch

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

::: mailnews/base/search/content/searchWidgets.js
@@ +221,5 @@
> + * priority, status, junk status, tags, hasAttachment status, and addressbook.
> + *
> + * @extends MozXULElement
> + */
> +class MozSearchvalue extends MozXULElement {

MozSearchvalue is a widget that allows selecting the value to search or filter on. It can be a text entry, priority, status, junk status, tags, hasAttachment status, and addressbook etc.

@@ +233,5 @@
> +    this.addEventListener("keypress", (event) => {
> +      if (event.keyCode != KeyEvent.DOM_VK_RETURN) {
> +        return;
> +      }
> +      onEnterInSearchTerm(event);

undeclared global, what does eslint say?

@@ +237,5 @@
> +      onEnterInSearchTerm(event);
> +    });
> +  }
> +
> +  connectedCallback() {

delay? 

protect against adding children more than once

@@ +310,5 @@
> +    const time = searchAttribute == Ci.nsMsgSearchAttrib.Date ? datePicker.value : new Date();
> +
> +    // Do .value instead of .setAttribute("value", xxx); to work around for bug #179412
> +    // (caused by bug #157210)
> +    //

bug 157210 appears fixed by now so this should be removed and setAttribute value used instead

@@ +344,5 @@
> +    }
> +
> +    const observingElements = Array.from(this.querySelectorAll("[inherits='disabled']"));
> +
> +    observingElements.forEach(elem => {

no need to declare the observing elements, or to make an array. 

this.querySelectorAll("[inherits='disabled']").forEach(....) works just fine

@@ +419,5 @@
> +  }
> +
> +  /**
> +   * parentValue forwards to the attribute.
> +   */

pretty useless doc. maybe just remove it

@@ +622,5 @@
> +    let menulist = this.childNodes[5];
> +    // Force initialization of the menulist custom element first.
> +    customElements.upgrade(menulist);
> +    let tagArray = MailServices.tags.getAllTags({});
> +    for (let i = 0; i < tagArray.length; ++i) {

i++

Comment 8

[Arshad Khan [:arshad]]

undeclared global, what does eslint say?

nothing :/

Comment 9

[Arshad Khan [:arshad]]

Attachment: searchvalue.patch

Comment 10

[Arshad Khan [:arshad]]

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

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9050665 [details] [diff] [review]
searchvalue.patch

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

There's a lot of tests that need adaption, like you see in the try run.

[  INFO -    EXCEPTION: fec.window.document.getAnonymousNodes(...) is null and so on

::: mailnews/base/search/content/searchWidgets.js
@@ +235,5 @@
> +      }
> +      onEnterInSearchTerm(event);
> +    });
> +
> +    this._childrenString = `

please just add this where it's used

@@ +292,5 @@
> +        return;
> +    }
> +    this.textContent = "";
> +
> +    this.appendChild(MozXULElement.parseXULToFragment(this._childrenString));

you're still not doing the hasChildNodes() check

@@ +545,5 @@
> +        children[7].querySelector(`[value="${val.hasAttachmentStatus}"]`);
> +      if (hasAttachmentStatus) {
> +
> +      }
> +        children[7].selectedItem = hasAttachmentStatus;

huh??

Comment 12

[Arshad Khan [:arshad]]

you're still not doing the hasChildNodes() check
if connectedcb is exec again then children will be removed by this.textContent = ""; so no need to check the child stuff.

Comment 13

[Arshad Khan [:arshad]]

eslint should also check the content of search/content directory. It doesn't. Should I open a bug for that and then you can assign it to someone to deal with it.

Comment 14

[Arshad Khan [:arshad]]

another try - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=13ee391514efa14bbe94b7ecaf1b214285f633a0

Comment 15

[Arshad Khan [:arshad]]

Attachment: searchvalue.patch

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9051498 [details] [diff] [review]
searchvalue.patch

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

please change the commit message to say "... searchvalue binding to a custom element search-value"

Anyway, doesn't work. Search Messages is not showing the searchvalue widget at all.

::: mailnews/base/search/content/searchWidgets.js
@@ +3,5 @@
>   * 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/. */
>  
> +/* global MozXULElement, gFilter, gFilterList, onEnterInSearchTerm, convertDateToString */
> +/* global convertPRTimeToString, convertStringToPRTime */

Can you file a follow-up bug to fix/rework these: it looks like dateFormat.js functionality can just be folded into this(?) widget's code, and nowadays should be using Services.intl, like https://searchfox.org/comm-central/rev/1ed80b150864c0b6e953d3d76ca6e45b6753f3c6/mozilla/browser/components/places/content/treeView.js#515

@@ +344,5 @@
> +    if (!this.hasChildNodes()) {
> +      return;
> +    }
> +
> +    Array.from(this.querySelectorAll("[inherits='disabled']")).forEach(elem => {

no need to use Array.from. forEach works with the this.querySelectorAll() results

@@ +625,5 @@
> +    let tagArray = MailServices.tags.getAllTags({});
> +    for (let i = 0; i < tagArray.length; i++) {
> +      const taginfo = tagArray[i];
> +      const newMenuItem = menulist.appendItem(taginfo.tag, taginfo.key);
> +      if (!i) {

while we're here, please make this

if (i == 0) {

@@ +648,5 @@
> +  initialize(menulist, bundle) {
> +    this.fillStringsForChildren(menulist.firstChild, bundle);
> +  }
> +}
> +customElements.define("searchvalue", MozSearchvalue);

since custom elements should really have a dash, let's make it search-value, and MozSearchValue

Comment 17

[Arshad Khan [:arshad]]

Attachment: searchvalue.patch

Comment 18

[Arshad Khan [:arshad]]

Attachment: searchvalue.patch

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9054161 [details] [diff] [review]
searchvalue.patch

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

Looks good, r=mkmelin

::: mailnews/base/search/content/searchWidgets.js
@@ +378,5 @@
> +        this.setAttribute("selectedIndex", "6");
> +      }
> +    }
> +
> +    // If it's not sender, to, cc, alladdresses, or toorcc, we don't care.

to or cc

@@ +645,5 @@
> +    // Force initialization of the menulist custom element.
> +    customElements.upgrade(parentNode);
> +  }
> +
> +  initialize(menulist, bundle) {

I think you do not need the initalize method, just use fillStringsForChildren directly

Comment 20

[Arshad Khan [:arshad]]

Attachment: searchvalue.patch

Comment 21

[Arshad Khan [:arshad]]

Attachment: searchvalue1.patch

Comment 22

[Arshad Khan [:arshad]]

Comment on attachment 9054697 [details] [diff] [review]
searchvalue1.patch

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

Magnus, I need your help here. I can't make some tests pass. Could you please take a look?

::: mail/test/mozmill/folder-display/test-mail-views.js
@@ +75,2 @@
>    // - make sure the constraint is right
> +  savc.assertValue(new elib.Elem(elem), "$label1");

cant validate elem instance of Element  against "$label1" value - I get this error. Any idea whats wrong?

::: mail/test/mozmill/search-window/test-search-window.js
@@ +81,5 @@
>    // XXX I am having real difficulty getting the click/type pair to actually
>    //  get the text in there reliably.  I am just going to poke things directly
>    //  into the text widget. (We used to use .aid instead of .a with swc.click
>    //  and swc.type.)
> +  let searchVal0 = gsv(swc, "searchVal0", 0);

searchVal0 is undefined. In all other cases as well. Any idea what's wrong? what is swc here?

Comment 23

[Magnus Melin [:mkmelin]]

Comment on attachment 9054697 [details] [diff] [review]
searchvalue1.patch

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

::: mail/test/mozmill/folder-display/test-mail-views.js
@@ +75,2 @@
>    // - make sure the constraint is right
> +  savc.assertValue(new elib.Elem(elem), "$label1");

well whatever you get from  savc.assertValue(new elib.Elem(elem) isn't going to be the string $label, but some element lib object

::: mail/test/mozmill/search-window/test-search-window.js
@@ +81,5 @@
>    // XXX I am having real difficulty getting the click/type pair to actually
>    //  get the text in there reliably.  I am just going to poke things directly
>    //  into the text widget. (We used to use .aid instead of .a with swc.click
>    //  and swc.type.)
> +  let searchVal0 = gsv(swc, "searchVal0", 0);

swc is search window controller

I'm not surprised it's not set. You do getElemenbtById for id=searchVal0, but I don't there ever being such a thing set up anywhere (though I could have overlooked it)

what's more, your gsv function doesn't ever return anything at all...

(also, I'd just inline this instead of creating a new gsv function

::: mailnews/base/search/content/searchWidgets.js
@@ +304,5 @@
> +    // Initialize strings.
> +    const bundle = Services.strings.createBundle("chrome://messenger/locale/messenger.properties");
> +
> +    // Initialize the priority picker.
> +    this.fillStringsForChildren(this.childNodes[1].firstChild, bundle);

all these filling in strings should probably also not happen when if the CE is connected a second time

@@ +328,5 @@
> +    // initialize the has attachment status picker
> +    this.fillStringsForChildren(this.childNodes[7].firstChild, bundle);
> +
> +    // initialize the junk score origin picker
> +    this.fillStringsForChildren(this.childNodes[8].firstChild, bundle);    

nit: trailing space

Comment 24

[Arshad Khan [:arshad]]

Fixed all the test failures - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=46f82261e9147ace5fd43aee509def3bd5900f68 . I can't run macos test atm due to Bug 1541131.

Comment 25

[Arshad Khan [:arshad]]

Attachment: searchvalue.patch

Comment 26

[Arshad Khan [:arshad]]

Attachment: searchvalue.patch

Comment 27

[Arshad Khan [:arshad]]

Attachment: searchvalue.patch

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

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 9056437 [details] [diff] [review]
searchvalue.patch

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

I notice at least creating a filter for Priority doesn't show the priority values in the filter listing

::: mail/test/mozmill/folder-display/test-mail-views.js
@@ +63,5 @@
>    // - make sure the name is right
>    savc.assertValue(savc.eid("name"), baseFolder.prettyName + "-Important");
>  
> +  var elem = savc.window.document.getElementById("searchVal0");
> +  var index = 0;

please use let

::: mail/test/mozmill/search-window/test-search-window.js
@@ +72,5 @@
>    //  get the text in there reliably.  I am just going to poke things directly
>    //  into the text widget. (We used to use .aid instead of .a with swc.click
>    //  and swc.type.)
> +  var searchVal0 = swc.window.document.getElementById("searchVal0");
> +  var index = 0;

please use let (like it was)
also I

@@ +86,5 @@
>    let plusButton = swc.eid("searchRow0", {tagName: "button", label: "+"});
>    swc.click(plusButton);
>  
>    // - put "bar" in it
> +  var searchVal1 = swc.window.document.getElementById("searchVal1");

let, like it was

Comment 29

[Magnus Melin [:mkmelin]]

Perhaps due to
JavaScript error: chrome://messenger/content/searchWidgets.js, line 469: TypeError: valueBox is undefined

Comment 30

[Arshad Khan [:arshad]]

Attachment: searchvalue.patch

Comment 31

[Arshad Khan [:arshad]]

There was no error, the problem was the xbl anon child items are not picked as legit child items via js and css selectors.

Comment 32

[Arshad Khan [:arshad]]

new try - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=07910d371758aaaa107968cb5c955fdfb2111027

Comment 33

[Magnus Melin [:mkmelin]]

Comment on attachment 9056726 [details] [diff] [review]
searchvalue.patch

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

Looks ok now. r=mkmelin
(There's some bitrot though)

::: mail/test/resources/mozmill/mozmill/extension/content/modules/controller.jsm
@@ +886,5 @@
>    return true;
>  };
>  
>  // Assert that a form element contains the expected value
> +MozMillController.prototype.assertValue = function(el, value, isHTMLElement) {

please remove this unnecessary change

Comment 34

[Arshad Khan [:arshad]]

Attachment: searchvalue.patch

Comment 35

[Jorg K (CEST = GMT+2)]

This seems to be conflicting with bug 1513836.

Comment 36

[Arshad Khan [:arshad]]

Attachment: searchvalue.patch

Comment 37

[Arshad Khan [:arshad]]

new try - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=955284228c3e227a5c7a12169ed4224e332ecf8f

Comment 38

[Arshad Khan [:arshad]]

Looks like the test failures are not related to this patch?

Comment 39

[Jorg K (CEST = GMT+2)]

Correct, busted by bug 1544013. Not an easy one to fix. What you need to know it that two Mozmill tests fail in account creation. I might switch them off to give you green Mozmill.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1aad2ba888ec
Migrate searchvalue binding to custom element. r=mkmelin