995758 - Address bar doesn't capture focus if a new tab is created via Ctrl+T shortcut while in Customization mode

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P5)

Description

[jaramat]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0

Steps to reproduce:
Enter Customization mode via PanelUI menu/right click - Customize.../or about:customizing. Press Ctrl+T to open a new tab.

Expected results:
New tab opens, and the address bar receives focus so the user can type address right away.

Actual results:
New tab opens, but the address bar isn't active until the user manually activates it.


Additionally, the same goes for Ctrl+K shortcut. When pressed, it quits Customization mode, but doesn't activate the Search bar.

Tested in latest Nightly 31.0a1 (2014-04-13), but this bug might be reproducible in Aurora and Beta as well.

Marked as trivial because this is a minor bug, and I don't expect many users to open a new tab from Customization mode anyway.

Comment 1

[Paul Silaghi, QA [:pauly]]

Confirmed 31.0a1 (2014-04-14), Win 7 x64.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

For the urlbar, it looks like the focusAndSelectUrlBar function at https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1945 is failing.  In particular, the call to gURLBar.select() isn't selecting the gURLBar.  I suspect that if the current url is "about:customizing", we will need to wait for an event before we call focusAndSelectUrlBar…

I tried:
gNavToolbox.addEventListener("customization-transitionend", …);
and:
gNavToolbox.addEventListener("customizationend", …);

But those both seemed to fire before the urlbar was unwrapped.  Maybe Gijs will know what event we should be waiting for is, so that we can call focusAndSelectUrlBar and have it actually focus the urlbar.  :)

Comment 3

[:Gijs (he/him)]

(In reply to Blake Winton (:bwinton) from comment #2)
> For the urlbar, it looks like the focusAndSelectUrlBar function at
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> js#1945 is failing.  In particular, the call to gURLBar.select() isn't
> selecting the gURLBar.  I suspect that if the current url is
> "about:customizing", we will need to wait for an event before we call
> focusAndSelectUrlBar…
> 
> I tried:
> gNavToolbox.addEventListener("customization-transitionend", …);
> and:
> gNavToolbox.addEventListener("customizationend", …);
> 
> But those both seemed to fire before the urlbar was unwrapped.  Maybe Gijs
> will know what event we should be waiting for is, so that we can call
> focusAndSelectUrlBar and have it actually focus the urlbar.  :)


