Closed Bug 1279337 Opened 6 years ago Closed 2 months ago

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

Categories

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

defect

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox57 --- fix-optional

People

(Reporter: tanvi, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId], [domsecurity-active])

Attachments

(2 files, 1 obsolete file)

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.
Attached patch icon.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8761932 - Flags: review?(gijskruitbosch+bugs)
Whiteboard: [userContextId] → [userContextId], [domsecurity-active]
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.
Attachment #8761932 - Flags: review?(gijskruitbosch+bugs)
Status: NEW → ASSIGNED
Attached patch icon.patchSplinter Review
Attachment #8761932 - Attachment is obsolete: true
Attachment #8797957 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8797957 - Flags: review?(gijskruitbosch+bugs) → review-
Assignee: amarchesini → nobody
Status: ASSIGNED → NEW
Assignee: nobody → jkt
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
Priority: P2 → P3
Assignee: jonathan → nobody

There is no more "containers icon".

Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.