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

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Sync
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kitcambridge, Assigned: kitcambridge)

Tracking

unspecified
Firefox 48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
Created 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 commit: https://reviewboard.mozilla.org/r/40297/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40297/
Attachment #8730986 - Flags: review?(markh)
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.

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6ae9eb6d8dca
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.