The content loses focus after calling gBrowser.moveTabBackward/moveTabForward

RESOLVED FIXED in Firefox 20

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ithinc, Assigned: ithinc)

Tracking

Trunk
Firefox 20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

4.56 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
this.mCurrentTab.focus() should not be called directly in gBrowser.moveTabBackward/moveTabForward.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2248
(Assignee)

Comment 1

4 years ago
Created attachment 692677 [details] [diff] [review]
patch
Attachment #692677 - Flags: review?(dao)
(Assignee)

Updated

4 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: The tab element is focused after calling gBrowser.moveTabBackward/moveTabForward → The content loses focus after calling gBrowser.moveTabBackward/moveTabForward
(Assignee)

Updated

4 years ago
Blocks: 364845
Comment on attachment 692677 [details] [diff] [review]
patch

>+          let wasFocused = false;
>+          try {
>+            wasFocused = (document.commandDispatcher.focusedElement == this.mCurrentTab);
>+          } catch (e) {}
>           this.mCurrentTab._selected = false;

Use document.activeElement and add a blank line after your code.

>           // use .item() instead of [] because dragging to the end of the strip goes out of
>           // bounds: .item() returns null (so it acts like appendChild), but [] throws
>           this.tabContainer.insertBefore(aTab, this.tabs.item(aIndex));
> 
>           for (let i = 0; i < this.tabs.length; i++) {
>             this.tabs[i]._tPos = i;
>             this.tabs[i]._selected = false;
>           }
>           this.mCurrentTab._selected = true;
>+          if (wasFocused)
>+            this.mCurrentTab.focus();

Add a blank line before your code.

>           this.tabContainer._handleTabSelect(false);
>-
>           if (aTab.pinned)
>             this.tabContainer._positionPinnedTabs();

Avoid this change.

r=me with these things addressed
Attachment #692677 - Flags: review?(dao) → review+
(Assignee)

Comment 3

4 years ago
(In reply to Dão Gottwald [:dao] from comment #2)
> 
> >+          let wasFocused = false;
> >+          try {
> >+            wasFocused = (document.commandDispatcher.focusedElement == this.mCurrentTab);
> >+          } catch (e) {}
> >           this.mCurrentTab._selected = false;
> 
> Use document.activeElement and add a blank line after your code.

Is there any difference here? I read that there're several instances using document.commandDispatcher.focusedElement in tabbox.xml, so I copied it here. 
And If I use document.activeElement, I assume try ... catch is no more needed?
Yes, try/catch isn't needed. document.activeElement is the standard way to do this. document.commandDispatcher.focusedElement is a proprietary Mozilla thing that you may need when dealing with anonymous content, which isn't the case here.
(Assignee)

Comment 5

4 years ago
Created attachment 695470 [details] [diff] [review]
patch for checkin
Attachment #692677 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/0185385d13ba
Assignee: nobody → ithinc
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0185385d13ba
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.