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

RESOLVED FIXED in Thunderbird 68.0

Status

enhancement
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: arshad, Assigned: arshad)

Tracking

(Blocks 2 bugs)

Trunk
Thunderbird 68.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 13 obsolete attachments)

50.80 KB, patch
arshad
: review+
Details | Diff | Splinter Review
Assignee

Description

2 months ago
No description provided.
Assignee

Updated

2 months ago
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
Assignee

Updated

2 months ago
Assignee

Comment 1

2 months ago
Posted patch searchvalue.patch (obsolete) — Splinter Review
Attachment #9050032 - Flags: review?(mkmelin+mozilla)

Comment 2

2 months ago
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.
Assignee

Comment 3

2 months ago
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.
Assignee

Comment 4

2 months ago
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.
Assignee

Comment 5

2 months ago

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

my linter preferes { var }.

Assignee

Comment 6

2 months ago
Posted patch searchvalue.patch (obsolete) — Splinter Review
Attachment #9050032 - Attachment is obsolete: true
Attachment #9050032 - Flags: review?(mkmelin+mozilla)
Attachment #9050222 - Flags: review?(mkmelin+mozilla)
Assignee

Updated

2 months ago
Depends on: 1523384
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++
Attachment #9050222 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 8

2 months ago

undeclared global, what does eslint say?

nothing :/

Assignee

Comment 9

2 months ago
Posted patch searchvalue.patch (obsolete) — Splinter Review
Attachment #9050222 - Attachment is obsolete: true
Attachment #9050665 - Flags: review?(mkmelin+mozilla)
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??
Attachment #9050665 - Flags: review?(mkmelin+mozilla) → review-
Assignee

Comment 12

2 months ago

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.

Assignee

Comment 13

2 months ago

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.

Assignee

Comment 15

2 months ago
Posted patch searchvalue.patch (obsolete) — Splinter Review
Attachment #9050665 - Attachment is obsolete: true
Attachment #9051498 - Flags: review+
Assignee

Updated

2 months ago
Attachment #9051498 - Flags: review+ → review?(mkmelin+mozilla)
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
Attachment #9051498 - Flags: review?(mkmelin+mozilla) → review-
Summary: [de-xbl] Migrate searchvalue to custom element. → [de-xbl] Migrate searchvalue binding to custom element.
Assignee

Updated

2 months ago
Blocks: 1539517
Assignee

Comment 17

2 months ago
Posted patch searchvalue.patch (obsolete) — Splinter Review
Attachment #9051498 - Attachment is obsolete: true
Attachment #9054160 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 18

2 months ago
Posted patch searchvalue.patch (obsolete) — Splinter Review
Attachment #9054160 - Attachment is obsolete: true
Attachment #9054160 - Flags: review?(mkmelin+mozilla)
Attachment #9054161 - Flags: review?(mkmelin+mozilla)
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
Attachment #9054161 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 20

2 months ago
Posted patch searchvalue.patch (obsolete) — Splinter Review
Attachment #9054161 - Attachment is obsolete: true
Attachment #9054443 - Flags: review+
Assignee

Comment 21

2 months ago
Posted patch searchvalue1.patch (obsolete) — Splinter Review
Assignee

Comment 22

2 months ago
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?
Attachment #9054697 - Flags: feedback?(mkmelin+mozilla)
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
Attachment #9054697 - Flags: feedback?(mkmelin+mozilla)
Assignee

Comment 25

a month ago
Posted patch searchvalue.patch (obsolete) — Splinter Review
Attachment #9054443 - Attachment is obsolete: true
Attachment #9054697 - Attachment is obsolete: true
Attachment #9056434 - Flags: review+
Assignee

Comment 26

a month ago
Posted patch searchvalue.patch (obsolete) — Splinter Review
Attachment #9056434 - Attachment is obsolete: true
Attachment #9056435 - Flags: review+
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
Attachment #9056437 - Flags: review?(mkmelin+mozilla) → review-

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

Assignee

Comment 30

a month ago
Posted patch searchvalue.patch (obsolete) — Splinter Review
Attachment #9056437 - Attachment is obsolete: true
Attachment #9056726 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 31

a month ago

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 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
Attachment #9056726 - Flags: review?(mkmelin+mozilla) → review+
Assignee

Comment 34

a month ago
Posted patch searchvalue.patch (obsolete) — Splinter Review
Attachment #9056726 - Attachment is obsolete: true
Attachment #9056866 - Flags: review+
Assignee

Updated

a month ago
Keywords: checkin-needed

Comment 35

a month ago

This seems to be conflicting with bug 1513836.

Keywords: checkin-needed
Assignee

Comment 36

a month ago
Attachment #9056866 - Attachment is obsolete: true
Attachment #9057948 - Flags: review+
Assignee

Comment 38

a month ago

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

Assignee

Updated

a month ago
Keywords: checkin-needed

Comment 39

a month ago

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

a month ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1aad2ba888ec
Migrate searchvalue binding to custom element. r=mkmelin

Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

a month ago
Target Milestone: --- → Thunderbird 68.0
You need to log in before you can comment on or make changes to this bug.