[de-xbl] convert the glodaSearch binding
Categories
(Thunderbird :: General, task)
Tracking
(Not tracked)
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.
Updated•6 years ago
|
Reporter | ||
Comment 1•5 years ago
|
||
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 | ||
Comment 2•5 years ago
•
|
||
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
Comment 3•5 years ago
|
||
(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 thegloda-search
CE.
Are you planning to make it accessible throughMozElements
or it can only be used asis="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
Assignee | ||
Comment 4•5 years ago
•
|
||
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.
Comment 5•5 years ago
|
||
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" />`
Assignee | ||
Comment 6•5 years ago
|
||
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.
Comment 7•5 years ago
|
||
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 8•5 years ago
|
||
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="&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…
Assignee | ||
Comment 9•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 10•5 years ago
|
||
Comment 11•5 years ago
•
|
||
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.
Reporter | ||
Comment 12•5 years ago
|
||
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
Assignee | ||
Comment 13•5 years ago
|
||
Here's an updated version.
I'm asking for feedback since a full review it's not possible until bug 1534455 lands.
Reporter | ||
Comment 14•5 years ago
|
||
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.
Assignee | ||
Comment 15•5 years ago
|
||
Thanks for the quick feedback.
I updated the patch, waiting for D33250 to land before asking for a full review.
Assignee | ||
Comment 16•5 years ago
|
||
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.
Reporter | ||
Comment 17•5 years ago
|
||
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
Assignee | ||
Comment 18•5 years ago
|
||
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?
Comment 19•5 years ago
|
||
Does mozmill/quick-filter-bar/test-keyboard-interface.js | test-keyboard-interface.js::test_escape_does_not_reach_us_from_gloda_search pass?
Assignee | ||
Comment 20•5 years ago
|
||
Fixed the test failure, sorry about that.
Reporter | ||
Comment 21•5 years ago
|
||
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.
Assignee | ||
Comment 22•5 years ago
|
||
The linter wasn't complaining about the missing XPCOMUtils
import.
Weird.
Assignee | ||
Comment 23•5 years ago
|
||
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.
Assignee | ||
Comment 24•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 25•5 years ago
|
||
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
Reporter | ||
Comment 26•5 years ago
|
||
Comment on attachment 9093734 [details] [diff] [review] 1542720-dexbl-glodasearch.patch Review of attachment 9093734 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! r=mkmelin
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 27•5 years ago
|
||
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
Updated•5 years ago
|
Comment 28•5 years ago
|
||
Kindly run the linter before submitting a patch :-(
Comment 29•5 years ago
|
||
Reporter | ||
Updated•5 years ago
|
Comment 30•5 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/395ad4f5654a Follow-up: Fix linting errors. r=mkmelin
Description
•