Closed Bug 1390313 Opened 7 years ago Closed 7 years ago

Item added to the overflow menu should scale down and fade out

Categories

(Firefox :: Theme, defect, P1)

Product:
Firefox
Component:
Theme
Version:
57 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: epang, Assigned: ewright)

References

Details

(Whiteboard: [reserve-photon-animation] )

Attachments

(2 files)

Move to Overflow Menu - Stand Alone.mp4
1.39 MB, video/mp4
Details
Bug 1390313 - Item added to the overflow menu should scale down and fade out. ui-r=epang
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
When an item is added to the overflow menu it should scale down while fading out.  

See attached motion.

Specs:

Duration: 1000ms
Following right click fade out and scale begins

Opacity: 100% > 0%
Scale: 100% > 50%
Whiteboard: [photon-animation] [triage] [ux] → [reserve-photon-animation]
Flags: qe-verify?
Priority: -- → P4
Flags: qe-verify? → qe-verify+
QA Contact: stefan.georgiev
Assignee: nobody → ewright
Status: NEW → ASSIGNED
Priority: P4 → P1
Comment on attachment 8906099 [details]
Bug 1390313 - Item added to the overflow menu should scale down and fade out. ui-r=epang

https://reviewboard.mozilla.org/r/177818/#review183096

I'm not sure how the delay in actually moving an item will affect location of items. For instance if I right click an item and choose "Pin to Overflow" then immediately open the overflow panel, should I see the item?

I'll be away for the next two days so I want to forward this to Gijs to get his opinion. I think this might work but I'm just concerned about the above workflow.

