Closed Bug 1101147 Opened 5 years ago Closed 5 years ago

Update about:newtab search styling

Categories

(Firefox :: General, defect)

defect
Not set
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
37.1
Tracking Status
firefox34 + verified
firefox35 + verified
firefox36 + verified

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

Attachments

(2 files, 10 obsolete files)

13.98 KB, patch
mossop
: review+
Details | Diff | Splinter Review
13.76 KB, patch
Felipe
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch Remove engines from dropdown (obsolete) — Splinter Review
Remove the engines from the dropdown, just leaving the "Manage search engines" entry
Attachment #8524869 - Flags: review?(dtownsend+bugmail)
Attached patch Dead code in ContentSearch (obsolete) — Splinter Review
Remove some dead code from ContentSearch now that it's not necessary to send the list of engines
Attachment #8524870 - Flags: review?(dtownsend+bugmail)
Make the "Manage Search Engines" entry open the preferences dialog in the right pane
Attachment #8524873 - Flags: review?(dtownsend+bugmail)
Attachment #8524874 - Flags: review?(dtownsend+bugmail)
I haven't tested yet which tests are broken due to some of the feature removal, will do that next
Comment on attachment 8524869 [details] [diff] [review]
Remove engines from dropdown

Does this break any tests?
Attachment #8524869 - Flags: review?(dtownsend+bugmail) → review+
Attachment #8524870 - Flags: review?(dtownsend+bugmail) → review+
Attachment #8524873 - Flags: review?(dtownsend+bugmail) → review+
Attachment #8524874 - Flags: review?(dtownsend+bugmail) → review+
Attached patch Adjust browser_ContentSearch (obsolete) — Splinter Review
Adjustments for browser_ContentSearch which was testing some of the stuff that got removed.
The addition of "engine-changed" in the observer in ContentSearch.jsm is because this test tests the scenario if the engine name changing (without changing which engine is the selected one)
Attachment #8525473 - Flags: review?(dtownsend+bugmail)
Attached patch Magnifying glass (obsolete) — Splinter Review
And an interdiff with the CSS changes to have the magnifying glass as the icon
Attachment #8525476 - Flags: review?(dtownsend+bugmail)
Attachment #8525473 - Flags: review?(dtownsend+bugmail) → review+
Comment on attachment 8525476 [details] [diff] [review]
Magnifying glass

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

r+ but check with UX on the specific values.

::: browser/base/content/newtab/newTab.css
@@ +340,2 @@
>    border: 1px solid transparent;
>    -moz-margin-end: 8px;

So in about:home I ended up going with 42px height and width and a margin-end of 5px. We should probably make the two match but I didn't get specific input from UX on what those values should be. Can you make sure UX are happy with these numbers and then I'll copy them into the about:home patch

@@ +345,1 @@
>    /* XXX TODO felipe replace this with magnifying glass and adjust size properties */

This TODO can go away now
Attachment #8525476 - Flags: review?(dtownsend+bugmail) → review+
Updated patch with css tweaks
Attachment #8524874 - Attachment is obsolete: true
Attachment #8525476 - Attachment is obsolete: true
Attachment #8525549 - Flags: review+
Group: mozilla-employee-confidential
Gavin asked to to take a peek at this and other bugs.

This generally looks fine, although I don't understand why we're removing UI to select a search engine (without having to jump through heavyweight UI to manage the default search engine). Feels like a bit of a backwards step for user choice, but maybe I'm missing some context.

Is there any plan to have the autocomplete here use the new UI from bug 1088660 (as in the screenshot in attachment 8511110 [details])? That would address that concern, as well as make for a consistent UX.
Attached patch Adjust browser_newtab_search (obsolete) — Splinter Review
Some straightforward adjustments for test browser_newtab_search which was testing the following:
 - popup panel contains the list of engines, and the proper current engine is selected
 - logo displays the correct engine logo

These two parts have been removed, the remaining of the test continues to pass with no changes
Attachment #8526013 - Flags: review?(dtownsend+bugmail)
Attached patch Folded patch for beta, r=Mossop (obsolete) — Splinter Review
This is a folded patch for beta with everything from this bug. It goes on top of the patch from bug 1101122 because that one is adding the magnifying glass images also used here
Attachment #8526015 - Flags: review+
(In reply to Justin Dolske [:Dolske] from comment #12)
> Is there any plan to have the autocomplete here use the new UI from bug
> 1088660 (as in the screenshot in attachment 8511110 [details])? That would
> address that concern, as well as make for a consistent UX.

Yeah, my understanding is that the plan is to unify the autocomplete panel and make them look like the one from the searchbar. I don't know if there are bugs open for that.
Attachment #8526013 - Flags: review?(dtownsend+bugmail) → review+
Instead of working on top of the tree, I first backed out the original patch landed here and created a new one, since otherwise half of the patch would be adding a bunch of code back
Attachment #8527071 - Flags: review?(dtownsend+bugmail)
missed hg qref, sorry
Attachment #8524869 - Attachment is obsolete: true
Attachment #8524870 - Attachment is obsolete: true
Attachment #8524873 - Attachment is obsolete: true
Attachment #8525473 - Attachment is obsolete: true
Attachment #8525549 - Attachment is obsolete: true
Attachment #8526013 - Attachment is obsolete: true
Attachment #8526015 - Attachment is obsolete: true
Attachment #8527071 - Attachment is obsolete: true
Attachment #8527071 - Flags: review?(dtownsend+bugmail)
Attachment #8527072 - Flags: review?(dtownsend+bugmail)
Attachment #8527072 - Flags: review?(dtownsend+bugmail) → review+
No longer depends on: 1103127
The existing test is working fine, just the new test that I added is failing on try (and passing locally, of course), but I don't want to block this landing on that, so I removed it from this patch to look into it with more time.
Attachment #8527246 - Flags: review+
Attachment #8527246 - Flags: approval-mozilla-beta?
Attachment #8527246 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Pushed the new version with the switchable UI based on browser.search.showOneOffButtons

https://hg.mozilla.org/releases/mozilla-beta/rev/65fa22861cad
https://hg.mozilla.org/releases/mozilla-beta/rev/eb7826389d23
QA Contact: petruta.rasa
The search field in about:newtab page only contains the magnifier glass with the "Manage Search Engines.." when clicked, the field is empty. Verified using Firefox 34.0 RC build (20141124205320) under Win 7 64-bit, Ubuntu 14.04 32-bit and Mac OSX 10.9.5.
Points: --- → 8
Flags: qe-verify?
Flags: firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/f1798d79a96f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Target Milestone: Firefox 37 → Firefox 36
Iteration: --- → 37.1
Depends on: 1106239
Flags: qe-verify? → qe-verify+
Depends on: 1107471
Depends on: 1108254
"Change Search Settings" drop-down is displayed when clicking the magnifying glass in about:newtab page using Firefox Developer Edition 36.0a2 2014-12-07 under Win 7 64-bit, Ubuntu 12.04 and Mac OSX 10.10.
For Firefox 35 beta 1 (20141201162954) the string is "Manage Search Engines...".
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.