Closed Bug 1533533 Opened 9 months ago Closed 8 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: rfeeley, Assigned: umaralam48, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [bugday-20190424] )

Attachments

(2 files)

Attached image 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
Component: Sync → Toolbars and Customization

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.

Mentor: sfoster

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

I would like to try to solve this issue.

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

Is anybody working on this?

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?

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

Flags: needinfo?(sfoster)

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

Flags: needinfo?(sfoster)

See also : Bug 1529874

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

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

Assignee: nobody → umaralam48
Status: NEW → ASSIGNED
Flags: needinfo?(francesco.lodolo)

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

Flags: needinfo?(francesco.lodolo)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/33363b7f728c
Add a label to hide synced tabs sidebar when it is shown r=sfoster,jaws,flod
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0962f523e2a
add spacing after comma in panelUI.inc.xul on a CLOSED TREE
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68

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

Flags: needinfo?(sfoster)
Regressions: 1547614
Duplicate of this bug: 1529874

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

Whiteboard: [bugday-20190424]
Flags: needinfo?(sfoster)
You need to log in before you can comment on or make changes to this bug.