Closed Bug 1365638 Opened 3 years ago Closed 3 years ago

Change label text for "Close sidebar" to "Close Sidebar" on header menu

Categories

(Firefox :: Menus, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 55
Iteration:
55.6 - May 29
Tracking Status
firefox55 --- verified

People

(Reporter: abenson, Assigned: bgrins)

References

Details

(Whiteboard: [photon-structure])

Attachments

(2 files)

The option to "Close sidebar" should have a capital S for Sidebar since it is kind of like a button action.

Reference: https://mozilla.invisionapp.com/share/PJBJCIBMQ#/screens/231191185_Explainer_-_Sidebar
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: -- → P1
Attachment #8868654 - Flags: review?(gijskruitbosch+bugs) → review?(francesco.lodolo)
Delegating to flod because I can never remember if capitalization changes need an id change, and this change is simple enough that I trust his review.
Comment on attachment 8868654 [details]
Backed out changeset d4cb588ab2cc (bug 1365638)

https://reviewboard.mozilla.org/r/140240/#review143576

::: browser/locales/en-US/chrome/browser/browser.dtd:659
(Diff revision 1)
>  <!ENTITY fullZoomToggleCmd.label        "Zoom Text Only">
>  <!ENTITY fullZoomToggleCmd.accesskey    "T">
>  <!ENTITY fullZoom.label                 "Zoom">
>  <!ENTITY fullZoom.accesskey             "Z">
>  
> -<!ENTITY sidebarCloseButton.tooltip     "Close sidebar">
> +<!ENTITY sidebarCloseButton.tooltip1    "Close Sidebar">

No need to use a new ID for this kind of changes (typos, capitalization), only change the English text in this file.

Reference: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings
Attachment #8868654 - Flags: review?(francesco.lodolo) → review-
Comment on attachment 8868654 [details]
Backed out changeset d4cb588ab2cc (bug 1365638)

https://reviewboard.mozilla.org/r/140240/#review143578
Attachment #8868654 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d4cb588ab2cc
Change label text for "Close sidebar" to "Close Sidebar" on header menu;r=Gijs
https://hg.mozilla.org/mozilla-central/rev/d4cb588ab2cc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.6 - May 29
Nope, tooltips (this is what this bug is about, right?) don't use title capitalization. bgrinstead, care to back out?
Flags: needinfo?(bgrinstead)
Summary: Change label text for "Close sidebar" to "Close Sidebar" on header menu → Change tooltip for "Close sidebar" to "Close Sidebar" on header menu
Wait, you're using sidebarCloseButton.tooltip for both the label and the tooltip?
Flags: qe-verify? → qe-verify-
(In reply to Dão Gottwald [::dao] from comment #8)
> Nope, tooltips (this is what this bug is about, right?) don't use title
> capitalization. bgrinstead, care to back out?

No, this is not about the tooltip. The label read "Close sidebar". Though, I wonder if the tooltip and label text should be the same? I'll defer to Michelle H. about that.
Flags: needinfo?(mheubusch)
Summary: Change tooltip for "Close sidebar" to "Close Sidebar" on header menu → Change label text for "Close sidebar" to "Close Sidebar" on header menu
To be clear, there are two different locations where the string "Close Sidebar" now shows up.  If you open the bookmarks sidebar (ctrl+b) then the text shows up when you hover the close icon in the sidebar, and it also shows up when you open the popup by clicking on "Bookmarks" in the header.  The question is if the capitalization should differ here, with the hover/tooltip text being "Close sidebar".
(In reply to Aaron Benson from comment #10)
> (In reply to Dão Gottwald [::dao] from comment #8)
> > Nope, tooltips (this is what this bug is about, right?) don't use title
> > capitalization. bgrinstead, care to back out?
> 
> No, this is not about the tooltip. The label read "Close sidebar". Though, I
> wonder if the tooltip and label text should be the same? I'll defer to
> Michelle H. about that.

They shouldn't be the same; the label should use title capitalization and the tooltip shouldn't.

This should already ring an alarm bell: <toolbarbutton label="&...tooltip;"
Please don't reuse strings like that.
Urgh, I didn't see this. Besides capitalization, some languages use completely different structures: label is an action, tooltip is something that describes the action associated to the object, e.g. "Close sidebar" vs "Closes the sidebar".
(In reply to Francesco Lodolo [:flod] from comment #13)
> Urgh, I didn't see this. Besides capitalization, some languages use
> completely different structures: label is an action, tooltip is something
> that describes the action associated to the object, e.g. "Close sidebar" vs
> "Closes the sidebar".

OK will make the change to use separate strings
Flags: needinfo?(mheubusch)
Flags: needinfo?(bgrinstead)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Comment on attachment 8869104 [details]
Bug 1365638 - Use a different string for the sidebar close buttons label and tooltip;

https://reviewboard.mozilla.org/r/140728/#review144186

::: browser/locales/en-US/chrome/browser/browser.dtd:670
(Diff revision 1)
>  <!ENTITY fullZoomToggleCmd.accesskey    "T">
>  <!ENTITY fullZoom.label                 "Zoom">
>  <!ENTITY fullZoom.accesskey             "Z">
>  
>  <!ENTITY sidebarCloseButton.tooltip     "Close sidebar">
> +<!ENTITY sidebarCloseButton.label       "Close Sidebar">

"sidebarCloseButton" implies that the tooltip and the label are for the same element, but they're not. Can you make up a different name?

FWIW, from an external perspective this is a menu item. That it's actually a (toolbar) button is an implementation detail that shouldn't be reflected in the string name as it might confuse localizers.
Comment on attachment 8869104 [details]
Bug 1365638 - Use a different string for the sidebar close buttons label and tooltip;

https://reviewboard.mozilla.org/r/140728/#review144568
Attachment #8869104 - Flags: review?(dao+bmo) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff4f6e4c19c9
Backed out changeset d4cb588ab2cc r=Gijs
https://hg.mozilla.org/integration/autoland/rev/6c78590231f4
Use a different string for the sidebar close buttons label and tooltip;r=dao
Backout portion:
https://hg.mozilla.org/mozilla-central/rev/ff4f6e4c19c9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
I have reproduced this bug with Nightly 55.0a1 (2017-05-17) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly 55.0a1 !

Build   ID    20170524030204
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170524]
QA Whiteboard: [bugday-20170524]
Managed to reproduce the issue on an affected build (Firefox 55.0b4, Build ID: 20170622104007), on Windows 10x64. 
[Testday - 20170623]
You need to log in before you can comment on or make changes to this bug.