"aftercustomization" on gNavToolbox should work? ( from http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#480 which is gCustomizeMode.exit)

You'd need to detect this by checking if the urlbar is in a toolbarpaletteitem, I guess...

(you could theoretically also check if it's disabled, but that likely won't be futureproof if we ever start disabling it for other reasons)

Comment 4

[Ray Lin[:ralin]]

Attachment: Bug-995758-Focus-new-tab-URLBar-in-Customization-mod.patch

Hi Gijs,

There's a serial of async tasks in gCustomizeMode.exit which to finish up Customization Mode, therefore focusAndSelectUrlBar will be triggered before exact gURLBar is ready.

As you said, listening to "aftercustomization" would able to focus gURLBar at right timing. This patch simply adds a listener to the event though I can not be sure if it is appropriate to handle this in the function. Could you give me some feedback? Thanks!

Comment 5

[:Gijs (he/him)]

Comment on attachment 8736257 [details] [diff] [review]
Bug-995758-Focus-new-tab-URLBar-in-Customization-mod.patch

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

Well, the callsite in this case seems to be from utilityOverlay.js' openLinkIn and friends, and that seems liable to change as well, so I guess that this is probably the most appropriate solution. Maybe you can add a comment about why we're doing this?

::: browser/base/content/browser.js
@@ +1831,5 @@
>  }
>  
>  function focusAndSelectUrlBar() {
> +  if (CustomizationHandler.isExitingCustomizeMode &&
> +      CustomizationHandler.isCustomizing()) {

Isn't just the first check here (for isExitingCustomizeMode) sufficient ?

@@ +1834,5 @@
> +  if (CustomizationHandler.isExitingCustomizeMode &&
> +      CustomizationHandler.isCustomizing()) {
> +    gNavToolbox.addEventListener("aftercustomization", function afterCustomize() {
> +      if (gURLBar) {
> +        gURLBar.select();

Hm, well, this should probably just re-call focusAndSelectUrlBar, because we need to handle the full screen case as well (you can be in full screen *and* in customize mode).

Comment 6

[Ray Lin[:ralin]]

Attachment: Bug-995758-Focus-new-tab-URLBar-in-Customization-mod.patch

Thanks for your feedback, Gijs. Could you help me to review this patch?

Comment 7

[:Gijs (he/him)]

Comment on attachment 8737072 [details] [diff] [review]
Bug-995758-Focus-new-tab-URLBar-in-Customization-mod.patch

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

::: browser/base/content/browser.js
@@ +1831,5 @@
>  }
>  
>  function focusAndSelectUrlBar() {
> +  // Bug 995758 - Capture focus if a new tab is created via Ctrl+T shortcut
> +  // while in Customization mode

The comment should explain why there is code here, not just reference the bug and its summary. The shortcut part of this is also not really important. Explain *why* we need to wait with the focusing of the URL bar until customization mode has finished.

@@ +1835,5 @@
> +  // while in Customization mode
> +  if (CustomizationHandler.isExitingCustomizeMode) {
> +    return gNavToolbox.addEventListener("aftercustomization", function afterCustomize() {
> +      focusAndSelectUrlBar();
> +      gNavToolbox.removeEventListener("aftercustomization", focusAndSelectUrlBar);

This doesn't work - you added an event listener referencing the 'afterCustomize' function, not the focusAndSelectUrlBar one, so you should remove the right listener.

Also, please put the listener removal before the call to focusAndSelectUrlBar. If the latter throws, for some reason, we wouldn't remove the listener, and that would be bad.

@@ +1836,5 @@
> +  if (CustomizationHandler.isExitingCustomizeMode) {
> +    return gNavToolbox.addEventListener("aftercustomization", function afterCustomize() {
> +      focusAndSelectUrlBar();
> +      gNavToolbox.removeEventListener("aftercustomization", focusAndSelectUrlBar);
> +    });

The previous patch returned true; why are you returning the result of the addEventListener call (undefined) now?

Comment 8

[Ray Lin[:ralin]]

Attachment: Bug-995758-Focus-new-tab-URLBar-in-Customization-mod-v2.patch

Thank for your reviewing, Gijs.
I've updated the patch, could you please review it again? Thanks!!

Please feel free to correct me if anything can be better, it helps me improve myself.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8737148 [details] [diff] [review]
Bug-995758-Focus-new-tab-URLBar-in-Customization-mod-v2.patch

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

::: browser/base/content/browser.js
@@ +1831,5 @@
>  }
>  
>  function focusAndSelectUrlBar() {
> +  // If we open a new tab in CustomizeMode, this function will be called
> +  // before new tab's URLBar is ready. Therefore, we need to wait for the

Instead of "is ready", can we describe more precisely what is happening? How about:

In customize mode, the url bar is disabled. If a new tab is opened or the user switches to a different tab, this function gets called before we've finished leaving customize mode, and the url bar will still be disabled. We can't focus it when it's disabled, so we need to re-run ourselves when we've finished leaving customize mode.

@@ +1840,5 @@
> +      gNavToolbox.removeEventListener("aftercustomization", afterCustomize);
> +      focusAndSelectUrlBar();
> +    });
> +
> +    return true;

Did you test this change and the previous version of this patch (that returned the result of addEventListener)? Can you explain why we're returning true rather than false/falsy-values? It seems like the callsites use the return value, so it is not unimportant to get it right, for some meaning of the word. :-)

Comment 10

[Ray Lin[:ralin]]

Attachment: Bug-995758-Focus-new-tab-URLBar-in-Customization-mod-v3.patch

The comment is much more precise than mine. :)

I think focusAndSelectUrlBar explicitly returns value is because some functions use it to decide next action. The case return false is only when gURLBar is unavailable, so we need to ensure behavior retains consistency. 

I made comment change to patch, and I'm grateful to have your guidance and explanation . Could you help me to review again? Thanks!!

Comment 11

[:Gijs (he/him)]

Comment on attachment 8738380 [details] [diff] [review]
Bug-995758-Focus-new-tab-URLBar-in-Customization-mod-v3.patch

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

Thanks!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2d8f9e9a35d7

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2d8f9e9a35d7

Comment 14

[Nazir Ahmed Sabbir [:NaSb]]

I've managed to reproduce this bug on Nightly 31.0a1 (2014-04-14) on Linux, 64 Bit.

This Bug is now verified as fixed on Firefox Developer Edition 48.0a2 (2016-05-09) 

Build ID: 20160509004024
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
OS: Linux 4.4.0-21-generic x86-64