Closed Bug 1247345 Opened 4 years ago Closed 4 years ago

Synced tab sidebar scrolls differently than bookmarks and history sidebar

Categories

(Firefox :: Sync, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox47 --- affected
firefox48 --- verified

People

(Reporter: rfeeley, Assigned: markh)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached image scroll.gif
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.
Blocks: 1239084
Flags: firefox-backlog+
Triage: P2
Priority: -- → P2
Blocks: 1257587
No longer blocks: 1257587
Depends on: 1257587
Blocks: 1257587
No longer depends on: 1257587
Assignee: nobody → markh
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.
Attachment #8732689 - Flags: review?(gijskruitbosch+bugs)
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) ?
Attachment #8732689 - Flags: review?(gijskruitbosch+bugs)
Status: NEW → ASSIGNED
(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?
Attachment #8732689 - Attachment is obsolete: true
Attachment #8733204 - Flags: feedback?(gijskruitbosch+bugs)
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.
Attachment #8733204 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
(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.
Attachment #8733204 - Attachment is obsolete: true
Attachment #8733641 - Flags: review?(gijskruitbosch+bugs)
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!
Attachment #8733641 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/84008247ec17
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OS X 10.9.5 using latest Nightly 48.0a1 (buildID: 20160413030239).
You need to log in before you can comment on or make changes to this bug.