Closed Bug 1109854 Opened 10 years ago Closed 10 years ago

Add string for empty search field

Categories

(Firefox :: Search, defect)

Product:
Firefox
Component:
Search
Version:
36 Branch
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal
Points:
2

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 37
Iteration:
37.3 - 12 Jan
Tracking Flags:
Tracking Status
firefox37 --- verified

People

(Reporter: phlsa, Assigned: abdelrahman, Mentored)

References

Details

Attachments

(1 file, 5 obsolete files)

rev 6 - empty search field
(deleted), patch
florian
: review+
Gavin
: approval-mozilla-aurora+
Details | Diff | Splinter Review
When the user just presses the down arrow on an empty search field, we show an awkward string (»Search for with:«) in the title of the one-off buttons. That section should instead say: »Search with:«
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
We have finally decided against doing this for 35; still something we want to do in 37 if possible. I'm unlikely to work in this bug this week, so unassigning myself.
Assignee: florian → nobody
Status: ASSIGNED → NEW
Iteration: 37.2 → ---
The current string is set at https://hg.mozilla.org/releases/mozilla-beta/annotate/c29a347abadc/browser/base/content/urlbarBindings.xml#l971 I guess we could use a different label when nothing has been typed in the search box, and alternate between that label and the existing set using a xul deck.
Mentor: florian
Attached patch rev 1 - empty search field (obsolete) (deleted) — — Splinter Review
Attachment #8541492 - Flags: review?(florian)
Assignee: nobody → a.ahmed1026
Status: NEW → ASSIGNED
Comment on attachment 8541492 [details] [diff] [review] rev 1 - empty search field Review of attachment 8541492 [details] [diff] [review]: ----------------------------------------------------------------- The patch works and looks good overall, but I'm not sure this approach is acceptable from a localization point of view. ::: browser/base/content/urlbarBindings.xml @@ +1066,5 @@ > self.removeAttribute("showonlysettings"); > + headerSearchFor.removeAttribute("hidden"); > + } > + else { > + headerSearchFor.setAttribute("hidden", true); I think setting headerSearchFor.hidden to true or false should work in this case. ::: browser/locales/en-US/chrome/browser/browser.dtd @@ +420,5 @@ > <!-- LOCALIZATION NOTE (searchFor.label, searchWith.label): > These two strings are used to build the header above the list of one-click > search providers: "Search for <used typed keywords> with:" --> > +<!ENTITY search.label "Search"> > +<!ENTITY searchFor.label " for "> When we change the content of a string, we should change its identifier to ensure localizers will notice the string has changed. @@ +425,1 @@ > <!ENTITY searchWith.label " with:"> flod, can you confirm if breaking this string into 3 parts to hide the "for" word is acceptable or if we need a separate "Search with:" like I initially thought?
Attachment #8541492 - Flags: feedback?(francesco.lodolo)
Comment on attachment 8541492 [details] [diff] [review] rev 1 - empty search field Review of attachment 8541492 [details] [diff] [review]: ----------------------------------------------------------------- Besides reused ID and obsolete comment, unfortunately this has way too many assumptions on the grammar structure to be safe. "Search" + " for " + X + " with:" You don't know if the equivalent of " for " is the right string to hide. Much safer using a separate string and clearly explaining its scope in the comment. Ideally we should get rid completely of concatenations and use placeholders, e.g. "Search for %1$S with: %2$S" and "Search with %S" (even if it implies using a string from .properties instead of .dtd).
Attachment #8541492 - Flags: feedback?(francesco.lodolo) → feedback-
(In reply to Francesco Lodolo [:flod] from comment #5) Thanks for the confirmation! > Ideally we should get rid completely of concatenations and use placeholders, > e.g. "Search for %1$S with: %2$S" and "Search with %S" (even if it implies > using a string from .properties instead of .dtd). The reason for using concatenation here is that the string the user typed is displayed with a different theming.
Comment on attachment 8541492 [details] [diff] [review] rev 1 - empty search field Review of attachment 8541492 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the late review here. Per comment 5, this approach won't be acceptable for localization; we need to have "Search with:" as a single localizable string. I proposed a different solution in comment 2, see https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/deck.
Attachment #8541492 - Flags: review?(florian) → review-
Attached patch rev 2 - empty search field (obsolete) (deleted) — — Splinter Review
Attachment #8541492 - Attachment is obsolete: true
Attachment #8544558 - Flags: review?(florian)
Attachment #8544558 - Attachment is obsolete: true
Attachment #8544558 - Flags: review?(florian)
Attached patch rev 3 - empty search field (obsolete) (deleted) — — Splinter Review
Attachment #8544568 - Flags: review?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #7) > we need > to have "Search with:" as a single localizable string. I proposed a > different solution in comment 2, see > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/deck. Hi Abdelrhman, Thanks for the updated patch. It seems your updated patch doesn't follow this suggestion. Is there something in my comment that you didn't understand and that I should further explain?
Attached patch rev 4 - empty search field (obsolete) (deleted) — — Splinter Review
Sorry, I haven't read your comments well. I think this patch fulfills requirements.
Attachment #8544568 - Attachment is obsolete: true
Attachment #8544568 - Flags: review?(florian)
Attachment #8544609 - Flags: review?(florian)
Is there a reason why setting the hidden attribute on 3 different nodes seems better to you than using a XUL deck like I suggested? Also, I suggested in comment 4 that you may be able to simplify lines like: headerSearchFor.setAttribute("hidden", true); to: headerSearchFor.hidden = true; Have you tried this? Did it not work?
Attached patch rev 5 - empty search field (obsolete) (deleted) — — Splinter Review
Attachment #8544609 - Attachment is obsolete: true
Attachment #8544609 - Flags: review?(florian)
Attachment #8545883 - Flags: review?(florian)
Comment on attachment 8545883 [details] [diff] [review] rev 5 - empty search field Review of attachment 8545883 [details] [diff] [review]: ----------------------------------------------------------------- The code looks good overall now, thanks :-). I'm just proposing a simplification in the XUL markup, and requesting more comments for localizers. ::: browser/base/content/urlbarBindings.xml @@ +988,5 @@ > </xul:treecols> > <xul:treechildren class="autocomplete-treebody"/> > </xul:tree> > + <xul:deck anonid="search-panel-one-offs-header" > + selectedIndex="0" You can move class="search-panel-header search-panel-current-input" to here... @@ +993,2 @@ > xbl:inherits="hidden=showonlysettings"> > + <xul:hbox anonid="search-panel-searchwith" ... and then this hbox is no longer needed. @@ +995,5 @@ > + class="search-panel-header search-panel-current-input"> > + <xul:label anonid="searchbar-oneoffheader-search" value="&search.label;"/> > + </xul:hbox> > + <xul:hbox anonid="search-panel-searchforwith" > + class="search-panel-header search-panel-current-input"> You can remove the search-panel-header class here if you set it on the deck. @@ +1077,5 @@ > self.removeAttribute("showonlysettings"); > + headerPanel.selectedIndex = 1; > + } > + else { > + headerPanel.selectedIndex = 0; nit: please fix the indent here. ::: browser/locales/en-US/chrome/browser/browser.dtd @@ +418,5 @@ > <!ENTITY searchFocusUnix.commandkey "j"> > > <!-- LOCALIZATION NOTE (searchFor.label, searchWith.label): > These two strings are used to build the header above the list of one-click > search providers: "Search for <used typed keywords> with:" --> The searcFor.label and searchWith.label strings should stay directly after this comment. @@ +419,5 @@ > > <!-- LOCALIZATION NOTE (searchFor.label, searchWith.label): > These two strings are used to build the header above the list of one-click > search providers: "Search for <used typed keywords> with:" --> > +<!ENTITY search.label "Search with:"> We should probably find a slightly more descriptive id for this string. What about "searchWithHeader.label"? Please add a localization note (see the comment of the other 2 strings for the format of these comments) explaining that this string will be shown instead of the searchFor.label and searchWith.label strings when the user hasn't typed anything, and that the wording should be as close as possible to the wording of these other 2 strings.
Attachment #8545883 - Flags: review?(florian)
Attachment #8545883 - Flags: review-
Attachment #8545883 - Flags: feedback+
Just a reminder: we need to get this patch landed before the merge on Monday.
Flags: needinfo?(florian)
Attached patch rev 6 - empty search field (deleted) — — Splinter Review
Attachment #8545883 - Attachment is obsolete: true
Attachment #8547177 - Flags: review?(florian)
Comment on attachment 8547177 [details] [diff] [review] rev 6 - empty search field Review of attachment 8547177 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks great now! :-) https://hg.mozilla.org/integration/fx-team/rev/9eb7ebdb6f6e ::: browser/locales/en-US/chrome/browser/browser.dtd @@ +419,5 @@ > > +<!-- LOCALIZATION NOTE (searchWithHeader.label): > + This string is used to build the header above the list of one-click > + instead of (searchFor.label, searchWith.label) in case of typed keyword is empty > + search providers: "Search with:" --> It seems there was a copy/paste mistake here. I rephrased this comment a little bit before the check-in.
Attachment #8547177 - Flags: review?(florian) → review+
Flags: needinfo?(florian)
Attachment #8547177 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/9eb7ebdb6f6e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Thanks Florian for your help and your efforts with me.
Iteration: --- → 37.3 - 12 Jan
QA Contact: petruta.rasa
(In reply to Abdelrhman Ahmed from comment #20) > Thanks Florian for your help and your efforts with me. You are welcome! If you would like to work on other search bugs, I just marked bug 1115002, bug 1113681, bug 1113639 and bug 1120957 as mentored.
This ended up getting merged to m-c in time for the uplift to Aurora.
status-firefox37: --- → fixed
Depends on: 1121550
Verified as fixed using Developer Edition 37.0a2 2014-01-14 under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
status-firefox37: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: