Closed
Bug 1047433
Opened 10 years ago
Closed 10 years ago
Add "(engine) Search" label for past search results in the location bar
Categories
(Firefox :: Address Bar, defect)
Firefox
Address Bar
Tracking
()
Tracking | Status | |
---|---|---|
firefox34 | --- | verified |
People
(Reporter: Paolo, Assigned: alexbardas)
References
Details
Attachments
(2 files, 2 obsolete files)
97.30 KB,
image/png
|
Details | |
6.89 KB,
patch
|
alexbardas
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
Points: --- → 1
QA Whiteboard: [qa+]
Flags: firefox-backlog+
Comment 1•10 years ago
|
||
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.
Comment 2•10 years ago
|
||
I think I'm suggesting WONTFIX - what do you think Philipp?
Flags: needinfo?(philipp)
Comment 3•10 years ago
|
||
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
Comment 4•10 years ago
|
||
I suppose we can add just a generic "Search: " or "Search for: " prefix, with the engine name below to indicate "with what"?
Comment 5•10 years ago
|
||
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•10 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)
Comment 7•10 years ago
|
||
(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•10 years ago
|
QA Whiteboard: [qa+]
Flags: qe-verify+
Updated•10 years ago
|
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Iteration: --- → 34.3
Updated•10 years ago
|
QA Contact: alexandra.lucinet
Updated•10 years ago
|
QA Contact: alexandra.lucinet → andrei.vaida
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8476351 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 9•10 years ago
|
||
Comment 10•10 years ago
|
||
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•10 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•10 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•10 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)
Comment 14•10 years ago
|
||
(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•10 years ago
|
||
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)
Comment 16•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8476830 -
Flags: feedback+
Reporter | ||
Comment 17•10 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•10 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•10 years ago
|
||
Attachment #8476830 -
Attachment is obsolete: true
Attachment #8478648 -
Flags: review+
Assignee | ||
Comment 20•10 years ago
|
||
Try results: https://tbpl.mozilla.org/?tree=Try&rev=bef66efb7ed8
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 21•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/333a22bc721f
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/333a22bc721f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Comment 23•10 years ago
|
||
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•10 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)
Comment 25•10 years ago
|
||
(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
Comment 26•10 years ago
|
||
(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•10 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•