Add "(engine) Search" label for past search results in the location bar

VERIFIED FIXED in Firefox 34

Status

()

Firefox
Address Bar
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Paolo, Assigned: alexbardas)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 34
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox34 verified)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Bug 1034381 has no localization yet, this means the patch shows just the engine name, for example "Google", for past search results in the location bar. The interface should be clarified by showing "Google Search".
(Reporter)

Updated

4 years ago
Points: --- → 1
QA Whiteboard: [qa+]
Flags: firefox-backlog+
This is a bit tricky - I remember discussing this in an old bug about the search bar placeholder text. Not all engine names fit into a "%S Search" format (or "Search %S" format), so IIRC we decided to stick with the name only.
I think I'm suggesting WONTFIX - what do you think Philipp?
Flags: needinfo?(philipp)
I'll let Philipp answer here too, but we want something in the row to make it clear that this row will kick off the search for the indicated terms again.

Right now, without the magnifying glass (see bug 1040725) or the "search" text, I don't think it's as clear as it should be.

http://cl.ly/image/3m453x1I3D09
I suppose we can add just a generic "Search: " or "Search for: " prefix, with the engine name below to indicate "with what"?
I'd prefer not to add text to the search string itself.
How about using »Search with $provider« in the second line? Together with the magnifying glass introduced in bug 1040725, that should make things very clear.
Flags: needinfo?(philipp)
(Reporter)

Comment 6

4 years ago
Also, in bug 951624 we need a string to display in the first item of the dropdown, in case pressing Enter will initiate a new search.

