1257008 - [Sync Tabs Sidebar] Right-clicking a device shouldn't collapse its list of tabs

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P3)

Description

[Lina Cambridge [:lina] (ex-Mozilla)]

Just a small glitch I noticed while working on bug 1254544.

STR:

1. Open the Synced Tabs sidebar.
2. Right-click on a device with some tabs.

AR: We collapse the tab list and show the context menu.
ER: Show the context menu without hiding the list.

Comment 1

[Lina Cambridge [:lina] (ex-Mozilla)]

Attachment: MozReview Request: Bug 1257008 - Don't collapse the tab list when right-clicking on a device in the Synced Tabs sidebar. r?markh

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

Comment 2

[Lina Cambridge [:lina] (ex-Mozilla)]

https://reviewboard.mozilla.org/r/40297/#review36801

::: browser/components/syncedtabs/TabListView.js:272
(Diff revision 1)
>  
> -    this._selectRow(itemNode);
> +    this._toggleRow(itemNode);
>    },
>  
> -  _selectRow(itemNode) {
> +  _toggleRow(itemNode) {
>      this.props.onSelectRow(this._getSelectionPosition(itemNode), itemNode.dataset.id);

There's a bit of indirection here in that `onSelectRow` in `TabListComponent.js` will toggle the branch if you give it an ID. This is the desired behavior for `onClick`, but not `handleContextMenu`.

Comment 3

[Lina Cambridge [:lina] (ex-Mozilla)]

Comment on attachment 8730986 [details]
MozReview Request: Bug 1257008 - Don't collapse the tab list when right-clicking on a device in the Synced Tabs sidebar. r?markh

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40297/diff/1-2/

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8730986 [details]
MozReview Request: Bug 1257008 - Don't collapse the tab list when right-clicking on a device in the Synced Tabs sidebar. r?markh

https://reviewboard.mozilla.org/r/40297/#review36827

I have a vague preference for the comments below, but if that becomes more difficult than it looks or you feel strongly that we shouldn't do that, feel free to ignore.

Thanks!

::: browser/components/syncedtabs/TabListView.js:269
(Diff revisions 1 - 2)
>        this.props.onToggleBranch(itemNode.dataset.id);
>        return;
>      }
>  
> -    this._toggleRow(itemNode);
> -  },
> +    let position = this._getSelectionPosition(itemNode);
> +    this.props.onToggleRow(position, itemNode.dataset.id);

so here we'd just do an onSelectRow before the onToggleRow.

::: browser/components/syncedtabs/TabListComponent.js:84
(Diff revision 2)
>    onFilterBlur() {
>      this._store.blurInput();
>    },
>  
> -  onSelectRow(position, id) {
> -    this._store.selectRow(position[0], position[1]);
> +  onToggleRow(position, id) {
> +    this.onSelectRow(position);

Can we avoid having onToggleRow also doing the select?

Comment 5

[Lina Cambridge [:lina] (ex-Mozilla)]

https://reviewboard.mozilla.org/r/40297/#review36827

Oh, I see what you were saying on IRC now...keep these separate, and have the view call them both.

I kind of like having a single call that means "do everything you need to make this active," but I don't have my heart set on that. Your suggestion seems sensible, too. And we can still have a call to do that; it'd just live in the view instead of the component.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6ae9eb6d8dca

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ae9eb6d8dca