[Sync Tabs Sidebar] Right-clicking a device shouldn't collapse its list of tabs
RESOLVED
FIXED
in Firefox 48
Status
()
People
(Reporter: lina, Assigned: lina)
Tracking
Firefox Tracking Flags
(firefox48 fixed)
Details
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.
(Assignee) | ||
Comment 1•3 years ago
|
||
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)
(Assignee) | ||
Comment 2•3 years ago
|
||
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`.
(Assignee) | ||
Comment 3•3 years ago
|
||
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•3 years ago
|
||
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+
(Assignee) | ||
Comment 5•3 years ago
|
||
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•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6ae9eb6d8dca
Status: NEW → RESOLVED
Last Resolved: 3 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.
Description
•