Last Comment Bug 822068 - The content loses focus after calling gBrowser.moveTabBackward/moveTabForward
: The content loses focus after calling gBrowser.moveTabBackward/moveTabForward
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 20
Assigned To: ithinc
:
Mentors:
Depends on:
Blocks: 364845
  Show dependency treegraph
 
Reported: 2012-12-15 17:06 PST by ithinc
Modified: 2012-12-26 05:20 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.51 KB, patch)
2012-12-15 17:10 PST, ithinc
dao+bmo: review+
Details | Diff | Review
patch for checkin (4.56 KB, patch)
2012-12-24 06:35 PST, ithinc
no flags Details | Diff | Review

Description ithinc 2012-12-15 17:06:01 PST
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 2012-12-15 17:10:28 PST
Created attachment 692677 [details] [diff] [review]
patch
Comment 2 Dão Gottwald [:dao] 2012-12-17 13:39:12 PST
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 2012-12-18 00:19:42 PST
(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] 2012-12-18 02:44:26 PST
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 2012-12-24 06:35:41 PST
Created attachment 695470 [details] [diff] [review]
patch for checkin
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-12-24 12:29:58 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/0185385d13ba
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-12-26 05:20:35 PST
https://hg.mozilla.org/mozilla-central/rev/0185385d13ba

Note You need to log in before you can comment on or make changes to this bug.