Mike de Boer [:mikedeboer] Assignee
	Description • 7 years ago

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

	Dave Townsend [:mossop]
	Comment 1 • 7 years ago

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.

	:Gijs (he/him)
	Comment 3 • 7 years ago
mozreview-review

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?

	Mike de Boer [:mikedeboer] Assignee
	Comment 4 • 7 years ago

(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.

	:Gijs (he/him)
	Comment 6 • 7 years ago
mozreview-review

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.

	Mike de Boer [:mikedeboer] Assignee
	Comment 7 • 7 years ago

(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.

	:Gijs (he/him)
	Comment 9 • 7 years ago
mozreview-review

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.

	Mike de Boer [:mikedeboer] Assignee
	Comment 11 • 7 years ago

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!

	:Gijs (he/him)
	Comment 12 • 7 years ago
mozreview-review

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! :-)

	Mike de Boer [:mikedeboer] Assignee
	Comment 13 • 7 years ago

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

\o/

	Pulsebot
	Comment 14 • 7 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 15 • 7 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 16 • 7 years ago

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

	Mike de Boer [:mikedeboer] Assignee
	Comment 19 • 7 years ago

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.

	:Gijs (he/him)
	Comment 20 • 7 years ago
mozreview-review

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?

	Mike de Boer [:mikedeboer] Assignee
	Comment 21 • 7 years ago

(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.

	Pulsebot
	Comment 23 • 7 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 24 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/af176d040fad

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 25 • 7 years ago

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. -

	Mike de Boer [:mikedeboer] Assignee
	Comment 26 • 7 years ago

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

	Pulsebot
	Comment 27 • 7 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 29 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f0a62e989c96

	Saddam Hossain
	Comment 30 • 7 years ago

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]

	Md. Majedul islam
	Comment 31 • 7 years ago

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

	Saddam Hossain
	Comment 32 • 7 years ago

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