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)
Tracking
()
VERIFIED
FIXED
Firefox 58
People
(Reporter: epang, Assigned: ewright)
References
Details
(Whiteboard: [reserve-photon-animation] )
Attachments
(2 files)
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%
Reporter | ||
Updated•7 years ago
|
Whiteboard: [photon-animation] [triage] [ux] → [reserve-photon-animation]
Updated•7 years ago
|
Flags: qe-verify?
Priority: -- → P4
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: stefan.georgiev
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ewright
Updated•7 years ago
|
Status: NEW → ASSIGNED
Priority: P4 → P1
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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)
Updated•7 years ago
|
Attachment #8906099 -
Flags: review?(gijskruitbosch+bugs)
Comment 4•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
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.
Comment 6•7 years ago
|
||
Can this just be added to CustomizeMode.addToPanel() ? It's all part of the "add to panel" animation anyways.
Assignee | ||
Comment 7•7 years ago
|
||
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.
Comment 8•7 years ago
|
||
(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.
Assignee | ||
Comment 9•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
The current patch has an issue with adding "flexible space". But related issues already exist with that.
Comment 12•7 years ago
|
||
(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 13•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 14•7 years ago
|
||
(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)
Assignee | ||
Comment 15•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
: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.
Comment 18•7 years ago
|
||
(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 19•7 years ago
|
||
mozreview-review |
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+
Comment 20•7 years ago
|
||
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).
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
(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 23•7 years ago
|
||
mozreview-review |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•7 years ago
|
||
:gijs, do you have any further advice for this one?
Flags: needinfo?(gijskruitbosch+bugs)
Comment 26•7 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(ewright)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
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
Assignee | ||
Comment 31•7 years ago
|
||
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)
Comment 32•7 years ago
|
||
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 33•7 years ago
|
||
mozreview-review |
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 ?
Comment 34•7 years ago
|
||
Clearing ni for comment #33, but please let me know if you need help with the remaining test.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 35•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 37•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 38•7 years ago
|
||
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
![]() |
||
Comment 39•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1053bf2fbaed
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Updated•7 years ago
|
Comment 40•7 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•