Allow dragging non-removable items (url bar, back/forward buttons) within their toolbar

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Toolbars and Customization
P1
enhancement
VERIFIED FIXED
a year ago
7 months ago

People

(Reporter: Sean White, Assigned: jaws)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

57 Branch
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [reserve-photon-structure])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

a year ago
With the splitting out of the navigation and refresh buttons from the location bar container we can now move the refresh button to wherever we may want. However the back and forward buttons are immovable, although you can place something between them.  The reason given for locking these buttons was that a user might move them out of the toolbar and then not be able to navigate.  This makes some sense but without the ability to move them within the toolbar it does almost defeat the purpose of pulling them out of the location bar container in the first place.

Updated

a year ago
Blocks: 1352693
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Allow movement of Back and Forward buttons within the main toolbar → Allow dragging non-removable items (url bar, back/forward buttons) within their toolbar
Whiteboard: [photon-structure][triage]

Updated

11 months ago
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [photon-structure][triage] → [photon-structure]

Updated

11 months ago
Whiteboard: [photon-structure] → [reserve-photon-structure]

Updated

11 months ago
Priority: P3 → P4
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Assignee: nobody → jaws
Status: NEW → ASSIGNED

Updated

11 months ago
Priority: P4 → P1

Comment 2

11 months ago
Can you point out if/where/how we're still showing "blocked" icons when trying to drag these items to another toolbar?
Flags: needinfo?(jaws)
(Assignee)

Comment 3

11 months ago
mozreview-review
Comment on attachment 8899553 [details]
Bug 1387313 - Allow dragging non-removable items (url bar, back/forward buttons) within their toolbar.

https://reviewboard.mozilla.org/r/170854/#review176226

::: browser/components/customizableui/CustomizeMode.jsm:1640
(Diff revision 1)
>      // Do nothing if the widget is not allowed to be removed.
>      if (targetArea.id == kPaletteId &&
>         !CustomizableUI.isWidgetRemovable(draggedItemId)) {
>        return;

This prevents the removable=false widget from moving to the palette.

::: browser/components/customizableui/CustomizeMode.jsm:1646
(Diff revision 1)
>      // Do nothing if the widget is not allowed to move to the target area.
>      if (targetArea.id != kPaletteId &&
>          !CustomizableUI.canWidgetMoveToArea(draggedItemId, targetArea.id)) {
>        return;

This prevents the removable=false widget from moving to a different non-palette area.
(Assignee)

Updated

11 months ago
Flags: needinfo?(jaws)

Comment 4

11 months ago
mozreview-review
Comment on attachment 8899553 [details]
Bug 1387313 - Allow dragging non-removable items (url bar, back/forward buttons) within their toolbar.

https://reviewboard.mozilla.org/r/170854/#review176292

I would expect this to fail tests, but even if it doesn't, we should add a new test that checks that (a) draggin these items works and (b) dragging them to other areas doesn't work. Sorry I didn't think of this last night when I set the needinfo - my brain must have been too tired. :-(

::: browser/components/customizableui/CustomizeMode.jsm:1571
(Diff revision 1)
>        item = item.parentNode;
>      }
>  
>      let draggedItem = item.firstChild;
>      let placeForItem = CustomizableUI.getPlaceForItem(item);
> -    let isRemovable = placeForItem == "palette" ||
> +    if (item.classList.contains(kPlaceholderClass)) {

You can just omit this. The placeholders were for the old panel. Feel free to remove all of the traces of those from this file in a separate commit, or just remove this check altogether.

::: browser/components/customizableui/CustomizeMode.jsm:2215
(Diff revision 1)
>        return;
>      }
>      let doc = aEvent.target.ownerDocument;
>      doc.documentElement.setAttribute("customizing-movingItem", true);
>      let item = this._getWrapper(aEvent.target);
> -    if (item && !item.classList.contains(kPlaceholderClass) &&
> +    if (item && !item.classList.contains(kPlaceholderClass)) {

Likewise, this can just be a check for `item` not being null.
Attachment #8899553 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)

Comment 6

11 months ago
mozreview-review
Comment on attachment 8899553 [details]
Bug 1387313 - Allow dragging non-removable items (url bar, back/forward buttons) within their toolbar.

https://reviewboard.mozilla.org/r/170854/#review176712

::: browser/components/customizableui/test/browser.ini:138
(Diff revision 2)
>  skip-if = os == "mac"
>  [browser_1087303_button_preferences.js]
>  [browser_1089591_still_customizable_after_reset.js]
>  [browser_1096763_seen_widgets_post_reset.js]
>  [browser_1161838_inserted_new_default_buttons.js]
> +[browser_1387313_allow_dragging_removable_false.js]

Nit: please omit the bug number. I've stopped using them because they don't really add anything apart from being able to find the bug that added the test easily - but hg blame lets you do that anyway - and they make autocompleting filenames in this dir a nightmare.

::: browser/components/customizableui/test/browser_1387313_allow_dragging_removable_false.js:3
(Diff revision 2)
> +"use strict";
> +
> +add_task(async function() {

Nit: please add a docstring comment above this task with a 1-sentence explanation of what the test is doing.
Attachment #8899553 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 7

11 months ago
mozreview-review
Comment on attachment 8899553 [details]
Bug 1387313 - Allow dragging non-removable items (url bar, back/forward buttons) within their toolbar.

https://reviewboard.mozilla.org/r/170854/#review176716

::: browser/components/customizableui/test/browser_1387313_allow_dragging_removable_false.js:10
(Diff revision 2)
> +  let forwardButton = document.getElementById("forward-button");
> +  is(forwardButton.getAttribute("removable"), "false", "forward-button should not be removable");
> +  ok(CustomizableUI.inDefaultState, "Should start in default state.");
> +
> +  let urlbarContainer = document.getElementById("urlbar-container");
> +  let placementsAfterDrag = getAreaWidgetIds(CustomizableUI.AREA_NAVBAR);;

Heads-up: eslint chokes on the extra semicolon here.
Comment hidden (mozreview-request)

Comment 9

11 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be0f7ac59f25
Allow dragging non-removable items (url bar, back/forward buttons) within their toolbar. r=Gijs

Comment 10

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/be0f7ac59f25
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

11 months ago
Iteration: --- → 57.2 - Aug 29
I'm confirming that bug is fixed, starting in Mozilla Firefox Nightly 57.0a1 (2017-08-24), so I'm marking this bug as VERIFIED. Thanks.

I found one issue (for now), which is being tracked in bug #1393512.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
QA Contact: Virtual

Updated

11 months ago
Flags: qe-verify?

Updated

11 months ago
Duplicate of this bug: 1394826

Updated

10 months ago
Depends on: 1403382

Updated

8 months ago
Duplicate of this bug: 972266
You need to log in before you can comment on or make changes to this bug.