+++ This bug was initially created as a clone of Bug #1400259 +++ STR: 1. Open "Hamburger" menu 2. Press "Help" item 3. Press "Esc" keyboard button *after the subview starts sliding in but before it finishes*. 4. reopen the hamburger menu ER: you get a normal menu AR: you get a tiny ~30x30px square with nothing in it as a panel. Looking at the DOM structure, none of the panelviews have a 'current' attribute, so I don't think the state machine that's tracking the transitions etc. is reversing its actions appropriately, somehow.

I wonder how far this goes towards fixing 1399230... anyway, one thing at a time.

For what it's worth, Gecko implements the 'transitioncancel' event if you want to detect aborted transitions.

(In reply to Brian Birtles (:birtles) from comment #3) > For what it's worth, Gecko implements the 'transitioncancel' event if you > want to detect aborted transitions. Thanks for the tip. I'm aware of transitioncancel, but unfortunately using that + transitionstart isn't something we've created a good pattern for in the frontend code at this stage. We should probably revisit some of our code with that in mind in the future - but not this close to merge day. In this case the popuphidden event is good enough, and the main issue (which the patch addresses) is the (order of) work that needs to happen to reverse the partially-finished switch from one view to another when it does get aborted, and passing the relevant data in order to be able to do that work.

Comment on attachment 8910031 [details] Bug 1401383 - remove anchor state after transition even if the transition is canceled, and always set main view as current, https://reviewboard.mozilla.org/r/181500/#review187058 Thanks, Gijs!

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/658b68ee4d0a remove anchor state after transition even if the transition is canceled, and always set main view as current, r=mikedeboer

Backed out for frequently failing browser-chrome's browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js, especially on Linux x64 asan: https://hg.mozilla.org/integration/autoland/rev/dbe8d28ff6effc8ceb07fdd38b73cc0815b237be Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=658b68ee4d0ae1b6c5ffb44ed38c310f50d4b283&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=132202782&repo=autoland [task 2017-09-20T14:37:30.138Z] 14:37:30 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | 68 - [task 2017-09-20T14:37:30.140Z] 14:37:30 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Panel has not shrunk from original size (723 >= 683) - [task 2017-09-20T14:37:30.145Z] 14:37:30 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Bottom of popup should be on-screen. (723 <= 1200) - [task 2017-09-20T14:37:30.147Z] 14:37:30 INFO - Restore original styling. Expect original dimensions. [task 2017-09-20T14:37:30.149Z] 14:37:30 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Browser height should return to its original value - [task 2017-09-20T14:37:30.151Z] 14:37:30 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Window width should not change - [task 2017-09-20T14:37:30.155Z] 14:37:30 INFO - Buffered messages finished [task 2017-09-20T14:37:30.158Z] 14:37:30 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Window height should return to its original value - Got 174, expected 560 [task 2017-09-20T14:37:30.160Z] 14:37:30 INFO - Stack trace: [task 2017-09-20T14:37:30.164Z] 14:37:30 INFO - chrome://mochikit/content/browser-test.js:test_is:1011 [task 2017-09-20T14:37:30.168Z] 14:37:30 INFO - chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:testPopupSize:270 It also fails frequently on Linux and Windows debug and opt, see e.g. https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=969ff8612b148e51b3cafaa369859ca2ffbb8425&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Of course, I can't reproduce this locally, not even with a debug build, and the screenshot from the failing build shows no panels open at all. Not sure where to begin here. :-\

I did recently disable the test on OSX - so are you sure it's running for you?

(In reply to Mike de Boer [:mikedeboer] from comment #9) > I did recently disable the test on OSX - so are you sure it's running for > you? Yep, trying to reproduce on Windows, no luck either with --run-until-failure or running the entire dir (though other tests like browser_ext_popup_select fail with hidpi-caused off-by-one errors). Seeing as that failed, trying an interactive loaner and a debug artifact build on my linux vm next. Though tbh, on Linux the failures happened on pgo builds too, so not sure what the deal is.

FTR, I hate the guts of this test so much... I'm tempted to take it down and re-implement it using a specific design goal in mind.

tracking as it sounds like we might need to uplift a fix for this to 57 beta

Yes. And Gijs, this patch is really important to land, because without it, it's easier to break the panel than I thought initially.

Per discussion on IRC, Mike is taking a look at this.

Comment on attachment 8910752 [details] Bug 1401383 - remove anchor state after transition even if the transition is canceled, and always set main view as current, Still working on this...

Comment on attachment 8910752 [details] Bug 1401383 - remove anchor state after transition even if the transition is canceled, and always set main view as current, https://reviewboard.mozilla.org/r/182220/#review187608 ::: browser/components/customizableui/PanelMultiView.jsm:435 (Diff revision 1) > this._currentSubView = null; > this.node.setAttribute("viewtype", "main"); > }); > } > } else if (this.panelViews) { > - this._mainView.setAttribute("current", "true"); > + this.hideAllSubViewsButOne(); Hrm, but we don't do this in the `showingSubView` case? Shouldn't we always do it? Don't get me wrong, if this fixes the issue and the test is happy, we can land it as-is, but it'd be good to understand why this works the way it does. ::: browser/components/customizableui/PanelMultiView.jsm:443 (Diff revision 1) > if (!this.panelViews) { > this._shiftMainView(); > } > } > > + hideAllSubViewsButOne(theOne = this._mainView) { Right, nice. Do we maybe want to call this instead of only cleaning up `previousView` in the `_cleanupTransitionPhase` code? ::: browser/components/customizableui/PanelMultiView.jsm:539 (Diff revision 1) > this.node.setAttribute("viewtype", "main"); > } else { > this.node.setAttribute("viewtype", "subview"); > } > + // If we've got an older transition still running, make sure to clean it up. > + await this._cleanupTransitionPhase(); I wonder if we need to generalize this somewhat to cope with bug 1401991 in all cases? That is, we will need to clean up nodes that have just moved into our panelmultiview based on the transitionphase they had in the other panelmultiview. Don't need to do that here (heaven forbid) but I imagine there's work left. Then again, I imagine you knew that already, just jotting it down for posterity I guess... ::: browser/components/customizableui/PanelMultiView.jsm:719 (Diff revision 1) > - let {phase, previousViewNode, viewNode, reverse, resolve, listener} = this._transitionDetails; > + let {phase, previousViewNode, viewNode, reverse, resolve, listener, anchor} = this._transitionDetails; > this._transitionDetails = null; > > // Do the things we _always_ need to do whenever the transition ends or is > // interrupted. > this._dispatchViewEvent(previousViewNode, "ViewHiding"); I think I forgot to mention this, but this can fire twice - once from `showMainView`, once here. Again, follow-up fodder if the current thing works, but I suppose we should deal with it one way or another. I suspect it only needs to fire in this instance if the panel is open (as `showMainView` would have fired it otherwise). Though, the other thing is whether we should always fire this iff we've fired `ViewShowing` for the same view, or maybe only if we've fired `ViewShown`? Right now we guarantee neither of these things, I believe, it's somewhere in the middle. ::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:164 (Diff revision 1) > + let shownPromise = new Promise(resolve => { > + panelMultiView.addEventListener("ViewShown", function listener(event) { > + let id = event.originalTarget.id || ""; > + if (id.includes(widgetId)) { > + panelMultiView.removeEventListener("ViewShown", listener); > + resolve(event.originalTarget); > + } > + }); > + }); You could use: ```js BrowserTestUtils.waitForEvent(panelMultiView, "ViewShown", e => (e.originalTarget.id || "").includes(widgetId)); ``` ::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:201 (Diff revision 1) > - // panelmultiview trips up our width checking causing it to be off-by-one. > - await BrowserTestUtils.waitForCondition(() => (!panel.hasAttribute("width") && (!panelview || !panelview.style.borderInlineStart))); > - await promiseAnimationFrame(browserWin); > // Wait long enough to make sure the initial resize debouncing timer has > // expired. > - await delay(100); > + await delay(500); Hrm, cheeky. OK for now, I think, but should we have a webextensions followup bug for just getting rid of this altogether? I don't really understand why it's necessary to begin with, but it's "always" been there.

(In reply to :Gijs from comment #18) > Hrm, cheeky. OK for now, I think, but should we have a webextensions > followup bug for just getting rid of this altogether? I don't really > understand why it's necessary to begin with, but it's "always" been there. This is to ensure that this timer has passed: https://hg.mozilla.org/mozilla-central/file/tip/browser/components/customizableui/PanelMultiView.jsm#l730 Now I was playing with the idea to do something like ```js this._autoResizeWorkaroundTimer = this.window.setTimeout(() => { if (!this._viewContainer) return; this._viewContainer.style.removeProperty("height"); this._viewContainer.style.removeProperty("width"); this._dispatchViewEvent(viewNode, "ViewShownForTest"); }, 500); ``` ...to help this test specifically. What do you think?

(In reply to Mike de Boer [:mikedeboer] from comment #19) > (In reply to :Gijs from comment #18) > > Hrm, cheeky. OK for now, I think, but should we have a webextensions > > followup bug for just getting rid of this altogether? I don't really > > understand why it's necessary to begin with, but it's "always" been there. > > This is to ensure that this timer has passed: > https://hg.mozilla.org/mozilla-central/file/tip/browser/components/ > customizableui/PanelMultiView.jsm#l730 > > Now I was playing with the idea to do something like > ```js > this._autoResizeWorkaroundTimer = this.window.setTimeout(() => { > if (!this._viewContainer) > return; > this._viewContainer.style.removeProperty("height"); > this._viewContainer.style.removeProperty("width"); > this._dispatchViewEvent(viewNode, "ViewShownForTest"); > }, 500); > ``` > ...to help this test specifically. What do you think? Not sure we need it, it looks like try was green and the event isn't necessarily prettier. I'm actually kind of surprised this works in that the 2 timers aren't necessarily guaranteed to fire in the 'right' order... anyway, at this stage, I'll take what I can get to get this stuff landed and uplifted without massive amounts of intermittent orange.

(In reply to :Gijs from comment #18) > Hrm, but we don't do this in the `showingSubView` case? Shouldn't we always > do it? No, because when `showingSubView` it'll call `showSubView`, which sets the attributes correctly as well. > Right, nice. Do we maybe want to call this instead of only cleaning up > `previousView` in the `_cleanupTransitionPhase` code? Maybe, but let's keep it under wraps as a measure when a bug is filed where this may be a proper fix, because I don't want to take this as a wallpaper measure against all the things that could potentially go wrong. I feel the same way that this is indeed the iffy-est part of the code right now, even though we are still able to reason about things. It'll save us a lot of complexity once we've migrated the Identity panel and cleaned things up here. > I wonder if we need to generalize this somewhat to cope with bug 1401991 in > all cases? I think this already properly fixes bug 1401991 and we don't need to do more right now. > I think I forgot to mention this, but this can fire twice - once from > `showMainView`, once here. Not in practice, because `showingSubView` will be set correctly. This did remind me to amend this patch, though! > ```js > BrowserTestUtils.waitForEvent(panelMultiView, "ViewShown", > e => (e.originalTarget.id || "").includes(widgetId)); > ``` Cool, changed.

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3fadde636965 remove anchor state after transition even if the transition is canceled, and always set main view as current, r=Gijs

Please request Beta approval on this when you get a chance.

Comment on attachment 8910752 [details] Bug 1401383 - remove anchor state after transition even if the transition is canceled, and always set main view as current, Approval Request Comment [Feature/Bug causing the regression]: bug 1374749 [User impact if declined]: Empty panel when the STR in comment 0 are followed. [Is this code covered by automated tests?]: No. [Has the fix been verified in Nightly?]: No. [Needs manual test from QE? If yes, steps to reproduce]: Yes, please see comment 0 for STR. [List of other uplifts needed for the feature/fix]: Just this one patch. [Is the change risky?]: No. [Why is the change risky/not risky?]: Because it improves the status quo a lot. [String changes made/needed]:

Verified on Windows, Mac, and Ubuntu.

Comment on attachment 8910752 [details] Bug 1401383 - remove anchor state after transition even if the transition is canceled, and always set main view as current, Fix a bad UX + has been verified, taking it. Should be in 57b4 (gtb tomorrow Thursday)

Let's make sure this works as intended on Beta 57 as well.

It seems that the fix in Bug 1401991 may have introduced the regression found in Bug 1407591. Bug 1407591 seems to affect the verification of this fix on Linux.

I have managed to reproduce the issue mentioned in comment 0 using an affected Firefox 57.0a1 (BuildId:20170919220202) build. This issue is verified fixed on Firefox 57.0b12 (BuildId:20171026211016) using Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.