Fix layout issues in Library Panel subviews

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
3 months ago
20 days ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months 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.
Flags: qe-verify+

Updated

3 months ago
Priority: P1 → --
QA Contact: gwimberly
Whiteboard: [photon-structure] → [photon-structure] [triage]
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.

Updated

3 months ago
Priority: -- → P2
Whiteboard: [photon-structure] [triage] → [photon-structure]
(Assignee)

Updated

3 months ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED

Updated

3 months ago
Iteration: --- → 55.7 - Jun 12
Priority: P2 → P1
Comment hidden (mozreview-request)

Comment 3

3 months 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?
Attachment #8876176 - Flags: review?(gijskruitbosch+bugs) → review-
(Assignee)

Comment 4

2 months 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.
Comment hidden (mozreview-request)

Comment 6

2 months 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.
Attachment #8876176 - Flags: review?(gijskruitbosch+bugs)

Updated

2 months ago
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
(Assignee)

Comment 7

2 months 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.
Comment hidden (mozreview-request)

Comment 9

2 months 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.
Attachment #8876176 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 months 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!
Attachment #8876176 - Flags: review+ → review?(gijskruitbosch+bugs)

Comment 12

2 months 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! :-)
Attachment #8876176 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 13

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

\o/

Comment 14

2 months 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
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
Flags: needinfo?(mdeboer)
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

Updated

2 months ago
Duplicate of this bug: 1370082
Comment hidden (mozreview-request)
(Assignee)

Comment 19

2 months 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.
Flags: needinfo?(mdeboer)
Attachment #8876176 - Flags: review+ → review?(gijskruitbosch+bugs)

Comment 20

2 months 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?
Attachment #8876176 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 21

2 months 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.
Comment hidden (mozreview-request)

Comment 23

2 months 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

Comment 24

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/af176d040fad
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

2 months ago
Depends on: 1373960

Updated

2 months ago
No longer depends on: 1373960
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. -
Status: RESOLVED → REOPENED
Flags: needinfo?(mdeboer)
Resolution: FIXED → ---
(Assignee)

Comment 26

2 months ago
This test failing had nothing to do with the patch in this bug. I'm attempting a re-land.
Flags: needinfo?(mdeboer)

Comment 27

2 months 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

Updated

2 months ago
Duplicate of this bug: 1374314

Comment 29

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0a62e989c96
Status: REOPENED → RESOLVED
Last Resolved: 2 months ago2 months ago
Resolution: --- → FIXED

Comment 30

2 months 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]

Comment 31

2 months 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
QA Whiteboard: [bugday-20170621]

Comment 32

2 months ago
As per Comment 30 and Comment 31, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified

Updated

2 months ago
Flags: qe-verify+
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.