822068 - The content loses focus after calling gBrowser.moveTabBackward/moveTabForward

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[ithinc]

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

Comment 1

[ithinc]

Attachment: patch

Comment 2

[Dão Gottwald [::dao]]

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

Comment 3

[ithinc]

(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?

Comment 4

[Dão Gottwald [::dao]]

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.

Comment 5

[ithinc]

Attachment: patch for checkin

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0185385d13ba

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0185385d13ba