Closed Bug 1054931 Opened 5 years ago Closed 5 years ago

Ctrl-K focuses an invisible search bar in about:newtab in "blank" mode

Categories

(Firefox :: Search, defect)

defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.3
Tracking Status
firefox34 + verified

People

(Reporter: protz, Assigned: alexbardas)

References

Details

Attachments

(1 file, 1 obsolete file)

STR:
- Ctrl-T (opens up a new tab)
- Switch (using the top-right gears button) to "blank" mode
- Hit Ctrl-K (invisible search box gets focused)
- Hit down arrow

Result:
- The search suggestions are shown in a dropdown menu, even though the initial search field is hidden.
Flags: firefox-backlog+
Element with id "newtab-search-container" gets a page-disabled="true" attribute which sets its opacity to 0, but the search input is still in the page and gets focused when CMD + K is pressed.

This is where the "page-disable" attribute is added / removed: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/page.js#146 .

We will also need a new test in browser/base/content/test/newtab/browser_newtab_search.js for this particular case.
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Thanks for taking this, Alex.  I mentioned something about e10s when we spoke briefly about this, but actually I think we don't have to worry about that right now.  That's because this module called NewTabUtils.jsm tracks a bunch of state related to all the newtab pages.  We should just use that in this bug.  Specifically you want to use the AllPages.enabled getter instead of checking the "page-disable" attribute on the customize button in the DOM: http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/NewTabUtils.jsm?rev=de9d70a14a16#240

So I think this bug should be pretty simple (aside from the test).  browser.js doesn't import NewTabUtils.jsm yet, so you'll need to add a lazy import for it.
Thanks for the info Drew.

Hopefully, unit tests cover all cases now.
Attachment #8475249 - Flags: review?(adw)
Flags: qe-verify?
Comment on attachment 8475249 [details] [diff] [review]
Ctrl / Cmd + K will open an about:home page with the search bar focused when newtab is in blank mode

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

Cool, thanks.  Please push to try and then add checkin-needed if try is OK.
Attachment #8475249 - Flags: review?(adw) → review+
Thanks Drew. I'm waiting for the test results here: https://tbpl.mozilla.org/?tree=Try&rev=0a44b508fcbb. I'll update the bug shortly after they finish running.
(In reply to Alex Bardas :alexbardas from comment #5)
> Thanks Drew. I'm waiting for the test results here:
> https://tbpl.mozilla.org/?tree=Try&rev=0a44b508fcbb. I'll update the bug
> shortly after they finish running.

I've canceled those because they were not running on enough problems. New tests here:
https://tbpl.mozilla.org/?tree=Try&rev=fb13891c0335
Iteration: --- → 34.3
QA Contact: cornel.ionce
Flags: qe-verify? → qe-verify+
Made setTimeout be in the same scope as clearTimeout.
Attachment #8475249 - Attachment is obsolete: true
Attachment #8475451 - Flags: review+
There was a minor problem solved in the previous patch. The try looks good: https://tbpl.mozilla.org/?tree=Try&rev=d60d164dd1d1 .
Good catch!  Let's add checkin-need once most of the try results come in.
Points: --- → 2
Try's taking forever and it looks OK so far, checkin-needed.
Keywords: checkin-needed
OS: Linux → All
Hardware: x86_64 → All
https://hg.mozilla.org/integration/fx-team/rev/afa010e8f62b

In the future, keep in mind that your commit message should be summarizing what the patch is doing, not restating the problem :)
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> https://hg.mozilla.org/integration/fx-team/rev/afa010e8f62b
> 
> In the future, keep in mind that your commit message should be summarizing
> what the patch is doing, not restating the problem :)
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Committing_Rules_and_Responsibilities#Checkin_comment

I'll keep that in mind Ryan, thank you for the link :)
https://hg.mozilla.org/mozilla-central/rev/afa010e8f62b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Assigning to Catalin since he handled bug 1041678 (and to balance load a bit).
QA Contact: cornel.ionce → catalin.varga
Verified fixed using latest Nightly, build ID: 20140821030201 on Mac OS X 10.9.4, Ubuntu 14.04 and Windows 7 64bit.

When about:newtab is set to blank, the search bar can't be selected anymore neither in normal/private (by typing about:newtab) nor e10s windows.
Status: RESOLVED → VERIFIED
QA Contact: catalin.varga → cornel.ionce
You need to log in before you can comment on or make changes to this bug.