Closed Bug 1028985 Opened 6 years ago Closed 6 years ago

Provide search suggestions on Firefox new tab page (about:newtab)

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.1
Tracking Status
firefox33 --- verified
firefox34 --- verified

People

(Reporter: madhava, Assigned: adw)

References

(Depends on 2 open bugs)

Details

(Keywords: feature)

Attachments

(2 files, 4 obsolete files)

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.
For the record, here is the latest look and feel, from bug 612453:

https://bug612453.bugzilla.mozilla.org/attachment.cgi?id=8419799
Flags: firefox-backlog+
QA Whiteboard: [qa+]
Assignee: nobody → mano
Iteration: --- → 33.2
Points: --- → 8
Is this different from bug 1015269?
Component: Search → New Tab Page
OS: Mac OS X → All
Hardware: x86 → All
Not really. Mike, are you still working on that one?
Flags: needinfo?(mconnor)
Status: NEW → ASSIGNED
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)
Duplicate of this bug: 1015269
QA Contact: petruta.rasa
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. :)
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)
Removed from Iteration 33.2
Iteration: 33.2 → ---
Flags: needinfo?(mmucci)
Depends on: 612453
Assignee: nobody → adw
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?
Assignee: adw → nobody
Attached patch WIP patch, depends on bug 612453 (obsolete) — Splinter Review
Assignee: nobody → adw
Status: NEW → ASSIGNED
Attachment #8460310 - Attachment is patch: true
Iteration: --- → 34.1
Attached patch patch (obsolete) — Splinter Review
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)
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
Attached patch patch v2 (obsolete) — Splinter Review
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)
Attachment #8462947 - Attachment is patch: true
Attached patch patch v2.1 (obsolete) — Splinter Review
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)
Attachment #8462949 - Attachment is patch: true
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+
Attached patch patch v3Splinter Review
(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
https://hg.mozilla.org/mozilla-central/rev/fe53a2b0e7d0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
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!]
Blocks: 1049799
Already verified in Nightly 34 (comment 20).
Attached patch Aurora/33 patchSplinter Review
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?
Attachment #8478724 - Attachment is patch: true
Attachment #8478724 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Depends on: 1060846
Depends on: 1060847
Depends on: 1060845
Depends on: 1060888
No longer blocks: 1071558
Depends on: 1071558
Depends on: 1089727
Depends on: 1097667
No longer depends on: 1097667
Depends on: 1093571
Depends on: 1096534
No longer depends on: 1096534
Depends on: 1096534
please see Bug 1092957
You need to log in before you can comment on or make changes to this bug.