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

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Menus
P1
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: Aaron Benson, Assigned: bgrins)

Tracking

unspecified
Firefox 55
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 months ago
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

Updated

2 months ago
Status: NEW → ASSIGNED
Flags: qe-verify?
Priority: -- → P1
Comment hidden (mozreview-request)

Updated

2 months ago
Attachment #8868654 - Flags: review?(gijskruitbosch+bugs) → review?(francesco.lodolo)

Comment 2

2 months ago
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

2 months ago
mozreview-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 hidden (mozreview-request)

Comment 5

2 months ago
mozreview-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+

Comment 6

2 months ago
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

Comment 7

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d4cb588ab2cc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

2 months ago
Iteration: --- → 55.6 - May 29

Comment 8

2 months ago
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

Comment 9

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

Updated

2 months ago
Flags: qe-verify? → qe-verify-
(Reporter)

Comment 10

2 months ago
(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
(Assignee)

Comment 11

2 months ago
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".
(Assignee)

Comment 14

2 months ago
(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)
(Assignee)

Updated

2 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

2 months ago
Status: REOPENED → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

2 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 20

2 months ago
mozreview-review
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+

Comment 21

2 months ago
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

Comment 22

2 months ago
backoutbugherder
Backout portion:
https://hg.mozilla.org/mozilla-central/rev/ff4f6e4c19c9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago2 months ago
Resolution: --- → FIXED

Comment 23

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6c78590231f4

Comment 24

2 months ago
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]

Comment 25

a month ago
Managed to reproduce the issue on an affected build (Firefox 55.0b4, Build ID: 20170622104007), on Windows 10x64. 
[Testday - 20170623]
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.