Fold searchbar-textbox with searchbar CE

RESOLVED FIXED in Firefox 69

Status

()

task
RESOLVED FIXED
3 months ago
5 days ago

People

(Reporter: ntim, Assigned: surkov)

Tracking

(Blocks 1 bug)

unspecified
Firefox 69
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox69 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

3 months ago

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

Assignee

Comment 1

16 days 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

14 days 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 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.

Assignee

Comment 3

13 days 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 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.

Assignee

Comment 5

13 days 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?

Flags: needinfo?(emilio)
Assignee

Comment 6

13 days 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.

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.

Flags: needinfo?(emilio)
Assignee

Comment 8

13 days 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?

Flags: needinfo?(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.

Flags: needinfo?(emilio)
Reporter

Updated

11 days ago
Type: enhancement → task
Assignee

Comment 11

10 days 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

10 days ago
Assignee: nobody → surkov.alexander
Attachment #9069396 - Attachment is obsolete: true

Comment 12

9 days ago
Pushed by asurkov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a885db13bf15
Fold searchbar-textbox with searchbar CE r=mak
Reporter

Updated

9 days ago
Blocks: 1556561

Comment 13

9 days ago
bugherder
Status: NEW → RESOLVED
Closed: 9 days ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
Depends on: 1557869
No longer depends on: 1557869
Regressions: 1557869
Reporter

Updated

8 days ago
Regressions: 1557942
Reporter

Updated

8 days ago
No longer regressions: 1557942

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

No longer blocks: 1557869
You need to log in before you can comment on or make changes to this bug.