1534404 - Fold searchbar-textbox with searchbar CE

Status: RESOLVED FIXED

(Firefox :: Search, task)

Description

[Tim Nguyen :ntim]

Since I believe only the searchbar CE uses the searchbar-textbox binding, they could be folded together.

Comment 1

[alexander :surkov (:asurkov)]

(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.

Comment 2

[Tim Nguyen :ntim]

(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 to searchbar since it's not tied to autocomplete at all, especially since the new markup shouldn't involve moz-input-box but use what textarea/input are using instead.

• searchParam

• onTextEntered

• openPopup

• onBeforeValueSet
  These override autocomplete properties, but overriding should be feasible from the parent CE as well, by doing this._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 the searchbar 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.

Comment 3

[alexander :surkov (:asurkov)]

(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 like this.closest("searchbar")._textboxInitialized = true to synchronise searchbar and searchbar-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.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: Bug 1534404 - convert searchbar-textbox to a Custom Element

Comment 5

[alexander :surkov (:asurkov)]

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?

Comment 6

[alexander :surkov (:asurkov)]

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

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 8

[alexander :surkov (:asurkov)]

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

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: Bug 1534404 - Fold searchbar-textbox with searchbar CE

Comment 11

[alexander :surkov (:asurkov)]

here's Tim's approach, I swamped in two CE controls so combinding those two into one seems a way to go

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a885db13bf15
Fold searchbar-textbox with searchbar CE r=mak

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/a885db13bf15

Comment 14

[Brad Werth [:bradwerth]]

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