Closed
Bug 1029501
Opened 10 years ago
Closed 10 years ago
Opening the awesometab and then pressing Escape should return to the previous conversation
Categories
(Instantbird Graveyard :: Conversation, defect)
Instantbird Graveyard
Conversation
Tracking
(Not tracked)
RESOLVED
FIXED
1.6
People
(Reporter: florian, Assigned: nhnt11)
Details
Attachments
(1 file, 4 obsolete files)
3.39 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
Steps to reproduce: 1. Press Command+T to open a new Firefox tab, but forget that Instantbird currently has focus. 2. Be surprised by the awesometab, and press Escape to undo showing it. Expected result: Escape should have returned the conversation in the state in which it was before. Actual result: The last tab is now selected.
Assignee | ||
Updated•10 years ago
|
Summary: Opening the awesometab and then pressing Escape should return to the previous conversation → Selecting a tab and closing it should focus the previously selected tab
Assignee | ||
Comment 1•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Attachment #8447103 -
Flags: review?(florian) → review+
Reporter | ||
Comment 2•10 years ago
|
||
Would be nice to cleanup macgestures.js :-)
Assignee | ||
Comment 3•10 years ago
|
||
Moved mPreviousTab field definition below mCurrentTab, update macgestures.js as suggested.
Attachment #8447103 -
Attachment is obsolete: true
Attachment #8447127 -
Flags: review?(florian)
Comment 4•10 years ago
|
||
I'm a bit confused by this rename. Are you changing the behavior that it will now select the previously selected tab or just the tab before it? If you're making it select the previously selected tab I have to strongly go against this. I find that behavior EXTREMELY confusing and unintuitive.
Assignee | ||
Updated•10 years ago
|
Summary: Selecting a tab and closing it should focus the previously selected tab → Opening the awesometab and then pressing Escape should return to the previous conversation
Assignee | ||
Comment 5•10 years ago
|
||
This changes the behavior to "If a new conversation tab is opened and then closed without switching away, we restore focus to the previous tab." as discussed.
Attachment #8447127 -
Attachment is obsolete: true
Attachment #8447127 -
Flags: review?(florian)
Attachment #8447148 -
Flags: review?(florian)
Reporter | ||
Comment 6•10 years ago
|
||
Comment on attachment 8447148 [details] [diff] [review] Patch v3 Review of attachment 8447148 [details] [diff] [review]: ----------------------------------------------------------------- ::: im/content/newtab.xml @@ +172,5 @@ > <method name="destroy"> > <body> > <![CDATA[ > + if (this.previousTab) > + window.getTabBrowser().selectedTab = this.previousTab; Will this code run if we are closing the whole window at once?
Assignee | ||
Comment 7•10 years ago
|
||
This uses the tabbrowser's _windowIsClosing property to prevent trying to focus the previous tab when closing the whole window. Not sure if this is the best solution since _windowIsClosing isn't really meant to be used by consumers. Also, I'd forgotten to forget the previous tab after opening a new conversation, this fixes that (last patch wasn't tested thoroughly, apologies). I've also taken the opportunity to ensure we don't call destroy() twice.
Attachment #8447148 -
Attachment is obsolete: true
Attachment #8447148 -
Flags: review?(florian)
Attachment #8448335 -
Flags: review?(florian)
Reporter | ||
Comment 8•10 years ago
|
||
Comment on attachment 8448335 [details] [diff] [review] Patch v4 Review of attachment 8448335 [details] [diff] [review]: ----------------------------------------------------------------- ::: im/content/newtab.xml @@ +373,5 @@ > return; > let item = this.listbox.selectedItem || this.listbox.firstChild; > this.startConversation(item); > + // Ensure we don't focus the previous tab after removing ours. > + this.previousTab = null; Shouldn't this be done in the startConversation method, rather than duplicating it? I see you are not doing it when using modifiers to avoid closing the awesometab, but does that really make sense? (ie. if you create a new conversation from the awesometab, and then close the awesometab, do you really expect to have the original tab focused?)
Assignee | ||
Comment 9•10 years ago
|
||
Makes sense to forget the previous tab if we opened a conversation in any case, thanks.
Attachment #8448335 -
Attachment is obsolete: true
Attachment #8448335 -
Flags: review?(florian)
Attachment #8448612 -
Flags: review?(florian)
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8448612 [details] [diff] [review] Patch v5 Review of attachment 8448612 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! :-)
Attachment #8448612 -
Flags: review?(florian) → review+
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/b82a9f18921f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Target Milestone: --- → 1.6
You need to log in
before you can comment on or make changes to this bug.
Description
•