1460982 - Convert <searchbar> to a Custom Element

Status: RESOLVED FIXED

(Firefox :: Search, task, P1)

Description

[Brian Grinstead [:bgrins]]

<searchbar> is used only once, in browser.xul. The implementation lives in https://searchfox.org/mozilla-central/source/browser/components/search/content/search.xml.

Some notes:

* [implements=nsIObserver] won't be available to the CE, but that's OK since we can set up the observer on a plain JS object assigned to the instance.
* Styles are loaded via <resources>. Bug 1460977 is on file to fix that.
* We'll want to lazy load the definition since it isn't visible by default. Bug 1460815 is on file to do that.
* Various strings are localized with dtd. I'd prefer to switch these to ftl if possible. Bug 1455649 will allow that.
* Initial conversion from XBL to JS is here https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/Searchbar.js (~400 LOC).

Comment 1

[Brian Grinstead [:bgrins]]

I noticed something that could make this more tricky. This comment seems to say we expect to dynamically apply the search-one-offs binding at runtime: https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/browser/components/search/content/search.xml#95-107.

But I think I'm misunderstanding that comment, because AFAICT .search-one-offs only binds to a single vbox: https://searchfox.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#1798. And scanning code that refers to that string I don't see anything obvious where we would be adding/removing the .search-one-offs class to any element: https://searchfox.org/mozilla-central/search?q=search-one-offs&path=.

Drew, do you know if there is a scenario where we would re-bind the <searchbar> to use the search-one-offs binding?

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1460982 - WIP - convert <searchbar> to a Custom Element

Review commit: https://reviewboard.mozilla.org/r/243520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243520/

Comment 3

[Brian Grinstead [:bgrins]]

The patch seems to work more-or-less, but still waiting on the blocking bugs before attempting to land anything.

Comment 4

[Drew Willcoxon :adw]

I think what this code is doing is not defining the binding dynamically but delaying the binding's construction, which (I think?) happens the first time a property/method is accessed on the binding, or maybe the first time the binding itself is accessed.  In a CE world, I guess that would mean that the CE shouldn't be created until popupshowing (but I don't know anything about CEs, so I don't know whether that makes sense).  But you should probably profile the performance to see whether the problem Florian was trying to fix is still a problem with a CE.

Comment 5

[Drew Willcoxon :adw]

The problem I mentioned that Florian fixed is bug 1369705.

Comment 6

[Brian Grinstead [:bgrins]]

Zibi: the searchbar uses some strings from browser.dtd: https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/browser/components/search/content/search.xml#24-66.

Do you have a recommendation for how/where to move these for Fluent? Here are the specific strings:

1) &searchInput.placeholder - Also used in aboutHome.xhtml: https://searchfox.org/mozilla-central/search?q=searchInput.placeholder&path=
2) &searchIcon.tooltip - This binding is the only consumer: https://searchfox.org/mozilla-central/search?q=searchIcon.tooltip&path=
3) &contentSearchSubmit.tooltip - Also used in aboutHome.xhtml: https://searchfox.org/mozilla-central/search?q=contentSearchSubmit.tooltip&path=

Comment 7

[Brian Grinstead [:bgrins]]

Drew, will the searchbar and/or the results popup be changed or removed as a result of quantumbar work?

Comment 8

[Drew Willcoxon :adw]

No, there are no plans to remove or modify the searchbar as part of quantumbar.  I haven't heard any other discussions about removing it or modifying it, either.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: Bug 1460982 - Convert <searchbar> to a Custom Element;r=adw

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> Zibi: the searchbar uses some strings from browser.dtd:
> https://searchfox.org/mozilla-central/rev/
> a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/browser/components/search/content/
> search.xml#24-66.
> 
> Do you have a recommendation for how/where to move these for Fluent? Here
> are the specific strings:
> 
> 1) &searchInput.placeholder - Also used in aboutHome.xhtml:
> https://searchfox.org/mozilla-central/search?q=searchInput.placeholder&path=
> 2) &searchIcon.tooltip - This binding is the only consumer:
> https://searchfox.org/mozilla-central/search?q=searchIcon.tooltip&path=
> 3) &contentSearchSubmit.tooltip - Also used in aboutHome.xhtml:
> https://searchfox.org/mozilla-central/search?q=contentSearchSubmit.
> tooltip&path=

Going to keep these in browser.dtd for now

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: Bug 1460982 - hg cp search.xml -> searchbar.js

Comment 12

[Brian Grinstead [:bgrins]]

Try and talos look fine:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee5763eef1f7b9394b45e21025610b4da5377277
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a06fef6819e12d6a3c3fe8c9a69a91544de3dc82&newProject=try&newRevision=ee5763eef1f7b9394b45e21025610b4da5377277&framework=1&showOnlyImportant=1

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 9009280 [details]
Bug 1460982 - Convert <searchbar> to a Custom Element;r=adw

Drew Willcoxon :adw has approved the revision.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb04b4d18e99
Convert <searchbar> to a Custom Element;r=adw

Comment 15

[Cristina Coroiu [:ccoroiu]]

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