1279337 - Move the containers icon from the Customize section to the Hamburger Menu section

Status: RESOLVED WORKSFORME

(Core :: DOM: Security, defect, P3)

Description

[Tanvi Vyas[:tanvi]]

When the usercontextid flag is enabled, the containers icon is currently in the Customize section.  So the user needs to go to the hamburger menu, click customize, and drag the container icon to the hamburger menu or the toolbar to be able to use it.

Instead, when the usercontextid flag is enabled, the containers icon should be in the hamburger menu by default.

The user could further customize it to move it into the toolbar, or out of the hamburger menu.  But its default location should be in the hamburger menu.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: icon.patch

Comment 2

[:Gijs (he/him)]

Comment on attachment 8761932 [details] [diff] [review]
icon.patch

Review of attachment 8761932 [details] [diff] [review]:
-----------------------------------------------------------------

This is going to break customizableUI tests when the pref gets flipped. You'll also need to adjust BrowserUITelemetry and its test.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: icon.patch

Comment 4

[:Gijs (he/him)]

Comment on attachment 8797957 [details] [diff] [review]
icon.patch

Review of attachment 8797957 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1098,5 @@
>      }
>    }, {
>      id: "containers-panelmenu",
>      type: "view",
> +    defaultArea: CustomizableUI.AREA_PANEL,

Shouldn't this also be conditional on the pref?

::: browser/components/customizableui/test/browser_876944_customize_mode_create_destroy.js
@@ +10,5 @@
> +add_task(function* setup() {
> +  yield new Promise((resolve) => {
> +    SpecialPowers.pushPrefEnv({"set": [
> +      ["privacy.userContext.enabled", true],
> +    ]}, resolve);

You can just yield SpecialPowers.pushPrefEnv.

@@ +37,5 @@
>    let panel = document.getElementById(CustomizableUI.AREA_PANEL);
>    // The value of expectedPlaceholders depends on the default palette layout.
>    // Bug 1229236 is for these tests to be smarter so the test doesn't need to
>    // change when the default placements change.
> +  let expectedPlaceholders = isInDevEdition() ? 1 : 3;

You can't hardcode this as you're doing now because when the pref goes away again (aurora) it'll go orange. This is why there's an "indevedition" getter. You'd need a similar thing. Perhaps we can make a generic getter that checks the number of items at the start of the test and does maths based on that. That would avoid having to update these tests all the time.

::: browser/components/customizableui/test/browser_880382_drag_wide_widgets_in_panel.js
@@ +23,5 @@
>                               "preferences-button",
>                               "add-ons-button",
>                               "developer-button",
>                               "sync-button",
> +                             "containers-panelmenu",

Again, this needs to check the pref.

::: browser/components/customizableui/test/browser_967000_button_sync.js
@@ +337,5 @@
>  });
> +
> +add_task(function* () {
> +  yield resetCustomization();
> +  ok(CustomizableUI.inDefaultState, "The panel UI is in default state again.");

Why is this necessary?

::: browser/modules/BrowserUITelemetry.jsm
@@ +44,5 @@
>        "preferences-button",
>        "add-ons-button",
>        "sync-button",
>        "developer-button",
> +      "containers-panelmenu",

This should check the pref as well.

Comment 5

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1279337 - Setting default placement of container drawers into AREA_PANEL

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

Comment 6

[Jonathan Kingston [:jkt] he/him]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5e5930cc048

Comment 7

[Firefox Bug Husbandry Bot]

Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 9

[:Gijs (he/him)]

There is no more "containers icon".