In the bookmarks and history sidebars, when there are enough items to overflow the sidebar, the search box remains fixed with the header at the top of the sidebar. The search box in the synced tabs sidebar does not, but should. See attached.

Not blocking for 47, but not to be forgotten.

Triage: P2

Gijs, In the SyncedTabs sidebar, the searchbox is part of the scrollable area. This patch is the best way I could find to fix this.

Comment on attachment 8732689 [details] [diff] [review] 0001-Bug-1247345-arrange-for-the-searchbox-to-not-scroll-.patch Review of attachment 8732689 [details] [diff] [review]: ----------------------------------------------------------------- I'm not thrilled with this. I tried to ping you on IRC, but maybe this is easier: wouldn't it be simpler to create an extra level of nesting and ensure that the search field isn't in the scrollable container to begin with? That is, can't we just change the html template (and maybe make a tiny change to the JS that inserts stuff based off the template) ?

(In reply to :Gijs Kruitbosch from comment #4) > I'm not thrilled with this. I tried to ping you on IRC, but maybe this is > easier: wouldn't it be simpler to create an extra level of nesting and > ensure that the search field isn't in the scrollable container to begin > with? Unless I'm missing what you are saying, there isn't a container that isn't scrollable in the current layout (unless you mean the "Synced Tabs" header? That's not part of the sidebar document) But yeah, it probably should be setup that way. This patch uses flexbox to create a non-scrollable header, and moves the search box from the template directly into this header, using CSS to hide it when necessary. The patch is quite a bit bigger, but it doesn't need to hard-code any heights into CSS. What do you think about this?

Comment on attachment 8733204 [details] [diff] [review] 0001-Bug-1247345-arrange-for-the-searchbox-to-not-scroll-.patch Review of attachment 8733204 [details] [diff] [review]: ----------------------------------------------------------------- I've not checked what this looks like yet, but in general this change looks neater to me. :-) ::: browser/components/syncedtabs/TabListView.js @@ +178,5 @@ > this.searchBox.classList.add("filtered"); > } else { > this.searchBox.classList.remove("filtered"); > } > + this.tabsFilter.value = state.filter; Moving this feels like an unrelated bugfix? ::: browser/themes/shared/syncedtabs/sidebar.inc.css @@ +187,5 @@ > display: unset; > opacity: 100; > } > > +.sidebar-search-container.tabs-container { Nit: I'd use :not(.selected) to have just 1 rule here that hides the item.

(In reply to :Gijs Kruitbosch from comment #6) > I've not checked what this looks like yet, but in general this change looks > neater to me. :-) Great, thanks (and yeah, I agree!) > ::: browser/components/syncedtabs/TabListView.js > @@ +178,5 @@ > > this.searchBox.classList.add("filtered"); > > } else { > > this.searchBox.classList.remove("filtered"); > > } > > + this.tabsFilter.value = state.filter; > > Moving this feels like an unrelated bugfix? Sadly not - the searchbox was previously in the template, which was re-created as the filter was changed - thus it was previously only necessary to set the filter string as the template was created. Now the searchbox is outside the template and persists even as the template is re-created - so now we explicitly update the searchbox instead of re-creating it. > Nit: I'd use :not(.selected) to have just 1 rule here that hides the item. Done, thanks.

Comment on attachment 8733641 [details] [diff] [review] 0001-Bug-1247345-arrange-for-the-searchbox-to-not-scroll-.patch Review of attachment 8733641 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Backout: https://hg.mozilla.org/integration/fx-team/rev/a7031fdd9cb9

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OS X 10.9.5 using latest Nightly 48.0a1 (buildID: 20160413030239).