1533533 - Add a label to hide synced tabs sidebar when it is shown

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P3)

Description

[Ryan Feeley [:rfeeley]]

Attachment: synced-tabs.png

STEPS TO REPRODUCE

1. Open Synced Tabs sidebar
2. Open Synced Tabs menu panel

EXPECTED RESULTS

• Sidebar toggle is labelled Hide Synced Tabs Sidebar

ACTUAL RESULTS

• Sidebar toggle is labelled Hide Synced Tabs Sidebar

Comment 1

[Sam Foster [:sfoster] (he/him)]

A bit of clarification on the STR:

Add the Synced Tabs button to your toolbar - by using the Customize option from the main menu, drag the 'Synced Tabs' icon to your toolbar to the right of the address/url bar.

Click the toggle sidebar icon in your toolbar to show the sidebar, and select the Synced Tabs panel from the picker at the top of the sidebar.

Sign into Sync - both the sidebar and the toolbar button will prompt you to sign in to Sync

Now, you are ready for the steps in comment #0. The menu that appears when you click the toolbar button has as its first item "View Synced Tabs Sidebar". This menu item actually toggles the visibility of the sidebar, so when the sidebar is already open, it should be labelled "Hide Synced Tabs Sidebar".

There is a currentID property on the SidebarUI which can be used to know if the Synced Tabs Sidebar is already open: https://searchfox.org/mozilla-central/source/browser/base/content/browser-sidebar.js#334

The current string used for this label is appMenuRemoteTabs.sidebar.label, which is used in the markup for this menuitem: https://searchfox.org/mozilla-central/source/browser/components/customizableui/content/panelUI.inc.xul#466

You'll need to define the new string for the "Hide.." variation and figure out how to switch it so the correct string is used for each state.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

The new string should be added in https://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#469 with the name of appMenuRemoteTabs.sidebar.hideLabel and the value of "Hide Synced Tabs Sidebar".

It looks like this code already handles changing the label based on if the sidebar is showing or not,
https://searchfox.org/mozilla-central/rev/aae527894a97ee3bbe0c2cfce9c67c59e8b8fcb9/browser/base/content/browser.js#5827-5837

Therefore, the attribute in the XUL file should be named checked-label and the current attribute should be set as unchecked-label (not sure if we should keep the label attribute or not, this would need to be tested).

Comment 3

[Berkay Barlas]

I would like to try to solve this issue.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Berkay Barlas from comment #3)

I would like to try to solve this issue.

You may work on this, but we only assign bugs to people once a patch is attached. Please ask any questions you may have here while working on it.

Comment 5

[afshan.shokath]

Is anybody working on this?

Comment 6

[Berkay Barlas]

Because this week is my midterm week(it will end tomorrow), I had trouble to find time to look at problem.

(In reply to (behind on reviews and needinfos) Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) from comment #2)

Therefore, the attribute in the XUL file should be named checked-label and the current attribute should be set as unchecked-label (not sure if we should keep the label attribute or not, this would need to be tested).

I couldn't understand how to set the attribute in the XUL as 'checked-label'. Does not they need to be named like this
label="&appMenuRemoteTabs.sidebar.label;" to show the text?

Comment 7

[Mohd Umar Alam [:umaralam48]]

The same behaviour is shown by the history sidebar toggle button. Should I file a separate bug for it ?

Comment 8

[Sam Foster [:sfoster] (he/him)]

(In reply to Mohd Umar Alam [:umaralam48] from comment #7)

The same behaviour is shown by the history sidebar toggle button. Should I file a separate bug for it ?

Yeah, go ahead. If it turns out both can be fixed with the same patch, we can always close it as a duplicate.

Comment 9

[Mohd Umar Alam [:umaralam48]]

See also : Bug 1529874

Comment 10

[Mohd Umar Alam [:umaralam48]]

Attachment: Bug 1533533-Add a label to hide synced tabs sidebar when it is shown r=sfoster, jaws, flod

Comment 11

[Sam Foster [:sfoster] (he/him)]

:flod, I think Attachment #9059442 [details] just needs your ok to land at this point.

Comment 12

[Francesco Lodolo [:flod]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #11)

:flod, I think Attachment #9059442 [details] just needs your ok to land at this point.

Not really. The review is mandatory only for FTL files, for DTDs and properties front-end devs have more than enough experience.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33363b7f728c
Add a label to hide synced tabs sidebar when it is shown r=sfoster,jaws,flod

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0962f523e2a
add spacing after comma in panelUI.inc.xul on a CLOSED TREE

Comment 15

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/33363b7f728c
https://hg.mozilla.org/mozilla-central/rev/c0962f523e2a

Comment 16

[Mohd Umar Alam [:umaralam48]]

This patch fixes the bug 1529874 too so maybe we can close it?

Comment 18

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 67.0a1 (2019-03-07) on Windows 7, 64 Bit. The fix of this bug is verified with latest Nightly!

Build ID 20190429095544
User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0