Show the overflow panel in customize mode

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
a year ago
11 months ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

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

Attachments

(3 attachments)

(Assignee)

Description

a year ago
Just like we currently show the main hamburger panel in customize mode, in the new design we should be showing the overflow menu instead.

In customize mode, at least initially the extra dynamically inserted/removed items (rather than the sticky/user-customized ones) won't be shown).

We should leave extra room at the bottom of the panel to insert items.

If no items are in the panel, a placeholder message with user instructions should be displayed.

The panel arrow should line up to the overflow button.
Flags: qe-verify-
Summary: Show the overflow panel in customize mode → [meta] Show the overflow panel in customize mode
Flags: qe-verify-
Keywords: meta
Summary: [meta] Show the overflow panel in customize mode → Show the overflow panel in customize mode
Version: 53 Branch → Trunk
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Priority: P2 → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8864955 - Flags: review?(mdeboer) → review?(dao+bmo)
Attachment #8864956 - Flags: review?(mdeboer) → review?(dao+bmo)
Attachment #8864955 - Flags: review?(dao+bmo) → review?(mdeboer)
Attachment #8864956 - Flags: review?(dao+bmo) → review?(mdeboer)

Comment 3

a year ago
mozreview-review
Comment on attachment 8864955 [details]
Bug 1354082 - part 1: make toggling the photon structure pref at runtime not break the panels,

https://reviewboard.mozilla.org/r/136608/#review140304

OK, we discussed this work f2f as well. Ideally, I'd like to see this patch accompanied with a try push that emulated the situation to come: 'browser.photon.structure.enabled' toggled to `true`.
Attachment #8864955 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 4

a year ago
mozreview-review
Comment on attachment 8864956 [details]
Bug 1354082 - part 2: make overflow panel show up in customize mode and make dragging work correctly,

https://reviewboard.mozilla.org/r/136610/#review140554