::: browser/components/customizableui/CustomizableUI.jsm:1970
(Diff revision 1)
> +    }
> +  },
> +
> +  animateOverflowPanel(aArea) {
> +    if (aArea == CustomizableUI.AREA_FIXED_OVERFLOW_PANEL
> +        && Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {

nit, && on the line above and at the end.

::: browser/themes/shared/customizableui/panelUI.inc.css:476
(Diff revision 1)
>    height: calc(51px + 2.2em);
>  }
>  
> +.toolbarbutton-1.transition-out {
> +  opacity: 0;
> +  transform:scale(0.5);

nit, space after colon

::: browser/themes/shared/customizableui/panelUI.inc.css:477
(Diff revision 1)
>  }
>  
> +.toolbarbutton-1.transition-out {
> +  opacity: 0;
> +  transform:scale(0.5);
> +  -moz-transform:scale(0.5);

No need for -moz- prefixed `transform`.
Attachment #8906099 - Flags: review?(jaws)
Attachment #8906099 - Flags: review?(gijskruitbosch+bugs)
Apologies for the delay. I will try to get to this tomorrow.
Comment on attachment 8906099 [details]
Bug 1390313 - Item added to the overflow menu should scale down and fade out. ui-r=epang

https://reviewboard.mozilla.org/r/177818/#review183728

OK, so, in the clear light of morning... Unfortunately, I don't think this is the right approach.

The animation should only show when the user manipulates the button using the context menu. It doesn't really make sense to show the same animation when we move items internally, or when the user drags/drops items in customize mode (because the item is no longer in its old position in that case), or when using "restore defaults" in customize mode, or when building up toolbars on first run. As far as I can tell, the current code affects all those cases, and if we wanted to keep the code like this, we'd have to find ways of making sure we exclude those cases, which is tricky because addWidgetToArea and removeWidgetFromArea are pretty generic APIs. In fact, the latter gets called from the former, which causes additional complications here.

Furthermore, this code would run in every window. So if you had 5 windows, the animation would run in all 5. I don't think it'll perform well in that case, and it's likely not useful in all but 1 of them (the foreground window where the user executes the move).

I think the correct solution is to add the animations to the helper functions that power the context menus, here: https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/browser/components/customizableui/CustomizeMode.jsm#625

This has the additional benefit of not introducing asynchronicity and/or multiple code paths into the core CUI code, of only affecting the context menu, and of separating the animation cleanly from the "internals" of how CustomizableUI works. You can just make the 2 (I think? `removeFromArea` and `addToPanel`) affected methods async, wait for the transition/animation to finish if the pref is set and the button is in the right place, and then execute the "real" move when finished.
Attachment #8906099 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8906099 [details]
Bug 1390313 - Item added to the overflow menu should scale down and fade out. ui-r=epang

https://reviewboard.mozilla.org/r/177818/#review183728

Unfortunately `removeFromArea` never triggers, that's not how the widget gets removed. Previously, it was removed in two places - generally one in `onWidgetRemoved` - (`container.removeChild(widgetNode);`) and once in `onWidgetAdded` - (`this.insertNode(aWidgetId, aArea, aPosition, true);`). This was tyhe only way I could find to get control of the widget before either of those remove it.
The overflow menu animation that was previously in `addToPanel` I can put back. It seemed to make more sense in customizableUI rather than customizeMode. 
Some of the cases you mentioned have already been addressed, it does not animate on drags/drop in customize mode (from the palette), nor when building up toolbars on first run nor on "restore defaults". The animations running in all of the windows was done intentionally, I can remove that loop and have it run only in the active window. However, you made me realize that when customize mode is open and the widget is in the toolbar if I drag and drop, or click the menu there are problems.
I'll see if I can find a better place to put this. I'd love further ideas or more explanation if I missed something.
Can this just be added to CustomizeMode.addToPanel() ? It's all part of the "add to panel" animation anyways.
I think so, if I call it all before the `CustomizableUI.addWidgetToArea(aNode.id, panel);` line. I could just animate until it is opacity 0, then let the other code run to take care of the removal as it has.
(In reply to Erica Wright [:ewright] from comment #7)
> I think so, if I call it all before the
> `CustomizableUI.addWidgetToArea(aNode.id, panel);` line. I could just
> animate until it is opacity 0, then let the other code run to take care of
> the removal as it has.

FWIW, 'removeFromArea' is what should be run if you use the context menu (in or outside of customize mode) and select 'remove from toolbar'. I tried to suggest adding the animation to both because I think the removal (shrink+fade) animation makes sense even in the 'remove from toolbar' case.
(In reply to :Gijs from comment #8)
> (In reply to Erica Wright [:ewright] from comment #7)
> > I think so, if I call it all before the
> > `CustomizableUI.addWidgetToArea(aNode.id, panel);` line. I could just
> > animate until it is opacity 0, then let the other code run to take care of
> > the removal as it has.
> 
> FWIW, 'removeFromArea' is what should be run if you use the context menu (in
> or outside of customize mode) and select 'remove from toolbar'. I tried to
> suggest adding the animation to both because I think the removal
> (shrink+fade) animation makes sense even in the 'remove from toolbar' case.

Oh! I thought that selecting "pin to overflow" was supposed to use `removeFromArea`. I understand what you were getting at, thank you.
The current patch has an issue with adding "flexible space". But related issues already exist with that.
(In reply to Erica Wright [:ewright] from comment #11)
> The current patch has an issue with adding "flexible space". But related
> issues already exist with that.

Sorry, can you elaborate on what you mean? Bug 1396953? Or something else?
Flags: needinfo?(ewright)
Comment on attachment 8906099 [details]
Bug 1390313 - Item added to the overflow menu should scale down and fade out. ui-r=epang

https://reviewboard.mozilla.org/r/177818/#review184712

Yep, this is roughly what I had in mind. I think we can de-duplicate the code a bit more, and it would be nice to ensure that the code with the animation is actually tested, at least in one of the CUI tests.

::: browser/components/customizableui/CustomizeMode.jsm:620
(Diff revision 2)
>      aNode = this._getCustomizableChildForNode(aNode);
>      if (aNode.localName == "toolbarpaletteitem" && aNode.firstChild) {
>        aNode = aNode.firstChild;
>      }
> +
> +    if (Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {

Can you make this a lazy preference getter (XPCOMUtils.defineLazyPreferenceGetter() ) at the top of the file?

::: browser/components/customizableui/CustomizeMode.jsm:621
(Diff revision 2)
> +      aNode.classList.add("transition-out");
> +      aNode.addEventListener("transitionend", function continueAddToToolbar(e) {
> +        if (e.propertyName == "transform" && e.target.id == aNode.id) {
> +          aNode.removeEventListener("transitionend", continueAddToToolbar);

This is duplicated in 3 places now. Can you add a utility method like so:

```js
_promiseTransitionOut(aNode) {
  if (!gCosmeticAnimationsEnabled) {
    return null;
  }
  return new Promise(resolve => {
    // add class, add event listeners for transition,
    // wait for the transition to finish or cancel and then resolve the promise.
  });
},
```

then make these (addToToolbar, addToPanel and removeArea) methods async, and do something like:

```js
let transitionPromise = this._promiseTransitionOut(aNode);
if (transitionPromise) {
  await transitionPromise;
}
// keep old code to add/move/remove button here.
```

? That would reduce the animation duplication, and it would also mean we keep only 1 copy of the actual add/move/remove code in each of those functions. :-)

::: browser/components/customizableui/CustomizeMode.jsm:645
(Diff revision 2)
>    addToPanel(aNode) {
>      aNode = this._getCustomizableChildForNode(aNode);
>      if (aNode.localName == "toolbarpaletteitem" && aNode.firstChild) {
>        aNode = aNode.firstChild;
>      }
> +    if (Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {

I'm assuming the tests currently turn this pref off? If so, can we turn it on in a new test task in the test file that checks the context menus (in browser/components/customizableui/tests somewhere) so that we make sure we do actually test that the variation of this code with the animations does in fact work?

::: browser/components/customizableui/CustomizeMode.jsm:646
(Diff revision 2)
>      aNode = this._getCustomizableChildForNode(aNode);
>      if (aNode.localName == "toolbarpaletteitem" && aNode.firstChild) {
>        aNode = aNode.firstChild;
>      }
> +    if (Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {
> +      aNode.classList.add("transition-out");

Hm. If there is only 1 item in the panel, the panel closes when removing an item, also because its anchor (the overflow button) disappears. Do we need to take that into account here? I don't remember off-hand where the logic lives that hides the panel in that case. Maybe it's fine, especially if we're postponing the actual removal, but please check that it behaves in a sane manner. :-)
Attachment #8906099 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #12)
> (In reply to Erica Wright [:ewright] from comment #11)
> > The current patch has an issue with adding "flexible space". But related
> > issues already exist with that.
> 
> Sorry, can you elaborate on what you mean? Bug 1396953? Or something else?

if you right click on "flexible space" and click "add to overflow menu" the overflow menu will animate, even though the widget doesn't get added. With my patch, because the widget will not animate out, it will also not animate the overflow, and will not show up in it. An issue that does exist though is that when clicking "add to toolbar" nothing happens - again because the widget is not able to animate out.
Flags: needinfo?(ewright)
(In reply to :Gijs from comment #13)
> Comment on attachment 8906099 [details]

> Hm. If there is only 1 item in the panel, the panel closes when removing an
> item, also because its anchor (the overflow button) disappears. Do we need
> to take that into account here? I don't remember off-hand where the logic
> lives that hides the panel in that case. Maybe it's fine, especially if
> we're postponing the actual removal, but please check that it behaves in a
> sane manner. :-)

I think it's better if the items in the overflow menu don't animate out. They are doing so because of the remove from toolbar rule.
:gijs, The tests were the toughest part currently. I'm unsure if promises are the best way to go, and if so, how to test that they were fired - other than what I did. I wrote it a different way using `waitForExplicitFinish` but this way seemed more straightforward.
(In reply to Erica Wright [:ewright] from comment #14)
> (In reply to :Gijs from comment #12)
> > (In reply to Erica Wright [:ewright] from comment #11)
> > > The current patch has an issue with adding "flexible space". But related
> > > issues already exist with that.
> > 
> > Sorry, can you elaborate on what you mean? Bug 1396953? Or something else?
> 
> if you right click on "flexible space" and click "add to overflow menu" the
> overflow menu will animate, even though the widget doesn't get added. With
> my patch, because the widget will not animate out, it will also not animate
> the overflow, and will not show up in it. An issue that does exist though is
> that when clicking "add to toolbar" nothing happens - again because the
> widget is not able to animate out.

Ugh, we shouldn't enable the "add to overflow" context menu for flexible spaces. I filed bug 1401152. We already do the right thing in the toolbar.
Comment on attachment 8906099 [details]
Bug 1390313 - Item added to the overflow menu should scale down and fade out. ui-r=epang

https://reviewboard.mozilla.org/r/177818/#review186444

This is great. My only comments are on the test.

I noticed I bitrotted you (bug 1399486), causing conflicts in CustomizeMode.jsm - sorry! When I tested, I rebased. This is my rebased patch, in case it's helpful: https://pastebin.mozilla.org/9032712 (I haven't addressed the other review feedback here, so that still needs to happen :-) ).

::: browser/components/customizableui/test/browser_widget_animation.js:1
(Diff revision 3)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

We normally license tests as public domain, if that's OK with you. :-)

::: browser/components/customizableui/test/browser_widget_animation.js:34
(Diff revision 3)
> +      }
> +    });
> +  });
> +}
> +
> +add_task(async function() {

Can you add a comment above this function to indicate what it's testing?

::: browser/components/customizableui/test/browser_widget_animation.js:45
(Diff revision 3)
> +  await Promise.all([promiseTransitionEnd(), promiseAnimationEnd()]);
> +  ok(true, "The animation promises resolved successfully");

Here, can you move the home button back to ensure the test leaves the browser in a clean state for the next test to run? It's probably easiest to do this by using `CustomizableUI.reset()`.

::: browser/components/customizableui/test/browser_widget_animation.js:45
(Diff revision 3)
> +  let moveToPanel = contextMenu.querySelector(".customize-context-moveToPanel");
> +  if (moveToPanel) {
> +    moveToPanel.click();
> +  }
> +
> +  await Promise.all([promiseTransitionEnd(), promiseAnimationEnd()]);

Clever! And sorry this took so much time. :-(

I'm a bit surprised this works without enabling the animations explicitly (ie by setting the pref in the test). I take it cosmetic animations are turned on in mochitests by default? Maybe we should reconsider that (in a different bug, not here).
Attachment #8906099 - Flags: review?(gijskruitbosch+bugs) → review+
Ah, and when running the customizableui tests locally, I get these failures:

The following tests failed:
3561 INFO TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_972267_customizationchange_events.js | Should be called for both windows. - Got 0, expected 2
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:1011
chrome://mochitests/content/browser/browser/components/customizableui/test/browser_972267_customizationchange_events.js:null:25
3562 INFO TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_972267_customizationchange_events.js | Should be called for both windows. - Got 0, expected 2
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:1011
chrome://mochitests/content/browser/browser/components/customizableui/test/browser_972267_customizationchange_events.js:null:29
3563 INFO TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_984455_bookmarks_items_reparenting.js | Test timed out - 
3564 INFO TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_985815_propagate_setToolbarVisibility.js | Should start in default state. - 
Stack trace:
    chrome://mochitests/content/browser/browser/components/customizableui/test/browser_985815_propagate_setToolbarVisibility.js:null:8
    Tester_execTest@chrome://mochikit/content/browser-test.js:798:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:697:9
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
3565 INFO TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_985815_propagate_setToolbarVisibility.js | Reset button should be disabled - 
Stack trace:
chrome://mochitests/content/browser/browser/components/customizableui/test/browser_985815_propagate_setToolbarVisibility.js:null:12
3566 INFO TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_customization_context_menus.js | Should be in panel - Got nav-bar, expected widget-overflow-fixed-list
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:1011
    chrome://mochitests/content/browser/browser/components/customizableui/test/browser_customization_context_menus.js:searchbar_context_move_to_panel_and_back:156
    Tester_execTest@chrome://mochikit/content/browser-test.js:798:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:697:9
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
3567 INFO TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_customization_context_menus.js | Should be in palette - Got [object Object], expected null
Stack trace:
chrome://mochikit/content/browser-test.js:test_is:1011
chrome://mochitests/content/browser/browser/components/customizableui/test/browser_customization_context_menus.js:searchbar_context_move_to_panel_and_back:172
3568 INFO TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_overflow_use_subviews.js | Uncaught exception - undefined - timed out after 50 tries.
Buffered messages finished
SUITE-END | took 128s



I expect we'll need to update those tests to turn the animations pref off so they continue to work, or add the same 'wait for the animation to complete' promise checks. Probably a good idea to do a trypush after that to check there isn't anything else that depends on this stuff (I think at least https://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/test/browser/browser_downloads_autohide.js might still do, too).
(In reply to :Gijs from comment #20)
> Ah, and when running the customizableui tests locally, I get these failures:
> ...
> I expect we'll need to update those tests to turn the animations pref off so
> they continue to work, or add the same 'wait for the animation to complete'
> promise checks. Probably a good idea to do a trypush after that to check
> there isn't anything else that depends on this stuff (I think at least
> https://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/
> test/browser/browser_downloads_autohide.js might still do, too).

Yeah, it seems they are turned on. Within this set I noticed some use `startCustomizing()` which turns them off for customization mode. But others haven't explicitly set them. 

As for those failures, I returned the home button and I don't have them for me locally.

try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52ca8d85a85f8951dd4c85d065852646b95fea51
Comment on attachment 8906099 [details]
Bug 1390313 - Item added to the overflow menu should scale down and fade out. ui-r=epang

https://reviewboard.mozilla.org/r/177818/#review186520

::: browser/components/customizableui/CustomizeMode.jsm:633
(Diff revision 4)
>    },
>  
> -  addToToolbar(aNode) {
> +  _promiseTransitionOut(aNode) {
> +    if (!gCosmeticAnimationsEnabled ||
> +        aNode.getAttribute("cui-anchorid") == "nav-bar-overflow-button" ||
> +        aNode.tagName == "toolbarspring") {

The CSS for this only applies for `toolbaritem` and `toolbarbutton`. Can you change this conditional to:

(aNode.tagName != "toolbaritem" && aNode.tagName != "toolbarbutton")

::: browser/components/customizableui/test/browser_widget_animation.js:34
(Diff revision 4)
> +    });
> +  });
> +}
> +
> +// Right-click on the home widget, use the context menu to move it to the overflow menu.
> +// The home widget should animate out, and the overflow menu should animate upon adding. 

nit, trailing whitespace which might cause an eslint failure.

::: browser/themes/shared/customizableui/panelUI.inc.css:476
(Diff revision 4)
> +toolbaritem.transition-out,
> +toolbarbutton.transition-out {

With the above request to tweak the early-return conditions inside of _promiseTransitionOut, can you remove the `toolbaritem` and `toolbarbutton` from the selector here? With that change, we won't be putting this className on any nodes that are not of these two types.
:gijs, do you have any further advice for this one?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Erica Wright [:ewright] from comment #25)
> :gijs, do you have any further advice for this one?

The patch looks fine, but the trypush shows a bunch of orange:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=52ca8d85a85f8951dd4c85d065852646b95fea51&selectedJob=131983322

I think this matches my expectations in comment 20. I expect the easiest way to deal with this is to:

1) turn off the animations pref by default in the manifest file for the customizableui directory ( https://groups.google.com/forum/?fromgroups=&hl=en#!searchin/firefox-dev/PSA%7Csort:date/firefox-dev/gFm6l3oYllo/SaZzpB4SAQAJ )
2) turn it on by using pushPrefEnv() in the new test you're adding.


Alternatively, you could try to fix the broken tests, for instance by using BrowserTestUtils.waitForEvent() to wait for the `customizationchange` event in some of these tests, accepting that it will fire asynchronously instead of synchronously now.

Still another alternative would be to use pushPrefEnv() to turn off the animations *only* in the tests where this is causing issues.

I think the latter two are probably more correct ways of addressing the orange (in that it'll ensure that what we test on infra is as close to what actually ships as possible), but I also wouldn't object to using my first suggestion and pushing the rest to a followup, if you would prefer to do that.

Does that help? Did you have specific questions? :-)
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ewright)
I think that covers it, thank you. I went with your first suggestion. 

new try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4de8530aa2e10bb519d0058705960dd2f7e9373

note: I used `Services.prefs.setBoolPref` rather than pushPrefEnv.
Flags: needinfo?(ewright)
As we talked about, a bunch of the failing tests have been edited now to wait for the animations to finish. As well, some test actually helped to point some things out with the wide widgets! so some of the implementation has changed too. 
And the try is looking better:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1388fe4efce20c1220e672868b5bde4f172dde47&selectedJob=134113696
I don't think any of the failing tests are related to this at this point. Do you have any further advice?
Flags: needinfo?(gijskruitbosch+bugs)
The patch needs to be rebased but it is easy to rebase this (I just did locally).

browser_985815_propagate_setToolbarVisibility.js looks like it is failing pretty consistently. The other test failures look like they can be ignored. I can't reproduce the failure with browser_985815_propagate_setToolbarVisibility.js locally though.
Comment on attachment 8906099 [details]
Bug 1390313 - Item added to the overflow menu should scale down and fade out. ui-r=epang

https://reviewboard.mozilla.org/r/177818/#review191606

This patch looks fine to me, though I concur with Jared that it looks like maybe browser/components/customizableui/test/browser_985815_propagate_setToolbarVisibility.js needs some love. Happy to help investigate that tomorrow if you need a hand!

::: browser/components/customizableui/CustomizeMode.jsm:634
(Diff revision 7)
>  
> -  addToToolbar(aNode) {
> +  _promiseWidgetAnimationOut(aNode) {
> +    if (!gCosmeticAnimationsEnabled ||
> +        aNode.getAttribute("cui-anchorid") == "nav-bar-overflow-button" ||
> +        (aNode.tagName != "toolbaritem" && aNode.tagName != "toolbarbutton") ||
> +        (aNode.id == "downloads-button" && aNode.hidden)) {

This is exciting. When do we hit this case? That is, how can the user (or a test, maybe?) remove the downloads button while it's hidden? Does this only happen from a test?

::: browser/components/customizableui/test/browser_984455_bookmarks_items_reparenting.js:218
(Diff revision 7)
>    gCustomizeMode.addToToolbar(bookmarksToolbarItems);
> +  await promiseWidgetAnimationOut(bookmarksToolbarItems);

Where we're directly using the add/remove helpers, can we simply await those methods themselves? (ie `await gCustomizeMode.addToToolbar(bookmarksToolbarItems);` in this case ?
Clearing ni for comment #33, but please let me know if you need help with the remaining test.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8906099 [details]
Bug 1390313 - Item added to the overflow menu should scale down and fade out. ui-r=epang

https://reviewboard.mozilla.org/r/177818/#review191606

> This is exciting. When do we hit this case? That is, how can the user (or a test, maybe?) remove the downloads button while it's hidden? Does this only happen from a test?

Yep, there's a test that decided to use the "downloads-button" at the node it moves around. But, the download's button is in auto-hide mode. Their test doesn't care, but mine does.
The failing test turned out to be because I was not waiting for `browser/components/customizableui/test/browser_984455_bookmarks_items_reparenting.js` to finish animations. Though it was not failing itself, it was setting up an environment for the next test to fail.

try:https://treeherder.mozilla.org/#/jobs?repo=try&revision=486658d0ccb22f3e7d3cbdf62459d6e9570285d4
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1053bf2fbaed
Item added to the overflow menu should scale down and fade out. ui-r=epang r=Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1053bf2fbaed
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Depends on: 1406191
Depends on: 1407435
Verified fixed on latest Nightly 58.0a1 (2017-10-17) across platforms: Windows 10 x64, Windows 7 x86, Mac OS X 10.13 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: