When selecting a subview in the Library Panel, there are sizing and scrolling behavior issues. It'd be best to fix them all here.

I see issues both with subviews of the Library panel and the Library panel itself. In both cases sometimes opening the panel I will see a brief flicker as if the panel is displayed fully open and then animates in. In both cases sometimes the panel opens to the left of the main menu and then animates to there before jumping back to overlay the main menu.

Comment on attachment 8876176 [details] Bug 1370580 - Part 1 - Ensure that the overflow rules are set the exact same way for the temporary panel as for the appMenu panel. https://reviewboard.mozilla.org/r/147596/#review151966 ::: browser/themes/shared/customizableui/panelUI.inc.css:353 (Diff revision 1) > -#appMenu-popup panelview { > +#appMenu-popup panelview, > +#customizationui-widget-multiview panelview { > min-width: @menuPanelWidth@; > } No, I specifically removed this in bug 1369564. This causes webextension and other popups to be too wide. Why do we need this? Should we instead set it on a specific set of panelviews? ::: browser/themes/shared/customizableui/panelUI.inc.css:2087 (Diff revision 1) > + overflow-x: visible; > + overflow-y: visible; Should this just be overflow: visible?

(In reply to :Gijs from comment #3) > No, I specifically removed this in bug 1369564. This causes webextension and > other popups to be too wide. > > Why do we need this? Should we instead set it on a specific set of > panelviews? I'll put it on the library panel specifically instead. Thanks for the reminder that this is a bad idea! > Should this just be overflow: visible? No, this is a photon-specific override and both are different than what we currently have.

Comment on attachment 8876176 [details] Bug 1370580 - Part 1 - Ensure that the overflow rules are set the exact same way for the temporary panel as for the appMenu panel. https://reviewboard.mozilla.org/r/147596/#review152440 Code-wise, this looks fine. However, I (still?) see flashing with this approach: https://www.screencast.com/t/nmWIHLLQ (for me, this displays best on OS X. There seem to be invalidation issues with flash on Windows. Happy to do a screencast with some other tool if you can recommend something better than Jing.) ::: browser/themes/shared/customizableui/panelUI.inc.css:354 (Diff revision 2) > background: var(--arrowpanel-background); > padding: 6px 0; > } > > -#appMenu-popup panelview { > +#appMenu-popup panelview, > +#customizationui-widget-multiview #appMenu-libraryView { I think we can just do this for the #appMenu-libraryView generally? The only other place it gets displayed is inside the #appMenu-popup, so I think that works? More generally, also with bug 1370083 in mind, I wonder if we need some way to apply this to all panels that aren't provided by add-ons (e.g. what about the history or developer tools panels?). I wonder if there is a webextension attribute or class we can use for this. It looks like there isn't anything, but we could either use :not([id^="PanelUI-webext-"]) (which is a bit hacky) or add an attribute in ext-browserAction.js . Probably a separate bug? ::: browser/themes/shared/customizableui/panelUI.inc.css:2086 (Diff revision 2) > +photonpanelmultiview .cui-widget-panelview { > + overflow-x: visible; > + overflow-y: visible; Please add a comment about why this is needed / what it's overriding.

(In reply to :Gijs from comment #6) > Code-wise, this looks fine. However, I (still?) see flashing with this > approach: > > https://www.screencast.com/t/nmWIHLLQ > > (for me, this displays best on OS X. There seem to be invalidation issues > with flash on Windows. Happy to do a screencast with some other tool if you > can recommend something better than Jing.) This is a different issue, quite specific to the history subview. Is it using the `.addBlocker()` things and perhaps that just whacks things up atm. I say this, because the second time you select this view, it doesn't flicker. I'll look into it, but will push it as a separate patch to this bug. I applied your suggestion to add an attribute to webextension panels, to exclude them from having a default min-width.

Comment on attachment 8876176 [details] Bug 1370580 - Part 1 - Ensure that the overflow rules are set the exact same way for the temporary panel as for the appMenu panel. https://reviewboard.mozilla.org/r/147596/#review153444 FWIW, when I tried this last time, I saw a flash repeatedly, but I agree we can think about looking into this separately if it's limited to the history view.

Comment on attachment 8876176 [details] Bug 1370580 - Part 1 - Ensure that the overflow rules are set the exact same way for the temporary panel as for the appMenu panel. This fixed the History view flickering for me. Can you please take a look? Thanks!

Comment on attachment 8876176 [details] Bug 1370580 - Part 1 - Ensure that the overflow rules are set the exact same way for the temporary panel as for the appMenu panel. https://reviewboard.mozilla.org/r/147596/#review154030 Naice! :-)

(In reply to :Gijs from comment #12) > Naice! :-) \o/

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d6be7dc56cc5 Part 1 - Ensure that the overflow rules are set the exact same way for the temporary panel as for the appMenu panel. r=Gijs

Backed out for failing test-oop-extensions/browser_ext_browserAction_popup_resize.js: https://hg.mozilla.org/integration/autoland/rev/7243e9c89663a9eaea708c0de888cb463ad888be Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d6be7dc56cc5956595870738bbdef102228148de&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=107387519&repo=autoland [task 2017-06-15T16:43:17.528118Z] 16:43:17 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Document has the expected compat mode - [task 2017-06-15T16:43:17.530381Z] 16:43:17 INFO - Increase body children's width. Expect them to wrap, and the frame to grow vertically rather than widen. [task 2017-06-15T16:43:17.534017Z] 16:43:17 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Browser height should increase (448 > 174) - [task 2017-06-15T16:43:17.536145Z] 16:43:17 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Window width should not change - [task 2017-06-15T16:43:17.540348Z] 16:43:17 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Window height should increase (448 >= 174) - [task 2017-06-15T16:43:17.545743Z] 16:43:17 INFO - Buffered messages finished [task 2017-06-15T16:43:17.548030Z] 16:43:17 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js | Document should not be vertically scrollable - Got 10, expected 0 [task 2017-06-15T16:43:17.550203Z] 16:43:17 INFO - Stack trace: [task 2017-06-15T16:43:17.556294Z] 16:43:17 INFO - chrome://mochikit/content/browser-test.js:test_is:1010 [task 2017-06-15T16:43:17.558368Z] 16:43:17 INFO - chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:testPopupSize:217

Also fails browser/components/customizableui/test/browser_947914_button_history.js at least on OS and Windows: https://treeherder.mozilla.org/logviewer.html#?job_id=107389383&repo=autoland

Comment on attachment 8876176 [details] Bug 1370580 - Part 1 - Ensure that the overflow rules are set the exact same way for the temporary panel as for the appMenu panel. I needed to make some extra changes to make WebExt panels behave. We should file a bug & talk with that team about what the _exact_ spec is for these panels in the Photon UI, because this is not maintainable.

Comment on attachment 8876176 [details] Bug 1370580 - Part 1 - Ensure that the overflow rules are set the exact same way for the temporary panel as for the appMenu panel. https://reviewboard.mozilla.org/r/147596/#review154458 ::: browser/components/extensions/ext-browserAction.js:165 (Diff revisions 4 - 5) > }, > > onViewShowing: event => { > let document = event.target.ownerDocument; > let tabbrowser = document.defaultView.gBrowser; > + event.target.setAttribute("current", true); Ew. This feels a bit hacky... can you add a comment here about why we need to do this? Also, presumably we should only do this if we actually have a view to show here (ie inside the if (popupURL) block) ? Maybe before the addBlocker() call? ::: browser/components/customizableui/PanelMultiView.jsm:635 (Diff revision 5) > }); > }, { once: true }); > }); > } else if (!this.panelViews) { > this._transitionHeight(() => { > viewNode.setAttribute("current", true); Is this redundant now? ::: browser/components/extensions/test/browser/browser_ext_popup_corners.js:6 (Diff revision 5) > /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */ > /* vim: set sts=2 sw=2 et tw=80: */ > "use strict"; > > add_task(async function testPopupBorderRadius() { > + await SpecialPowers.pushPrefEnv({set: [["browser.photon.structure.enabled", false]]}); Out of curiosity, why is this now necessary, and not before this patch?

(In reply to :Gijs from comment #20) > Ew. This feels a bit hacky... can you add a comment here about why we need > to do this? Also, presumably we should only do this if we actually have a > view to show here (ie inside the if (popupURL) block) ? Maybe before the > addBlocker() call? Indeed, I was able to add it there and the test didn't fail on me. > Is this redundant now? Always has been! However, I'm afraid to change more and more. > Out of curiosity, why is this now necessary, and not before this patch? Because this patch introduces a border-radius of `0` for temporary panels. So in this test, without the pref set, the panel sometimes has a border radius and sometimes does not, because we haven't converted all the panels yet. So, during the interim, we force the old panel styles.

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/af176d040fad Part 1 - Ensure that the overflow rules are set the exact same way for the temporary panel as for the appMenu panel. r=Gijs

Backed out for frequently failing browser_ext_sidebarAction.js on Linux debug: https://hg.mozilla.org/mozilla-central/rev/316014448e427486cce211217e2aee2f18ee3d69 See https://treeherder.mozilla.org/#/jobs?repo=autoland&tochange=9afdcc19353075ad2e4054ac1f487266d429d257&fromchange=ec461d56ce3a2d9cf2823521a36d66781fcbfbc2&filter-searchStr=3a1e40728c6f1e953ecbc06e8ff8c6bb43d59375 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=107890607&repo=autoland [task 2017-06-17T11:16:56.712895Z] 11:16:56 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_sidebarAction.js | sidebar box is visible in first window - [task 2017-06-17T11:16:56.716149Z] 11:16:56 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_sidebarAction.js | unable to set empty panel - [task 2017-06-17T11:16:56.719223Z] 11:16:56 INFO - Buffered messages logged at 11:16:55 [task 2017-06-17T11:16:56.721350Z] 11:16:56 INFO - Console message: Warning: attempting to write 8780 bytes to preference extensions.webextensions.uuids. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file. This preference will not be sent to any content processes. [task 2017-06-17T11:16:56.727975Z] 11:16:56 INFO - Leaving test bound sidebar_empty_panel [task 2017-06-17T11:16:56.729725Z] 11:16:56 INFO - Entering test bound cleanup [task 2017-06-17T11:16:56.735257Z] 11:16:56 INFO - Leaving test bound cleanup [task 2017-06-17T11:16:56.737035Z] 11:16:56 INFO - Buffered messages finished [task 2017-06-17T11:16:56.739165Z] 11:16:56 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_sidebarAction.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -

This test failing had nothing to do with the patch in this bug. I'm attempting a re-land.

Pushed by mdeboer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f0a62e989c96 Part 1 - Ensure that the overflow rules are set the exact same way for the temporary panel as for the appMenu panel. r=Gijs

I have reproduced this Bug on Nightly 55.0a1 (2017-06-06) on Windows 10, 64 Bit! The bug's fix is now verified on latest Nightly 56.0a1 Build ID : 20170620030208 User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0 [bugday-20170621]

I have reproduced this bug with nightly 55.0a1 (2017-06-06) on ubuntu 16.04(64 Bit). The bug's fix is now verified on Latest Nightly 56.0a1. Build ID 20170620100236 User Agent Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0

As per Comment 30 and Comment 31, I am marking this bug as verified fixed.