bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in 1.6

Status

Instantbird
Conversation
--
minor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: florian, Assigned: nhnt11)

Tracking

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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

4 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

4 years ago
Created attachment 8447103 [details] [diff] [review]
Patch
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Attachment #8447103 - Flags: review?(florian)
(Reporter)

Updated

4 years ago
Attachment #8447103 - Flags: review?(florian) → review+
(Reporter)

Comment 2

4 years ago
Would be nice to cleanup macgestures.js :-)
(Assignee)

Comment 3

4 years ago
Created attachment 8447127 [details] [diff] [review]
Patch v2

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.
(Assignee)

Updated

4 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

4 years ago
Created attachment 8447148 [details] [diff] [review]
Patch v3

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

4 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

4 years ago
Created attachment 8448335 [details] [diff] [review]
Patch v4

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

4 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

4 years ago
Created attachment 8448612 [details] [diff] [review]
Patch v5

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

4 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

4 years ago
https://hg.mozilla.org/comm-central/rev/b82a9f18921f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.