[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)
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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
|
||
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
|
||
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
|
||
Reporter | ||
Comment 12•5 years ago
|
||
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
|
||
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
|
||
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
|
||
Assignee | ||
Comment 22•5 years ago
|
||
The linter wasn't complaining about the missing XPCOMUtils
import.
Weird.
Assignee | ||
Comment 23•5 years ago
|
||
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
|
||
Reporter | ||
Comment 26•5 years ago
|
||
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
|
||
Description
•