Closed Bug 1534345 Opened 6 years ago Closed 6 years ago

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

Categories

(Thunderbird :: Mail Window Front End, task)

Product:
Thunderbird
Component:
Mail Window Front End
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0

People

(Reporter: arshad, Assigned: arshad)

References

Details

Attachments

(1 file, 13 obsolete files)

searchvalue.patch
(deleted), patch
arshad
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → arshdkhn1
Status: NEW → ASSIGNED
Blocks: tb-war-on-xbl
Attached patch searchvalue.patch (obsolete) (deleted) — — Splinter Review
Attachment #9050032 - Flags: review?(mkmelin+mozilla)
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 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 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.

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

my linter preferes { var }.

Attached patch searchvalue.patch (obsolete) (deleted) — — Splinter Review
Attachment #9050032 - Attachment is obsolete: true
Attachment #9050032 - Flags: review?(mkmelin+mozilla)
Attachment #9050222 - Flags: review?(mkmelin+mozilla)
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)

undeclared global, what does eslint say?

nothing :/

Attached patch searchvalue.patch (obsolete) (deleted) — — 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-

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.

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.

Attached patch searchvalue.patch (obsolete) (deleted) — — Splinter Review
Attachment #9050665 - Attachment is obsolete: true
Attachment #9051498 - Flags: review+
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.
Blocks: 1539517
Attached patch searchvalue.patch (obsolete) (deleted) — — Splinter Review
Attachment #9051498 - Attachment is obsolete: true
Attachment #9054160 - Flags: review?(mkmelin+mozilla)
Attached patch searchvalue.patch (obsolete) (deleted) — — 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+
Attached patch searchvalue.patch (obsolete) (deleted) — — Splinter Review
Attachment #9054161 - Attachment is obsolete: true
Attachment #9054443 - Flags: review+
Attached patch searchvalue1.patch (obsolete) (deleted) — — Splinter Review
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)
Attached patch searchvalue.patch (obsolete) (deleted) — — Splinter Review
Attachment #9054443 - Attachment is obsolete: true
Attachment #9054697 - Attachment is obsolete: true
Attachment #9056434 - Flags: review+
Attached patch searchvalue.patch (obsolete) (deleted) — — 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

Attached patch searchvalue.patch (obsolete) (deleted) — — Splinter Review
Attachment #9056437 - Attachment is obsolete: true
Attachment #9056726 - Flags: review?(mkmelin+mozilla)

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+
Attached patch searchvalue.patch (obsolete) (deleted) — — Splinter Review
Attachment #9056726 - Attachment is obsolete: true
Attachment #9056866 - Flags: review+
Keywords: checkin-needed

This seems to be conflicting with bug 1513836.

Keywords: checkin-needed
Attached patch searchvalue.patch (deleted) — — Splinter Review
Attachment #9056866 - Attachment is obsolete: true
Attachment #9057948 - Flags: review+

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

Keywords: checkin-needed

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.

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

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: