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.

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

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. :)

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

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

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

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?

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 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. :-)

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 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!

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