Closed Bug 1257008 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Sync, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: lina, Assigned: lina)

Details

Attachments

(1 file)

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.
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 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 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?
Attachment #8730986 - Flags: review?(markh) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/6ae9eb6d8dca
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: