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

Status: RESOLVED FIXED

(Firefox :: Menus, defect, P1)

Description

[Aaron Benson]

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

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Backed out changeset d4cb588ab2cc (bug 1365638)

Review commit: https://reviewboard.mozilla.org/r/140240/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140240/

Comment 2

[:Gijs (he/him)]

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 3

[Francesco Lodolo [:flod]]

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

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8868654 [details]
Backed out changeset d4cb588ab2cc (bug 1365638)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140240/diff/1-2/

Comment 5

[:Gijs (he/him)]

Comment on attachment 8868654 [details]
Backed out changeset d4cb588ab2cc (bug 1365638)

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4cb588ab2cc
Change label text for "Close sidebar" to "Close Sidebar" on header menu;r=Gijs

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d4cb588ab2cc

Comment 8

[Dão Gottwald [:dao]]

Nope, tooltips (this is what this bug is about, right?) don't use title capitalization. bgrinstead, care to back out?

Comment 9

[Dão Gottwald [:dao]]

Wait, you're using sidebarCloseButton.tooltip for both the label and the tooltip?

Comment 10

[Aaron Benson]

(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.

Comment 11

[Brian Grinstead [:bgrins]]

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".

Comment 12

[Dão Gottwald [:dao]]

(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.

Comment 13

[Francesco Lodolo [:flod]]

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".

Comment 14

[Brian Grinstead [:bgrins]]

(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

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: Bug 1365638 - Use a different string for the sidebar close buttons label and tooltip;

Review commit: https://reviewboard.mozilla.org/r/140728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140728/

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8868654 [details]
Backed out changeset d4cb588ab2cc (bug 1365638)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140240/diff/2-3/

Comment 17

[Dão Gottwald [:dao]]

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 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8868654 [details]
Backed out changeset d4cb588ab2cc (bug 1365638)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140240/diff/3-4/

Comment 19

[Brian Grinstead [:bgrins]]

Comment on attachment 8869104 [details]
Bug 1365638 - Use a different string for the sidebar close buttons label and tooltip;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140728/diff/1-2/

Comment 20

[Dão Gottwald [:dao]]

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backout portion:
https://hg.mozilla.org/mozilla-central/rev/ff4f6e4c19c9

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6c78590231f4

Comment 24

[Sajedul Islam]

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]

Comment 25

[Fahima Zulfath]

Managed to reproduce the issue on an affected build (Firefox 55.0b4, Build ID: 20170622104007), on Windows 10x64. 
[Testday - 20170623]