Fold searchbar-textbox with searchbar CE
Categories
(Firefox :: Search, task)
Tracking
()
Tracking | Status | |
---|---|---|
firefox69 | --- | fixed |
People
(Reporter: ntim, Assigned: surkov)
References
Details
Attachments
(1 file, 1 obsolete file)
Since I believe only the searchbar CE uses the searchbar-textbox binding, they could be folded together.
Assignee | ||
Comment 1•5 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #0)
Since I believe only the searchbar CE uses the searchbar-textbox binding, they could be folded together.
searchbar-textbox overrides a lot coming from autocomplete, so as long as searchbar is not an autocomplete itself, it makes sense to keep these things separately I'd say. Also, both searchbar and searchbar-textbox widgets are not small (300+ lines in each), which might make another argument to keep those separately. And it will require less changes if we keep status quo.
Reporter | ||
Comment 2•5 years ago
|
||
(In reply to alexander :surkov (:asurkov) from comment #1)
(In reply to Tim Nguyen :ntim from comment #0)
Since I believe only the searchbar CE uses the searchbar-textbox binding, they could be folded together.
searchbar-textbox overrides a lot coming from autocomplete, so as long as searchbar is not an autocomplete itself, it makes sense to keep these things separately I'd say. Also, both searchbar and searchbar-textbox widgets are not small (300+ lines in each), which might make another argument to keep those separately. And it will require less changes if we keep status quo.
By "folding", I meant overriding by setting the searchbar-textbox
expando properties from searchbar
, similar to what bgrins has done in bug 1539665.
But thinking more about it, it might make sense to fix this at the same time as bug 1534455 rather than separately, since bug 1534455 moves out some searchbar-textbox
specific logic from the autocomplete
binding (mainly the <children includes="">
which are only needed for searchbar-textbox
).
So after bug 1534455, this markup would simply become:
<stringbundle src="chrome://browser/locale/search.properties"></stringbundle>
<hbox class="searchbar-search-button" tooltiptext="&searchIcon.tooltip;">
<image class="searchbar-search-icon"></image>
<image class="searchbar-search-icon-overlay"></image>
</hbox>
<html:input class="searchbar-textbox" is="autocomplete-input" inputtype="search" placeholder="&searchInput.placeholder;" flex="1" autocompletepopup="PopupSearchAutoComplete" autocompletesearch="search-autocomplete" autocompletesearchparam="searchbar-history" maxrows="10" completeselectedindex="true" minresultsforpopup="0"/>
<hbox class="search-go-container">
<image class="search-go-button urlbar-icon" hidden="true" onclick="handleSearchCommand(event);" tooltiptext="&contentSearchSubmit.tooltip;"></image>
</hbox>
As for the other properties in searchbar-textbox
, there's:
-
initContextMenu
I think it would be fine to move the context menu logic tosearchbar
since it's not tied toautocomplete
at all, especially since the new markup shouldn't involvemoz-input-box
but use whattextarea
/input
are using instead. -
searchParam
-
onTextEntered
-
openPopup
-
onBeforeValueSet
These overrideautocomplete
properties, but overriding should be feasible from the parent CE as well, by doingthis._textbox.searchParam = ...
. -
openSearch
-
handleEnter
-
handleKeyboardNavigation
These are mainly used as event handlers, so it should be possible to init those from the searchbar CE. -
selectedButton
-
searchbarController
These are props specific to searchbar-textbox and we can move them to thesearchbar
CE.
If the size of the searchbar
CE turns out to be unreasonable, I think it's fine to create a customized input element that extends autocomplete-input, but I don't think it will necessarily make things cleaner, given that we'd need to keep existing hacks like this.closest("searchbar")._textboxInitialized = true
to synchronise searchbar
and searchbar-textbox
together.
Anyway, I'm not planning to work on this or bug 1534455 anytime soon, so feel free to finish this work in whatever way you think is cleaner, but overall, I'd really like <textbox>
to go away for these usages and that we start using <html:input/>
with custom behaviour, this way we can get rid of textbox completely in the future.
Assignee | ||
Comment 3•5 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #2)
If the size of the
searchbar
CE turns out to be unreasonable, I think it's fine to create a customized input element that extends autocomplete-input, but I don't think it will necessarily make things cleaner, given that we'd need to keep existing hacks likethis.closest("searchbar")._textboxInitialized = true
to synchronisesearchbar
andsearchbar-textbox
together.
agreed, keeping the status quo is not the nicest approach to organize the code, but I think it's the easiest one in order to kill XBL.
Assignee | ||
Comment 4•5 years ago
|
||
Assignee | ||
Comment 5•5 years ago
|
||
with the patch applied and when running browser/components/customizableui/test/browser_1003588_no_specials_in_panel.js I see that searchbar diconnectedCallback triggers disconnectedCallback and then connectedCallback on a child searchbar-textbox custom element. It seems like a bug. Emilio, any thoughts on this?
Assignee | ||
Comment 6•5 years ago
•
|
||
Here's a test case that demos it:
class TopEl extends HTMLElement {
connectedCallback() {
console.log('top el connected');
this.innerHTML = `<inner-el></inner-el>`;
}
disconnectedCallback() {
console.log('top el diconnected');
}
}
customElements.define('top-el', TopEl);
class InnerEl extends HTMLElement {
connectedCallback() {
console.log('inner el connected');
}
disconnectedCallback() {
console.log('inner el diconnected');
}
}
customElements.define('inner-el', InnerEl);
let el = document.createElement('top-el');
console.log(`#1: connect`);
document.body.appendChild(el);
console.log(`#2: disconnect`);
el.remove();
console.log(`#3: connect`);
document.body.appendChild(el);
#1: connect debugger eval code:23:9
top el connected debugger eval code:3:13
inner el connected debugger eval code:14:13
#2: disconnect debugger eval code:25:9
top el diconnected debugger eval code:7:13
inner el diconnected debugger eval code:17:13
#3: connect debugger eval code:27:9
top el connected debugger eval code:3:13
inner el connected debugger eval code:14:13
inner el diconnected debugger eval code:17:13
inner el connected
After top CE is connected for 2nd time, the inner CE gets connected-disconnected-connected.
Comment 7•5 years ago
|
||
That makes perfect sense to me (and you can see other browsers agree with us too).
The second connect for TopEl
does this.innerHTML = '<inner-el></inner-el>'
. That removes the inner element and creates a new one, so it's normal that you get a new disconnect / connect pair.
Assignee | ||
Comment 8•5 years ago
|
||
hm, still not sure I'm clear on the behavior. If you do this.innerHTML = '', then I would expect to see disconnected called on an existing child element, and then connected for a new child element. No clue why I have 2nd connected callback with null parentNode. After all how is it possible that a connected element has no parent?
Does the behavior comply with the spec?
Comment 9•5 years ago
|
||
Connected callbacks works like a queue of events (they're spec it that way), they're not sync. So when you connect the outer element also containing the inner one, you're connecting the outer, posting a callback, then connecting the inner child, posting a callback, then when the first callback runs, you disconnect the inner one (which doesn't remove the connected callback from the queue), and then you connect the newly-created inner element.
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 10•5 years ago
|
||
Assignee | ||
Comment 11•5 years ago
|
||
here's Tim's approach, I swamped in two CE controls so combinding those two into one seems a way to go
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 12•5 years ago
|
||
Pushed by asurkov@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a885db13bf15 Fold searchbar-textbox with searchbar CE r=mak
Comment 13•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Comment 14•5 years ago
|
||
I believe this change is causing an error message to trigger on browser launch: JavaScript error: chrome://browser/content/search/searchbar.js, line 164: TypeError: this._textbox is undefined
Description
•