Gavin, do you remember the original reason for which "%1 search" or "Search with %1" wasn't easily localizable? Can we work around that?
Flags: needinfo?(gavin.sharp)
(In reply to :Paolo Amadini from comment #6)
> Gavin, do you remember the original reason for which "%1 search" or "Search
> with %1" wasn't easily localizable? Can we work around that?

It wasn't really related to localizability, it was related to the awkwardness of that phrasing for certain search engine names. E.g. "%1 search" is awkward if the engine name is "MSN Search", which sometimes happens (though not in our set of default engines anymore).

"Search with $PROVIDER" will probably work fine in most situations.
Flags: needinfo?(gavin.sharp)

Updated

4 years ago
QA Whiteboard: [qa+]
Flags: qe-verify+

Updated

4 years ago
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Iteration: --- → 34.3
QA Contact: alexandra.lucinet
QA Contact: alexandra.lucinet → andrei.vaida
(Assignee)

Comment 8

4 years ago
Created attachment 8476351 [details] [diff] [review]
Add "Search with $provider" localized label for search results in location bar
Attachment #8476351 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 9

4 years ago
Created attachment 8476370 [details]
screenshot.png
Ugh, global/actions.properties is rather vague. Should be e.g. global/autocompleteActions.properties or even just global/autocomplete.properties. Unfortunately, there's also global/actions.dtd, which I guess you tried to follow here.
(Reporter)

Comment 11

4 years ago
(In reply to Dão Gottwald [:dao] from comment #10)
> Ugh, global/actions.properties is rather vague. Should be e.g.
> global/autocompleteActions.properties or even just
> global/autocomplete.properties. Unfortunately, there's also
> global/actions.dtd, which I guess you tried to follow here.

Yes, and I believe the two items should have the same base name. Dão, do you think it would be fine to rename actions.dtd to autocomplete.dtd, and add autocomplete.properties?
(Reporter)

Comment 12

4 years ago
Comment on attachment 8476351 [details] [diff] [review]
Add "Search with $provider" localized label for search results in location bar

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

::: toolkit/content/widgets/autocomplete.xml
@@ +1532,5 @@
>            if (!searchEngine) {
>              this._setUpDescription(this._url, url);
>            } else {
> +            let bundle = Services.strings.createBundle("chrome://global/locale/actions.properties");
> +            searchEngine = bundle.formatStringFromName("searchWithEngine", [searchEngine], 1);

The string bundle should definitely use a lazy getter. Not sure whether you can use XPCOMUtils to define one in an XBL binding, or it needs to be implemented manually with a null check.
Attachment #8476351 - Flags: review?(paolo.mozmail)
(Reporter)

Comment 13

4 years ago
Hm, actually, seeing how the switchToTab label is used:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/autocomplete.xml#1449

Maybe we should just move it to the .properties file, and adjust the calling code?

Blair, you're adding various strings in bug 951624. Do you agree it makes sense to move them all to a .properties file?
Flags: needinfo?(bmcbride)
(In reply to :Paolo Amadini from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > Ugh, global/actions.properties is rather vague. Should be e.g.
> > global/autocompleteActions.properties or even just
> > global/autocomplete.properties. Unfortunately, there's also
> > global/actions.dtd, which I guess you tried to follow here.
> 
> Yes, and I believe the two items should have the same base name. Dão, do you
> think it would be fine to rename actions.dtd to autocomplete.dtd, and add
> autocomplete.properties?

Yeah. Or move everything to the properties file, like you said.
(Assignee)

Comment 15

4 years ago
Created attachment 8476830 [details] [diff] [review]
Add "Search with $provider" localized label for search results in location bar

Ok, I've renamed the file and also added switchToTab to it & found a better way to include the strings.

Blair, is it ok to have only autocomplete.properties?
Attachment #8476351 - Attachment is obsolete: true
Attachment #8476830 - Flags: review?(paolo.mozmail)
Fine by me. 

But just so that we're aware of the original reason for this: When this was implemented, there was concern that because this is a time-sensitive hot-loop, fetching from a string bundle is repetitive work that we could avoid. Inserting those strings by using XML Entities avoids that by making it a one-time cost.
Flags: needinfo?(bmcbride)
(Reporter)

Comment 17

4 years ago
(In reply to Blair McBride [:Unfocused] from comment #16)
> But just so that we're aware of the original reason for this: When this was
> implemented, there was concern that because this is a time-sensitive
> hot-loop, fetching from a string bundle is repetitive work that we could
> avoid. Inserting those strings by using XML Entities avoids that by making
> it a one-time cost.

Thanks for mentioning this, performance should also be taken into account. In the current implementation, this XML code is inserted dynamically anyways, so I'm not sure whether there is a performance loss or win here. If formatting from the string bundle turns out to be a problem, we can always cache the string.

In any case, I'm always looking for optimizations driven by actual data, because otherwise we might be adding unneeded complexity for a marginal benefit. So, I'd not be worried about caching at the moment.
(Reporter)

Comment 18

4 years ago
Comment on attachment 8476830 [details] [diff] [review]
Add "Search with $provider" localized label for search results in location bar

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

::: toolkit/content/widgets/autocomplete.xml
@@ +1535,5 @@
>            this._setUpDescription(this._title, title);
>            if (!searchEngine) {
>              this._setUpDescription(this._url, url);
>            } else {
> +            searchEngine = this._stringBundle.formatStringFromName("searchWithEngine", [searchEngine], 1);

Please use a different variable for the resulting display value.

::: toolkit/locales/en-US/chrome/global/autocomplete.properties
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# LOCALIZATION NOTE (searchWithEngine): %S will be replaced with
> +# the search engine provider's name.
> +searchWithEngine = Search with %S

I'd add a specific localization note about the "Search with MSN Search" case, mentioning the string has been designed to make this case look OK. It may be a useful reminder for translators.
Attachment #8476830 - Flags: review?(paolo.mozmail) → review+
(Assignee)

Comment 19

4 years ago
Created attachment 8478648 [details] [diff] [review]
Add "Search with $provider" localized label for search results in location bar
Attachment #8476830 - Attachment is obsolete: true
Attachment #8478648 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/333a22bc721f
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/333a22bc721f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Testing performed on Nightly 34.0a1 (2014-08-28) using Windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.4.

Potential issues:
1. [All platforms] The "Search with" string is not displayed in the suggestions pane for searches made using eBay.
 - screenshot: http://i.imgur.com/BXQCf7X.png 
2. [All platforms] The "Search with" string is not displayed in the suggestions pane for bookmarked matches.
 - screenshot: http://i.imgur.com/4h9Pq0W.png 
3. [Linux only] In case of minimum location bar width, the end of the "Search with" string is cutoff.
 - screenshot: http://i.imgur.com/MYDdkc4.png (the Twitter entry)

Alex, what's your take on these?

Acceptance criteria used:
- The "Search with <provider>" string is currently *not* localized.
- The "Search with <provider>" string is *not* displayed in case of switch-to-tab entries from the suggestions pane.
- The "Search with <provider>" string is displayed for sub-string matches as well.
- The "Search with <provider>" string is displayed properly in case of narrow location bar & suggestions pane.
- The "Search with <provider>" string is displayed properly for suggestions pane matches that are bookmarked.
- The "Search with <provider>" string is displayed properly in case of high contrast themes.
- The "Search with <provider>" string works as expected in case of private browsing.
- The "Search with <provider>" string works as expected in case of suggestions with multiple spaces.
status-firefox34: --- → fixed
Flags: needinfo?(abardas)
(Assignee)

Comment 24

4 years ago
(In reply to Andrei Vaida, QA [:avaida] from comment #23)
> Testing performed on Nightly 34.0a1 (2014-08-28) using Windows 7 64-bit,
> Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.4.
> 
> Potential issues:
> 1. [All platforms] The "Search with" string is not displayed in the
> suggestions pane for searches made using eBay.
>  - screenshot: http://i.imgur.com/BXQCf7X.png 
> 2. [All platforms] The "Search with" string is not displayed in the
> suggestions pane for bookmarked matches.
>  - screenshot: http://i.imgur.com/4h9Pq0W.png 
> 3. [Linux only] In case of minimum location bar width, the end of the
> "Search with" string is cutoff.
>  - screenshot: http://i.imgur.com/MYDdkc4.png (the Twitter entry)

This is a follow-up from bug 1034381 and things which were not implemented previously there are also not here.

1) Andrei, can you confirm that it can also be reproduced with the build from bug 1034381 ?

2) I think bug 1047436 will handle it.

3) This was not taken into consideration, I guess an ellipsis should be shown but a ux guy should confirm this.
Flags: needinfo?(abardas)
(In reply to Alex Bardas :alexbardas from comment #24)
> This is a follow-up from bug 1034381 and things which were not implemented previously 
> there are also not here.
> 
> 1) Andrei, can you confirm that it can also be reproduced with the build
> from bug 1034381 ?
This *is reproducible* with Nightly 34.0a1 (2014-08-12).
> 3) This was not taken into consideration, I guess an ellipsis should be
> shown but a ux guy should confirm this.
Not sure who to CC on this matter.

Thanks for the prompt answer, Alex. I believe this is now verified fixed.
Status: RESOLVED → VERIFIED
status-firefox34: fixed → verified
(In reply to Alex Bardas :alexbardas from comment #24)
> 3) This was not taken into consideration, I guess an ellipsis should be
> shown but a ux guy should confirm this.

Yes, it ideally should show an ellipsis. But looking at the patch, I have no idea why it isn't. New bug?
(Assignee)

Comment 27

4 years ago
(In reply to Blair McBride [:Unfocused] from comment #26)
> (In reply to Alex Bardas :alexbardas from comment #24)
> > 3) This was not taken into consideration, I guess an ellipsis should be
> > shown but a ux guy should confirm this.
> 
> Yes, it ideally should show an ellipsis. But looking at the patch, I have no
> idea why it isn't. New bug?

It is also strange that it's a Linux only bug. I'll add now a new bug for it and cc you on it.
(Assignee)

Updated

4 years ago
Depends on: 1063553
You need to log in before you can comment on or make changes to this bug.