Closed Bug 1029501 Opened 8 years ago Closed 8 years ago

Opening the awesometab and then pressing Escape should return to the previous conversation

Categories

(Instantbird :: Conversation, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: nhnt11)

Details

Attachments

(1 file, 4 obsolete files)

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.
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
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Attachment #8447103 - Flags: review?(florian)
Attachment #8447103 - Flags: review?(florian) → review+
Would be nice to cleanup macgestures.js :-)
Attached patch Patch v2 (obsolete) — Splinter Review
Moved mPreviousTab field definition below mCurrentTab, update macgestures.js as suggested.
Attachment #8447103 - Attachment is obsolete: true
Attachment #8447127 - Flags: review?(florian)
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.
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
Attached patch Patch v3 (obsolete) — Splinter Review
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)
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?
Attached patch Patch v4 (obsolete) — Splinter Review
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)
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?)
Attached patch Patch v5Splinter Review
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)
Comment on attachment 8448612 [details] [diff] [review]
Patch v5

Review of attachment 8448612 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! :-)
Attachment #8448612 - Flags: review?(florian) → review+
https://hg.mozilla.org/comm-central/rev/b82a9f18921f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.