::: browser/components/customizableui/CustomizeMode.jsm:359
(Diff revision 1)
>  
>        this._addDragHandlers(this.visiblePalette);
>  
>        window.gNavToolbox.addEventListener("toolbarvisibilitychange", this);
>  
> +      if (gPhotonStructure) {

Huh, here and below the checks are clearly wrong... :-\
(Assignee)

Comment 5

a year ago
(In reply to Mike de Boer [:mikedeboer] from comment #3)
> Comment on attachment 8864955 [details]
> Bug 1354082 - part 1: make toggling the photon structure pref at runtime not
> break the panels,
> 
> https://reviewboard.mozilla.org/r/136608/#review140304
> 
> OK, we discussed this work f2f as well. Ideally, I'd like to see this patch
> accompanied with a try push that emulated the situation to come:
> 'browser.photon.structure.enabled' toggled to `true`.

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


I'm reasonably confident the CUI tests should be green. I haven't looked for other tests. I expect there will be some others that will need adjusting for when we really flip the pref, but this should be the bulk of the work, and I would prefer not to block this bug on fixing the remaining tests, if that's OK. :-)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

a year ago
mozreview-review
Comment on attachment 8866303 [details]
Bug 1354082 - part 0: set prefs in all the tests that need it,

https://reviewboard.mozilla.org/r/137938/#review141090

r=me with comments addressed. Thanks!

::: browser/components/customizableui/test/browser_890140_orphaned_placeholders.js:26
(Diff revision 1)
>      ok(!CustomizableUI.inDefaultState, "Should not be in default state if on DevEdition.");
>    }
>  
>    // This test relies on an exact number of widgets being in the panel.
>    // Remove the buttons to satisfy that. (bug 1229236)
> +  Cu.reportError("removing original buttons");

Please remove this line.

::: browser/components/customizableui/test/browser_938995_indefaultstate_nonremovable.js:10
(Diff revision 1)
>  "use strict";
>  
>  const kWidgetId = "test-non-removable-widget";
>  
>  // Adding non-removable items to a toolbar or the panel shouldn't change inDefaultState
> -add_task(function() {
> +add_task(function*() {

Please use `async` and `await` here.

::: browser/components/customizableui/test/browser_981305_separator_insertion.js:65
(Diff revision 1)
>        CustomizableUI.reset();
>      }
>    };
>  }
>  
> +add_task(function* () {

Please use `async` and `await` here.
Attachment #8866303 - Flags: review?(mdeboer) → review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8864956 [details]
Bug 1354082 - part 2: make overflow panel show up in customize mode and make dragging work correctly,

https://reviewboard.mozilla.org/r/136610/#review141102

Also here, r=me with comments addressed. Nice work!

::: browser/components/customizableui/CustomizeMode.jsm:1564
(Diff revision 3)
>      }
>      // Drawing in the titlebar means not showing the titlebar, hence the negation:
>      Services.prefs.setBoolPref(kDrawInTitlebarPref, !aShouldShowTitlebar);
>    },
>  
> +  _getDOMWindowUtils() {

Why not just:
```js
get _dwu() {
  if (!this.__dwu) {
    //...
  }
  return this.__dwu;
},

get _dir() {
  if (!this.__dir) {
    //...
  }
  return this.__dir;
},
```
?

::: browser/components/customizableui/CustomizeMode.jsm:2032
(Diff revision 3)
>            let draggedWrapper = this.document.getElementById("wrapper-" + aDraggedItemId);
>            let originArea = this._getCustomizableParent(draggedWrapper);
>            makeSpaceImmediately = originArea == targetArea;
>          }
> -        // Calculate width of the item when it'd be dropped in this position
> -        let width = this._getDragItemSize(aItem, draggedItem).width;
> +        let propertyToMeasure = aAreaType == "toolbar" ? "width" : "height";
> +        // Calculate width/height of the item when it'd be dropped in this position

nit: missing dot.

::: browser/components/customizableui/CustomizeMode.jsm:2034
(Diff revision 3)
>            makeSpaceImmediately = originArea == targetArea;
>          }
> -        // Calculate width of the item when it'd be dropped in this position
> -        let width = this._getDragItemSize(aItem, draggedItem).width;
> -        let direction = window.getComputedStyle(aItem).direction;
> +        let propertyToMeasure = aAreaType == "toolbar" ? "width" : "height";
> +        // Calculate width/height of the item when it'd be dropped in this position
> +        let borderWidth = this._getDragItemSize(aItem, draggedItem)[propertyToMeasure];
> +        let direction = aAreaType == "toolbar" ? "Inline" : "Block";

nit: `direction` reminds me of 'ltr' or 'rtl' values; perhaps change it to `side` or `layoutSide` or `orientation`?
Attachment #8864956 - Flags: review?(mdeboer) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

a year ago
hg error in cmd: hg rebase -s ae3b5973065d -d ed073a4c343e: abort: can't rebase public changeset ae3b5973065d
(see 'hg help phases' for details)

Comment 16

a year ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a92f82ecbf10
part 0: set prefs in all the tests that need it, r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/59cd845e4cb0
part 1: make toggling the photon structure pref at runtime not break the panels, r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/119ce372e28e
part 2: make overflow panel show up in customize mode and make dragging work correctly, r=mikedeboer

Comment 17

a year ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9621b63fc745
rm space because it's 'async function()' but 'function* ()', rs=bustage on a CLOSED TREE

Comment 18

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a92f82ecbf10
https://hg.mozilla.org/mozilla-central/rev/59cd845e4cb0
https://hg.mozilla.org/mozilla-central/rev/119ce372e28e
https://hg.mozilla.org/mozilla-central/rev/9621b63fc745
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
(Assignee)

Updated

a year ago
Depends on: 1387182

Updated

a year ago
Depends on: 1389656
(Assignee)

Updated

11 months ago
Depends on: 1395950
You need to log in before you can comment on or make changes to this bug.