Closed Bug 1106559 Opened 9 years ago Closed 9 years ago

Improve the search preference UI (add ability to edit keywords, reorder/remove engines, restore defaults)

Categories

(Firefox :: Search, defect)

defect
Not set
normal
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.2
Tracking Status
firefox35 + verified
firefox36 + verified
firefox37 + verified
relnote-firefox --- 35+

People

(Reporter: gaston, Assigned: florian)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

Seen in latest 34.0 beta, 34.0 rc1 and rc2 - there's no way to add/modify search keywords. There's also no way to reorder the search engines in the new prefs pane.
As pointed out by :RattyAway on irc, one can set browser.search.showOneOffButtons to false in about:config to get the previous search engine manager back.
(In reply to Landry Breuil (:gaston) from comment #1)
> As pointed out by :RattyAway on irc, one can set
> browser.search.showOneOffButtons to false in about:config to get the
> previous search engine manager back.

You can also just load chrome://browser/content/search/engineManager.xul in a tab.
Depends on: 1106055
OS: OpenBSD → All
Hardware: x86_64 → All
Version: 34 Branch → Trunk
Flags: firefox-backlog+
Attached patch Patch v1 (obsolete) — Splinter Review
The theming still needs love, but apart from that I think it's ready for review.
Assignee: nobody → florian
Status: NEW → ASSIGNED
Attachment #8534637 - Flags: review?(felipc)
Points: --- → 8
Flags: qe-verify+
Iteration: --- → 37.2
Attached patch Strings onlySplinter Review
Approval Request Comment
[Feature/regressing bug #]: bug 1088660. 
[User impact if declined]: No way to edit search keywords in Firefox 35/36.
[Describe test coverage new/current, TBPL]: N/A, string only patch.
[Risks and why]: N/A
[String/UUID change made/needed]: This adds strings, after discussing with gavin/Pike/flod/chofmann this is the right thing to do for 35.
Attachment #8535295 - Flags: review?(felipc)
Attachment #8535295 - Flags: approval-mozilla-beta?
Attachment #8535295 - Flags: approval-mozilla-aurora?
Attachment #8535295 - Flags: approval-mozilla-beta?
Attachment #8535295 - Flags: approval-mozilla-beta+
Attachment #8535295 - Flags: approval-mozilla-aurora?
Attachment #8535295 - Flags: approval-mozilla-aurora+
Attachment #8535295 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/1e535a5222fb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Not yet fixed in Nightly as well as Beta, Aurora.
Status: RESOLVED → REOPENED
Flags: needinfo?(kwierso)
Resolution: FIXED → ---
Flags: needinfo?(florian)
[Tracking Requested - why for this release]:
Status: REOPENED → ASSIGNED
Flags: needinfo?(kwierso)
Flags: needinfo?(florian)
>> +<!ENTITY restoreDefaultSearchEngines.accesskey "d">
>> +<!ENTITY removeEngine.accesskey                "r">

This may sound like nitpicking and isn't vital here, but please use matching cases for access keys in general to avoid unintended current or future behavior and sometimes confusing localizers (and note this when reviewing).
Attaching an interdiff of the additional change for easier review by felipe.
Summary: No way to change search keywords after bug 1088660 → Improve the search preference UI
Attached patch Patch v2 (obsolete) — Splinter Review
I'll do the in-content UI version of this in a follow-up.

Dão, I think Felipe isn't fully comfortable reviewing the theming changes here; would you like to do a quick review of them?
Flags: needinfo?(dao)
Attachment #8536906 - Flags: review?(felipc)
Comment on attachment 8536906 [details] [diff] [review]
Patch v2

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

::: browser/components/preferences/search.js
@@ +82,5 @@
>  
> +  onTreeKeyPress: function(aEvent) {
> +    let index = gEngineView.selectedIndex;
> +    let tree = document.getElementById("engineList");
> +    if (aEvent.charCode == KeyEvent.DOM_VK_SPACE) {

Add a comment here explaining what is VK_SPACE doing

@@ +88,5 @@
> +      gEngineView.setCellValue(index, tree.columns.getFirstColumn(),
> +                               newValue.toString());
> +    }
> +    else {
> +      let isMac = ("nsILocalFileMac" in Components.interfaces);

Use Services.appinfo.OS ?
Attachment #8536906 - Flags: review?(felipc) → review+
Attachment #8534637 - Flags: review?(felipc)
Attachment #8534637 - Attachment is obsolete: true
Attachment #8536901 - Attachment is obsolete: true
Comment on attachment 8536906 [details] [diff] [review]
Patch v2

>--- /dev/null
>+++ b/browser/themes/linux/preferences/search.css

>+.searchengine-menuitem > .menu-iconic-left {
>+  display: -moz-box;
>+}

Use the menuitem-with-favicon class?

>+#engineList treechildren::-moz-tree-checkbox {
>+  list-style-image: url("chrome://browser/skin/checkbox.png");
>+  -moz-image-region: rect(0, 16px, 16px, 0);
>+}

This checkbox looks awfully alien over here. Why not just keep the standard appearance? It's not clear to me why these checkboxes should be different.

>+#engineList treechildren::-moz-tree-row {
>+  height: 20px;
>+}

Can you explain what this is trying to do?
Flags: needinfo?(dao)
Attachment #8536906 - Flags: review-
(In reply to Dão Gottwald [:dao] from comment #17)
> >+#engineList treechildren::-moz-tree-checkbox {
> >+  list-style-image: url("chrome://browser/skin/checkbox.png");
> >+  -moz-image-region: rect(0, 16px, 16px, 0);
> >+}
> 
> This checkbox looks awfully alien over here. Why not just keep the standard
> appearance? It's not clear to me why these checkboxes should be different.

So I missed that you switched from a richlist to a classic tree. I'm not sure that's a great choice. Is there any way we can use the standard checkbox appearance there?
(In reply to Dão Gottwald [:dao] from comment #18)

> Is there any way we can use the standard checkbox appearance there?

No, see bug 462618.
(In reply to Dão Gottwald [:dao] from comment #17)
> Comment on attachment 8536906 [details] [diff] [review]
> Patch v2
> 
> >--- /dev/null
> >+++ b/browser/themes/linux/preferences/search.css
> 
> >+.searchengine-menuitem > .menu-iconic-left {
> >+  display: -moz-box;
> >+}
> 
> Use the menuitem-with-favicon class?

If I mxr that class, the only 2 CSS rules I find are:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#510
510   .menuitem-with-favicon > .menu-iconic-left > .menu-iconic-icon {
511     image-rendering: -moz-crisp-edges;
512   }

and
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#370
370   :-moz-any(menuitem[type], .menuitem-with-favicon) > .menu-iconic-left {
371     visibility: visible;
372   }

I'm afraid neither of these rules will help with "display".


> >+#engineList treechildren::-moz-tree-row {
> >+  height: 20px;
> >+}
> 
> Can you explain what this is trying to do?

It's copied from http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/engineManager.css#14
It ensures the height of the tree rows is large enough for a 16x16px provider icon to be displayed.
(In reply to Florian Quèze [:florian] [:flo] from comment #20)
> > >+.searchengine-menuitem > .menu-iconic-left {
> > >+  display: -moz-box;
> > >+}
> > 
> > Use the menuitem-with-favicon class?
> 
> If I mxr that class, the only 2 CSS rules I find are:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> css#510
> 510   .menuitem-with-favicon > .menu-iconic-left > .menu-iconic-icon {
> 511     image-rendering: -moz-crisp-edges;
> 512   }
> 
> and
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#370
> 370   :-moz-any(menuitem[type], .menuitem-with-favicon) > .menu-iconic-left {
> 371     visibility: visible;
> 372   }
> 
> I'm afraid neither of these rules will help with "display".

So why is this code needed in the first place? I blindly assumed you wanted to work around the fact that we normally hide menu-iconic-left on GTK.

> > >+#engineList treechildren::-moz-tree-row {
> > >+  height: 20px;
> > >+}
> > 
> > Can you explain what this is trying to do?
> 
> It's copied from
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/
> engineManager.css#14
> It ensures the height of the tree rows is large enough for a 16x16px
> provider icon to be displayed.

Does it work like a min-height here? I.e. will the rows grow for larger text?

> > Is there any way we can use the standard checkbox appearance there?
> 
> No, see bug 462618.

Can we use an unboxed check mark then, like we do in other XUL trees? Mimicking a real checkbox and failing half-way looks rather cheap.
(In reply to Dão Gottwald [:dao] from comment #21)
> (In reply to Florian Quèze [:florian] [:flo] from comment #20)
> > > >+.searchengine-menuitem > .menu-iconic-left {
> > > >+  display: -moz-box;
> > > >+}
> > > 
> > > Use the menuitem-with-favicon class?
> > 
> > If I mxr that class, the only 2 CSS rules I find are:
> > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> > css#510
> > 510   .menuitem-with-favicon > .menu-iconic-left > .menu-iconic-icon {
> > 511     image-rendering: -moz-crisp-edges;
> > 512   }
> > 
> > and
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#370
> > 370   :-moz-any(menuitem[type], .menuitem-with-favicon) > .menu-iconic-left {
> > 371     visibility: visible;
> > 372   }
> > 
> > I'm afraid neither of these rules will help with "display".
> 
> So why is this code needed in the first place? I blindly assumed you wanted
> to work around the fact that we normally hide menu-iconic-left on GTK.

I work around http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/menu.css#151
151 menulist > menupopup > menuitem > .menu-iconic-left,
152 menulist > menupopup > menu > .menu-iconic-left {
153   display: none;
154 }


> > It's copied from
> > http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/
> > engineManager.css#14
> > It ensures the height of the tree rows is large enough for a 16x16px
> > provider icon to be displayed.
> 
> Does it work like a min-height here? I.e. will the rows grow for larger text?

I think so. It's just ported from the existing engine manager.

> > > Is there any way we can use the standard checkbox appearance there?
> > 
> > No, see bug 462618.
> 
> Can we use an unboxed check mark then, like we do in other XUL trees?

The check mark we use in other XUL trees (afaik only the session restore one) is absolutely awful. Bug 462618 was actually filed because UX wanted these checkboxes to look native. UX also asked for native checkboxes here, and provided the images I needed.

However, I'm happy to use your suggestion of unboxed check marks for the in-content version.

> Mimicking a real checkbox and failing half-way looks rather cheap.

On Windows and Mac I think these checkboxes will look like the native ones in most cases.
(In reply to Florian Quèze [:florian] [:flo] from comment #22)
> I work around
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> menu.css#151
> 151 menulist > menupopup > menuitem > .menu-iconic-left,
> 152 menulist > menupopup > menu > .menu-iconic-left {
> 153   display: none;
> 154 }

Have you tried to find out what the purpose of that rule is? Does it exist only on Linux and if so, why?

> > Does it work like a min-height here? I.e. will the rows grow for larger text?
> 
> I think so. It's just ported from the existing engine manager.

Could you please test this?

> > Can we use an unboxed check mark then, like we do in other XUL trees?
> 
> The check mark we use in other XUL trees (afaik only the session restore
> one) is absolutely awful.

You mean the particular check mark image is awful, or that there's no box around it? The former can be fixed easily. I'd be careful with statements like "absolutely awful", because what I saw when opening the preferences window with this patch applied looked quite a bit more jarring than e.g. about:sessionrestore...

> > Mimicking a real checkbox and failing half-way looks rather cheap.
> 
> On Windows and Mac I think these checkboxes will look like the native ones
> in most cases.

Besides most cases, there's a long tail of other cases on Windows, and then there's Linux (also a tier 1 platform). I don't think this meets our quality bar there.
(In reply to Dão Gottwald [:dao] from comment #23)
> (In reply to Florian Quèze [:florian] [:flo] from comment #22)
> > I work around
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> > menu.css#151
> > 151 menulist > menupopup > menuitem > .menu-iconic-left,
> > 152 menulist > menupopup > menu > .menu-iconic-left {
> > 153   display: none;
> > 154 }
> 
> Have you tried to find out what the purpose of that rule is?

It's already part of the first revision in hg. I don't think this is worth digging in CVS history right now.

> Does it exist only on Linux and if so, why?

Linux only, I don't know why.

> > > Does it work like a min-height here? I.e. will the rows grow for larger text?
> > 
> > I think so. It's just ported from the existing engine manager.
> 
> Could you please test this?

I will, and will change it to min-height if it's better.

> > > Can we use an unboxed check mark then, like we do in other XUL trees?
> > 
> > The check mark we use in other XUL trees (afaik only the session restore
> > one) is absolutely awful.
> 
> You mean the particular check mark image is awful, or that there's no box
> around it?

I mean the particular check mark image. And more specifically mixing that image in the same preference pane as a native checkbox. I'm not that annoyed with the sessionrestore appearance as there's no native checkbox next to the tree there (and probably also because I got used to it after a while).

> what I saw when opening the preferences
> window with this patch applied looked quite a bit more jarring than e.g.
> about:sessionrestore...

A screenshot would have been appreciated if there's something we can improve.

> > > Mimicking a real checkbox and failing half-way looks rather cheap.
> > 
> > On Windows and Mac I think these checkboxes will look like the native ones
> > in most cases.
> 
> Besides most cases, there's a long tail of other cases on Windows, and then
> there's Linux (also a tier 1 platform). I don't think this meets our quality
> bar there.

The goal here is to restore in 35 some functionality that's missing in 34. The patch here is relatively large and I would like to have QA spend some time on it soon before we uplift it to beta. I think we all agree that the checkbox appearance isn't ideal, but if we want to polish it, I think follow-up patches riding the trains will be better. Also, I'm following your advice of unboxed checkboxes for the in-content version (and it does look better there; thanks for the idea! :-)), so I think long-term you'll have something that looks like you want.
(In reply to Florian Quèze [:florian] [:flo] from comment #24)
> (In reply to Dão Gottwald [:dao] from comment #23)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #22)
> > > I work around
> > > http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> > > menu.css#151
> > > 151 menulist > menupopup > menuitem > .menu-iconic-left,
> > > 152 menulist > menupopup > menu > .menu-iconic-left {
> > > 153   display: none;
> > > 154 }
> > 
> > Have you tried to find out what the purpose of that rule is?
> 
> It's already part of the first revision in hg. I don't think this is worth
> digging in CVS history right now.

You can alternatively file a bug on it (and reference it in a CSS comment).

> > > > Can we use an unboxed check mark then, like we do in other XUL trees?
> > > 
> > > The check mark we use in other XUL trees (afaik only the session restore
> > > one) is absolutely awful.
> > 
> > You mean the particular check mark image is awful, or that there's no box
> > around it?
> 
> I mean the particular check mark image.

Could you please file a bug on that?

> > what I saw when opening the preferences
> > window with this patch applied looked quite a bit more jarring than e.g.
> > about:sessionrestore...
> 
> A screenshot would have been appreciated if there's something we can improve.

I'll attach a screenshot, but you can't really improve it using a static checkbox image. If you try to tweak it for my OS theme (Ubuntu default theme), you'll automatically make it worse for others. So this approach is pretty much a non-starter on Linux. Can you please rip out this part of the patch?
(In reply to Florian Quèze [:florian] [:flo] from comment #24)

> > > > Does it work like a min-height here? I.e. will the rows grow for larger text?
> > > 
> > > I think so. It's just ported from the existing engine manager.
> > 
> > Could you please test this?
> 
> I will, and will change it to min-height if it's better.

min-height indeed works better here.
(In reply to Dão Gottwald [:dao] from comment #25)
> Can you please rip out this part of the patch?

What do you suggest we do instead?
(In reply to Florian Quèze [:florian] [:flo] from comment #27)
> (In reply to Dão Gottwald [:dao] from comment #25)
> > Can you please rip out this part of the patch?
> 
> What do you suggest we do instead?

Are you saying http://i.imgur.com/Pz2zfq8.png is better than http://i.imgur.com/xtsDG0P.png?
(In reply to Florian Quèze [:florian] [:flo] from comment #28)
> (In reply to Florian Quèze [:florian] [:flo] from comment #27)
> > (In reply to Dão Gottwald [:dao] from comment #25)
> > > Can you please rip out this part of the patch?
> > 
> > What do you suggest we do instead?
> 
> Are you saying http://i.imgur.com/Pz2zfq8.png is better than
> http://i.imgur.com/xtsDG0P.png?

Yes. Even if we don't improve the checkmark image (which we should), it looks more intentional than the pseudo checkboxes that quite obviously don't match the native checkbox in that window.
(In reply to Dão Gottwald [:dao] from comment #25)
> (In reply to Florian Quèze [:florian] [:flo] from comment #24)
> > (In reply to Dão Gottwald [:dao] from comment #23)
> > > (In reply to Florian Quèze [:florian] [:flo] from comment #22)
> > > > I work around
> > > > http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> > > > menu.css#151
> > > > 151 menulist > menupopup > menuitem > .menu-iconic-left,
> > > > 152 menulist > menupopup > menu > .menu-iconic-left {
> > > > 153   display: none;
> > > > 154 }
> > > 
> > > Have you tried to find out what the purpose of that rule is?
> > 
> > It's already part of the first revision in hg. I don't think this is worth
> > digging in CVS history right now.
> 
> You can alternatively file a bug on it (and reference it in a CSS comment).

Filed bug 1112310.

> > > > > Can we use an unboxed check mark then, like we do in other XUL trees?
> > > > 
> > > > The check mark we use in other XUL trees (afaik only the session restore
> > > > one) is absolutely awful.
> > > 
> > > You mean the particular check mark image is awful, or that there's no box
> > > around it?
> > 
> > I mean the particular check mark image.
> 
> Could you please file a bug on that?

Filed bug 1112306.
Attached patch Patch v3Splinter Review
Attachment #8536906 - Attachment is obsolete: true
Attachment #8537436 - Flags: review?(dao)
Attachment #8537446 - Flags: review?(felipc)
Attachment #8537446 - Flags: review?(felipc) → review+
Comment on attachment 8537436 [details] [diff] [review]
Patch v3

>--- a/browser/components/preferences/preferences.xul
>+++ b/browser/components/preferences/preferences.xul
>@@ -12,16 +12,17 @@
> <!-- XXX This should be in applications.xul, but bug 393953 means putting it
>    - there causes the Applications pane not to work the first time you open
>    - the Preferences dialog in a browsing session, so we work around the problem
>    - by putting it here instead.
>    -->
> <?xml-stylesheet href="chrome://browser/content/preferences/handlers.css"?>
> <?xml-stylesheet href="chrome://browser/skin/preferences/applications.css"?>
> <?xml-stylesheet href="chrome://browser/content/preferences/search.css"?>
>+<?xml-stylesheet href="chrome://browser/skin/preferences/search.css"?>

chrome://browser/content/preferences/search.css should be removed here.

Since we don't plan on using checkbox.png elsewhere and this is dying code, I'd prefer if you put that image in chrome://browser/skin/preferences/ instead of chrome://browser/skin/.
Attachment #8537436 - Flags: review?(dao) → review+
Blocks: 1102909
Blocks: 1102513
Blocks: 1102912
Blocks: 1106241
Blocks: 1104705
Comment on attachment 8537436 [details] [diff] [review]
Patch v3

Approval Request Comment
[Feature/regressing bug #]: preference UI for the new search UI; regression in 34
[User impact if declined]: impossible to set search keywords, remove or move search engines.
[Describe test coverage new/current, TBPL]: currently on central. I'll coordinate with QA to check if they prefer the patch to bake a few days on central before landing on aurora/beta or if they prefer the patch to land everywhere asap.
[Risks and why]: moderate (the patch is relatively large, but also self contained), but I think acceptable at this point.
[String/UUID change made/needed]: the strings have landed earlier (comment 8).
Attachment #8537436 - Flags: approval-mozilla-beta?
Attachment #8537436 - Flags: approval-mozilla-aurora?
Comment on attachment 8537446 [details] [diff] [review]
in-content version of the patch

Approval Request Comment
[Feature/regressing bug #]: new search UI
[User impact if declined]: impossible to add search keywords, move or remove search engines from the in-content preferences (ie. high impact on aurora where it's pref'ed on, and low impact on beta where it's off by default).
[Describe test coverage new/current, TBPL]: will coordinate with QA.
[Risks and why]: acceptable: self contained change in a UI that isn't enabled on release yet.
[String/UUID change made/needed]: none.

We need this on aurora; it's not required on beta, but given that it's low risk there, I think we should take it there too for the sake of consistency.
Attachment #8537446 - Flags: approval-mozilla-beta?
Attachment #8537446 - Flags: approval-mozilla-aurora?
QA Contact: petruta.rasa
Depends on: 1113096
Depends on: 1113100
Tracking in case Lukas wants it.
Attachment #8537436 - Flags: approval-mozilla-beta?
Attachment #8537436 - Flags: approval-mozilla-beta+
Attachment #8537436 - Flags: approval-mozilla-aurora?
Attachment #8537436 - Flags: approval-mozilla-aurora+
Attachment #8537446 - Flags: approval-mozilla-beta?
Attachment #8537446 - Flags: approval-mozilla-beta+
Attachment #8537446 - Flags: approval-mozilla-aurora?
Attachment #8537446 - Flags: approval-mozilla-aurora+
No longer depends on: 1113100
Depends on: 1113173
We tested the improved UI today using Nightly 37.0a1 2014-12-18 under Windows 7 64-bit, Windows 8.1 32-bit, Mac OS X 10.9.5 and Ubuntu 12.04 LTS 32-bit and spotted several issues as well as possible improvements:
- Typing the space character in the keyword field of a search engine, disables that search engine
- No notification when trying to add the same keyword for two search engines - the textarea from the second one remains on focus
- Artifact when editing a keyword, the keyword is displayed under the text-area
- Unnecessary scrollbar when entering about:preferences from url and then going to Search section (same if removing all search engines)
- The options to move search engines position and to add/edit keywords are not visible enough

Detailed testing and steps to reproduce as well as some other improvements ideas at https://etherpad.mozilla.org/Fx34b4-Search

Please check the etherpad and let us know what needs to be logged.
Depends on: 1113197
(In reply to Petruta Rasa [QA] [:petruta] from comment #39)
> We tested the improved UI today using Nightly 37.0a1 2014-12-18 under
> Windows 7 64-bit, Windows 8.1 32-bit, Mac OS X 10.9.5 and Ubuntu 12.04 LTS
> 32-bit and spotted several issues as well as possible improvements:
> - Typing the space character in the keyword field of a search engine,
> disables that search engine

This appears both with the standalone dialog and with the in-content version; I would like to fix it for 35.

> - No notification when trying to add the same keyword for two search engines
> - the textarea from the second one remains on focus
> - Artifact when editing a keyword, the keyword is displayed under the
> text-area
> - Unnecessary scrollbar when entering about:preferences from url and then
> going to Search section (same if removing all search engines)

These issues seem to be only for the in-content version; we should fix them, but that's less urgent.

> - The options to move search engines position and to add/edit keywords are
> not visible enough

I'm not sure if it's a bug, or if it's expected that only advanced users would need these features, and they would be able to discover them. You can still file a bug and needinfo UX people to get confirmation.


> Detailed testing and steps to reproduce as well as some other improvements
> ideas at https://etherpad.mozilla.org/Fx34b4-Search
> 
> Please check the etherpad and let us know what needs to be logged.

Several good bugs to file, thanks! I added a few comments in the pad.

Assuming you haven't done it yet, could you please also test the preferences in a separate dialog (set browser.preferences.inContent to false from about:config), as that's what's going to be visible by default in Firefox 35?
No longer depends on: 1113197
No longer blocks: 1113692
Release Note Request (optional, but appreciated)
[Why is this notable]: Many search features were restored
[Suggested wording]: More features in new search UI.
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Summary: Improve the search preference UI → Improve the search preference UI (add ability to edit keywords, reorder/remove engines, restore defaults)
Verified the preferences in a separate dialog in Firefox 35 RC build.
Verified as fixed under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5, versions 36 and 37.
Status: RESOLVED → VERIFIED
Blocks: 648398
Depends on: 1129587
Depends on: 1129597
Depends on: 1138112
Depends on: 1194842
Depends on: 1192475
You need to log in before you can comment on or make changes to this bug.