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)
Firefox
Sync
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.
Assignee | ||
Comment 1•8 years ago
|
||
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•8 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•8 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•8 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•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6ae9eb6d8dca
Status: NEW → RESOLVED
Closed: 8 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
•