Closed
Bug 1054931
Opened 10 years ago
Closed 10 years ago
Ctrl-K focuses an invisible search bar in about:newtab in "blank" mode
Categories
(Firefox :: Search, defect)
Firefox
Search
Tracking
()
People
(Reporter: protz, Assigned: alexbardas)
References
Details
Attachments
(1 file, 1 obsolete file)
Ctrl / Cmd + K will open an about:home page with the search bar focused when newtab is in blank mode
6.86 KB,
patch
|
alexbardas
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
status-firefox34:
--- → affected
tracking-firefox34:
--- → +
Updated•10 years ago
|
Flags: firefox-backlog+
Assignee | ||
Comment 1•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
Thanks for the info Drew. Hopefully, unit tests cover all cases now.
Attachment #8475249 -
Flags: review?(adw)
Updated•10 years ago
|
Flags: qe-verify?
Comment 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
(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
Updated•10 years ago
|
Iteration: --- → 34.3
Updated•10 years ago
|
QA Contact: cornel.ionce
Updated•10 years ago
|
Flags: qe-verify? → qe-verify+
Assignee | ||
Comment 7•10 years ago
|
||
Made setTimeout be in the same scope as clearTimeout.
Attachment #8475249 -
Attachment is obsolete: true
Attachment #8475451 -
Flags: review+
Assignee | ||
Comment 8•10 years ago
|
||
There was a minor problem solved in the previous patch. The try looks good: https://tbpl.mozilla.org/?tree=Try&rev=d60d164dd1d1 .
Comment 9•10 years ago
|
||
Good catch! Let's add checkin-need once most of the try results come in.
Updated•10 years ago
|
Points: --- → 2
Comment 10•10 years ago
|
||
Try's taking forever and it looks OK so far, checkin-needed.
Comment 11•10 years ago
|
||
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
Assignee | ||
Comment 12•10 years ago
|
||
(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 :)
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/afa010e8f62b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Comment 14•10 years ago
|
||
Assigning to Catalin since he handled bug 1041678 (and to balance load a bit).
QA Contact: cornel.ionce → catalin.varga
Comment 15•10 years ago
|
||
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.
Description
•