Closed
Bug 1028985
Opened 10 years ago
Closed 10 years ago
Provide search suggestions on Firefox new tab page (about:newtab)
Categories
(Firefox :: New Tab Page, defect)
Firefox
New Tab Page
Tracking
()
People
(Reporter: madhava, Assigned: adw)
References
(Depends on 1 open bug)
Details
(Keywords: feature)
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The new search field on about:newtab should provide search suggest. It should behave as the about:home search field changes happening in bug 612453.
Reporter | ||
Comment 1•10 years ago
|
||
For the record, here is the latest look and feel, from bug 612453:
https://bug612453.bugzilla.mozilla.org/attachment.cgi?id=8419799
Updated•10 years ago
|
Flags: firefox-backlog+
Updated•10 years ago
|
QA Whiteboard: [qa+]
Updated•10 years ago
|
Assignee: nobody → mano
Iteration: --- → 33.2
Points: --- → 8
Assignee | ||
Comment 2•10 years ago
|
||
Is this different from bug 1015269?
Assignee | ||
Updated•10 years ago
|
Component: Search → New Tab Page
OS: Mac OS X → All
Hardware: x86 → All
Comment 3•10 years ago
|
||
Not really. Mike, are you still working on that one?
Flags: needinfo?(mconnor)
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 4•10 years ago
|
||
I didn't know the longer term goal was for about:newtab to be unprivileged, so copying the about:home stuff feels like a better plan. Will dupe forward.
Flags: needinfo?(mconnor)
Updated•10 years ago
|
QA Contact: petruta.rasa
Comment 6•10 years ago
|
||
Please request a11y-review from me on the patch when it's ready, too, so we can make sure this feature is accessible to keyboard and screen reader users when it lands. Look in bug 612453 for how I did it there, too. :)
Comment 7•10 years ago
|
||
Marco, can you please drop this from the current iteration? I talked to Gavin and this isn't ready yet. It needs to wait for bug 612453 as that will provide some basic infrastructure. Sorry, Mano.
Assignee: mano → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mmucci)
Updated•10 years ago
|
Assignee: nobody → adw
Reporter | ||
Comment 9•10 years ago
|
||
I hear that, do to refactoring going to to resolve bug 612453, that this would be relatively easy to do as well. Is that the case?
Comment 10•10 years ago
|
||
yes
Updated•10 years ago
|
Assignee: adw → nobody
Assignee | ||
Comment 11•10 years ago
|
||
Assignee: nobody → adw
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Attachment #8460310 -
Attachment is patch: true
Updated•10 years ago
|
Iteration: --- → 34.1
Assignee | ||
Comment 12•10 years ago
|
||
Based on the patches in bug 612453.
https://tbpl.mozilla.org/?tree=Try&rev=b66b8f35b8ef
Attachment #8460310 -
Attachment is obsolete: true
Attachment #8462756 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 13•10 years ago
|
||
There was one intermittent failure on Linux x64 Debug, when the test opens the search panel and checks it. It gets stuck waiting for the panel to open. At that time, the current engine is the suggestions engine, which this patch added, and the suggestion engine doesn't have a logo, so there's no predetermined place for the panel to popup. (When I run locally, I see it pop up near the bottom right corner of the page.)
I changed the test to stop doing that if the current engine doesn't have a logo. Try looks better so far:
https://tbpl.mozilla.org/?tree=Try&rev=09a2ebc1e40b
Assignee | ||
Comment 14•10 years ago
|
||
Here's the updated patch as described in the previous comment.
Attachment #8462756 -
Attachment is obsolete: true
Attachment #8462756 -
Flags: review?(MattN+bmo)
Attachment #8462947 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8462947 -
Attachment is patch: true
Assignee | ||
Comment 15•10 years ago
|
||
This is equivalent to the previous but preserves blame better. Sorry for the churn.
Attachment #8462947 -
Attachment is obsolete: true
Attachment #8462947 -
Flags: review?(MattN+bmo)
Attachment #8462949 -
Flags: review?(MattN+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8462949 -
Attachment is patch: true
Comment 16•10 years ago
|
||
Comment on attachment 8462949 [details] [diff] [review]
patch v2.1
Review of attachment 8462949 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with comments.
You'll probably want to adjust the commit message too.
::: browser/base/content/newtab/newTab.css
@@ +394,5 @@
> }
> +
> +.searchSuggestionTable {
> + font: message-box;
> + font-size: 16px;
Why not em so it zooms?
::: browser/base/content/newtab/newTab.xul
@@ +5,4 @@
> - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>
> <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/content/searchSuggestionUI.css" type="text/css"?>
In the dependency I just commented that this should get appended dynamically with <style scoped>.
::: browser/base/content/newtab/search.js
@@ +55,5 @@
> },
>
> handleEvent: function (event) {
> + let methodName = "on" + event.detail.type;
> + if (methodName in this) {
hasOwnProperty would be somewhat safer to avoid accidentally calling one of the functions on a default event handler property e.g. if |this| was not what was expected.
::: browser/base/content/test/newtab/browser.ini
@@ +34,5 @@
> searchEngine1xLogo.xml
> searchEngine2xLogo.xml
> searchEngine1x2xLogo.xml
> + searchEngineSuggestions.xml
> + searchEngineSuggestions.sjs
I don't think we need a third copy of these files. You should be able to use relative paths "../"… to use the same files.
Attachment #8462949 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #16)
> ::: browser/base/content/newtab/newTab.css
> @@ +394,5 @@
> > }
> > +
> > +.searchSuggestionTable {
> > + font: message-box;
> > + font-size: 16px;
>
> Why not em so it zooms?
So it matches the input selector at the top of the file. If by zooms you mean Cmd-+/-, it does zoom when you do that.
> ::: browser/base/content/newtab/newTab.xul
> @@ +5,4 @@
> > - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> >
> > <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
> > +<?xml-stylesheet href="chrome://browser/content/searchSuggestionUI.css" type="text/css"?>
>
> In the dependency I just commented that this should get appended dynamically
> with <style scoped>.
It didn't work unfortunately (bug 612453 comment 85).
> ::: browser/base/content/newtab/search.js
> @@ +55,5 @@
> > },
> >
> > handleEvent: function (event) {
> > + let methodName = "on" + event.detail.type;
> > + if (methodName in this) {
>
> hasOwnProperty would be somewhat safer to avoid accidentally calling one of
> the functions on a default event handler property e.g. if |this| was not
> what was expected.
Done.
> ::: browser/base/content/test/newtab/browser.ini
> @@ +34,5 @@
> > searchEngine1xLogo.xml
> > searchEngine2xLogo.xml
> > searchEngine1x2xLogo.xml
> > + searchEngineSuggestions.xml
> > + searchEngineSuggestions.sjs
>
> I don't think we need a third copy of these files. You should be able to use
> relative paths "../"… to use the same files.
Removed these copies and changed browser.ini to ../general/searchSuggestionEngine.xml and ../general/searchSuggestionEngine.sjs.
Attachment #8462949 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
Comment 19•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Updated•10 years ago
|
Updated•10 years ago
|
Comment 20•10 years ago
|
||
Verified using Nightly 34.0a1 20140804030205 under Win 7 64-bit, Mac OSX 10.9.4 and Ubuntu 12.10 32-bit: search suggestions are displayed on about:newtab page for all default search engines except Twitter. Logged separate issues for the bugs found.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Updated•10 years ago
|
Comment 21•10 years ago
|
||
Already verified in Nightly 34 (comment 20).
Assignee | ||
Comment 22•10 years ago
|
||
This isn't substantially different from patch v3, just updates some line numbers to apply totally cleanly.
Bug 1054516 is the uplift bug for this feature.
mozilla-aurora try push with this and patches for other bugs mentioned in bug 1054516: https://tbpl.mozilla.org/?tree=Try&rev=fba51f37ff40
Approval Request Comment
[Feature/regressing bug #]:
this bug, search suggestions in about:newtab
[User impact if declined]:
no search suggestions in about:newtab
[Describe test coverage new/current, TBPL]:
currently covered by automated tests on 34; this patch adds tests to 33; see also try link above
[Risks and why]:
moderate risk mediated by aforementioned testing; modifies the about:newtab page; we want about:newtab search suggestions on 33 as stated in bug 1054516
[String/UUID change made/needed]:
none
Attachment #8478724 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Attachment #8478724 -
Attachment is patch: true
Updated•10 years ago
|
Attachment #8478724 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 23•10 years ago
|
||
Comment 24•10 years ago
|
||
Verified that the search suggestions are displayed on about:newtab page using latest Aurora 33.0a2 (20140828004002) under Win 7 64-bit, Ubuntu 13.04 32-bit and Mac OSX 10.8.5.
Keywords: verifyme
Assignee | ||
Updated•10 years ago
|
Comment 25•10 years ago
|
||
please see Bug 1092957
You need to log in
before you can comment on or make changes to this bug.
Description
•