Closed Bug 1402845 Opened 7 years ago Closed 6 years ago

When uBlock, NoScript and other browserAction buttons are in the overflow menu, their panel is shown with wrong size and with a "two step resizing"

Categories

(Firefox :: Toolbars and Customization, defect, P3)

Product:
Firefox
Component:
Toolbars and Customization
Version:
57 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox57 - wontfix
firefox58 + wontfix
firefox59 --- wontfix
firefox60 --- verified
firefox61 --- verified

People

(Reporter: filippo.mannino, Assigned: sfoster)

References

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(7 files, 1 obsolete file)

Phase 1.png
32.29 KB, image/png
Details
Phase 2.png
41.15 KB, image/png
Details
Bug 1402845 - Fix panelview sizing when customRectGetter is used.
59 bytes, text/x-review-board-request
Paolo
: review+
Details
screenshot of overflow menu opening from bottom
85.33 KB, image/png
Details
NoScript's UI opened from the overflow menu
140.17 KB, image/png
Details
Ublock_open.swf
670.26 KB, application/x-shockwave-flash
Details
doorhanger 2 phases..gif
1.03 MB, image/gif
Details
Attached image Phase 1.png — 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20170924220116

Steps to reproduce:

Install uBlock Origin webextension.
Put uBlock Origin button in the "extra" menu (the one with double chevron, don't remember the right name).
Open the "extra" menu and click on the uBlock Origin button.


Actual results:

uBlock Origin panel is shown with a wrong size and with a "two phase" resizing: a first phase with wrong width and height, and (after about one second) a second phase with correct height and wrong width.


Expected results:

uBlock Origin panel should be shown with a correct size.
Attached image Phase 2.png — 

    
  

          status-firefox57:
          --- → ?

          status-firefox58:
          --- → ?

          tracking-firefox57:
          --- → ?

          tracking-firefox58:
          --- → ?
Component: Untriaged → Theme

    
  
See Also:  → 1374749, 1373490
Whiteboard: [photon-structure][triage]

    
  
Status: UNCONFIRMED → NEW
Has STR: --- → yes

          status-firefox57:
          ? → affected

          status-firefox58:
          ? → affected
Ever confirmed: true

    
  
Summary: When uBlock Origin button is put in the "extra" menu, uBlock panel is shown with wrong size and with a "two step resizing" → When uBlock Origin button is put in the overflow menu, uBlock panel is shown with wrong size and with a "two step resizing"
Flags: qe-verify+
Priority: -- → P3
QA Contact: gwimberly
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Depends on: 1401991

    
    

    
  
Given that this was triaged as a P3, I doubt it will be fixed in 57 release. Untracked.

          status-firefox57:
          affected → wontfix

          tracking-firefox57:
          ? → -

    
    

    
  
(In reply to Ritu Kothari (:ritu) from comment #3)
> Given that this was triaged as a P3, I doubt it will be fixed in 57 release.
> Untracked.

Yeah, that's not what P3 means within structure. I don't know why we have 2 different interpretations of priorities, but we do. Re-setting some flags.

          status-firefox57:
          wontfix → fix-optional

    
    

    
  
I'm looking into this. This looks like a panelmultiview bug, starting to dig... but if someone more familiar with the panelview code wants to steal it to get this closed out quickly, or wants to feed me suggestions, let me know.
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Priority: P3 → P1

    
  
See Also:  → 1383294
Assignee: sfoster → mdeboer
Blocks: 1282189

    
    

    
      
  


  

    
    

    
  
Comment on attachment 8915906 [details]
Bug 1402845 - Don't set a max-height on panelviews used by WebExtensions, because it causes browser contents to be cut-off.

https://reviewboard.mozilla.org/r/187162/#review192736

This change doesn't seem to fix anything for me - I still see the same 2-step resizing behaviour that appears pre-patch.

        Attachment #8915906 -
        Flags: review?(gijskruitbosch+bugs)

    
  
Blocks: 1403466

    
    

    
  
After re-testing on OS X, I see that this fixes bug 1403466, but not this bug. So, can you move the patch and/or land on inbound with that bug number? r=me on the code and fixing bug 1403466, but this bug needs more fixes than just this patch.
Flags: needinfo?(mdeboer)

    
    

    
  
It does indeed. I'll push this patch as part of bug 1403466 instead. I'll make sure to push to try before I do that ;-)

The sizing behavior is due to the WebExtension page/ content script passing the document dimensions in two steps. But since the XUL layout where the <browser> is in, it might be another interaction burp between the two.
Flags: needinfo?(mdeboer)

        Attachment #8915906 -
        Attachment is obsolete: true

    
    

    
  
Not actively working on this right now.
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3

    
    

    
  
I'll take this back. As bug 1282189 has a patch that should make it to central shortly, I've reversed the dependency and my patch will build on that work.
Assignee: nobody → sfoster
No longer blocks: 1403466
Status: NEW → ASSIGNED
Depends on: 1403466
Priority: P3 → P1

    
    

    
  
(In reply to Sam Foster [:sfoster] from comment #12)
> I'll take this back. As bug 1282189 has a patch that should make it to
> central shortly, I've reversed the dependency and my patch will build on
> that work.

Wrong bug. Bug 1403466 has the patch I want to build on.

    
    

    
  
Kris, while looking into this, I noticed that the uBlock Origin extension (and possibly others) triggers a steady stream of Extension:BrowserResized messages from http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-browser-content.js#287 while this popup is open. We handle these by updating the browser width/height in http://searchfox.org/mozilla-central/source/browser/components/extensions/ExtensionPopups.jsm#323, and forwarding as a "WebExtPopupResized" message. All without checking if the size actually changed. I'm not sure if this hurts much, but I'm thinking we could at least return early from BasePopup resizeBrowser? 

Probably orthogonal to this bug, so I'm not blocked on this.
Flags: needinfo?(kmaglione+bmo)

    
    

    
  
Here's what I know so far. When we prepare to switch views in the panelmultiview,, we attempt to calculate the dimensions for the incoming viewpanel by calling the customRectGetter to know the extension content size, and then adding the height of the header[1]

Problem is, the header has height 0 at that point. So the panel transition plays using a height that is short the 40px of the header. When the transition ends and after the _autoResizeWorkaroundTimer, the style.height property is removed[2], and the panel subsequently resizes to its correct height - which is the 2-step effect from comment #0. 

I'm not sure yet what the right fix is here. With the CSS we have today, we actually know that if there's a header at all, it will always be 40px tall, so we could conceivably hard-code this (ick?) Otherwise, we'll need to re-jigger what gets measured and sized when. That looks like either quite a bit of re-ordering of the sequence in _transitionViews, and/or some multi-step assignment, then promiseLayoutFlushed, measure, assign again type of stuff. Before I embark on that, I thought I'd get your thoughts Mike? 

[1] http://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm#677
[2] http://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm#799
Flags: needinfo?(mdeboer)

    
    

    
  
(In reply to Sam Foster [:sfoster] from comment #15)
> Kris, while looking into this, I noticed that the uBlock Origin extension
> (and possibly others) triggers a steady stream of Extension:BrowserResized
> messages from
> http://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> ext-browser-content.js#287 while this popup is open. We handle these by
> updating the browser width/height in
> http://searchfox.org/mozilla-central/source/browser/components/extensions/
> ExtensionPopups.jsm#323, and forwarding as a "WebExtPopupResized" message.
> All without checking if the size actually changed. I'm not sure if this
> hurts much, but I'm thinking we could at least return early from BasePopup
> resizeBrowser? 
> 
> Probably orthogonal to this bug, so I'm not blocked on this.

I've seen this too - the MutationObserver is firing continuously, because uBlock updates DOM attributes for styling purposes through a polling loop.
Flags: needinfo?(mdeboer)

    
    

    
  
(In reply to Sam Foster [:sfoster] from comment #16)
> Here's what I know so far. When we prepare to switch views in the
> panelmultiview,, we attempt to calculate the dimensions for the incoming
> viewpanel by calling the customRectGetter to know the extension content
> size, and then adding the height of the header[1]
> 
> Problem is, the header has height 0 at that point. So the panel transition
> plays using a height that is short the 40px of the header. When the
> transition ends and after the _autoResizeWorkaroundTimer, the style.height
> property is removed[2], and the panel subsequently resizes to its correct
> height - which is the 2-step effect from comment #0.

Really? I think the header being 0-height is something that hints at the fact that we're not waiting long enough for content layout to settle and have the correct measurements, right?
Perhaps we need to wait for a layout flush before we call customRectGetter? 

BTW, I'm all for caching the header size at a later point, once we do get it. But that won't help us here, unless we really start hardcoding the 40px - which I think is a no-go.

I'd be interested what you'll get when you wait for layout to settle and starts to report proper values.

    
    

    
      
  

    
    

    
  
Mike spotted that we need to move the addition of the in-transition class up ahead of any measurement - without this we have a CSS rule that makes the panelview visibility:collapse. When I applied that fix, I still had strange two-step behavior and needed to also restore a fix I had spotted earlier: when attaching the browser to the view, we *do* want to resize with any height stashed in the dimensions property. With the old logic, this gets skipped when we are fixedWidth.

    
    

    
      
  

    
    

    
  
Comment on attachment 8919871 [details]
Bug 1402845 - Fix panelview sizing when customRectGetter is used.

https://reviewboard.mozilla.org/r/190816/#review196226

::: browser/components/customizableui/PanelMultiView.jsm:679
(Diff revision 3)
> -      viewRect = Object.assign({height, width}, viewNode.customRectGetter());
> +      // viewNode has visibility:collapse until it is in-transition
> +      viewNode.setAttribute("in-transition", true);
> +      viewRect = Object.assign({height, width},
> +        await BrowserUtils.promiseLayoutFlushed(this.document, "layout", () => {
> +          return viewNode.customRectGetter();
> +      }));

nit: Can you format this slightly differently?

```js
viewRect = Object.assign({height, width},
  await BrowserUtils.promiseLayoutFlushed(this.document, "layout", () => {
    return viewNode.customRectGetter();
  })
);
```

You can even go for:

```js
viewRect = Object.assign({height, width}, await BrowserUtils.promiseLayoutFlushed(this.document,
  "layout", () => viewNode.customRectGetter()));
```

Your choice, of course.

::: browser/components/extensions/ExtensionPopups.jsm
(Diff revision 3)
>  }
>  
>  class ViewPopup extends BasePopup {
>    constructor(extension, window, popupURL, browserStyle, fixedWidth, blockParser) {
>      let document = window.document;
> -

nit: Please leave this newline where it was ;)

::: browser/components/extensions/ExtensionPopups.jsm
(Diff revision 3)
>      let screenBottom = win.screen.availTop + win.screen.availHeight;
>      this.extraHeight = {
>        bottom: Math.max(0, screenBottom - popupBottom),
>        top:  Math.max(0, popupTop - win.screen.availTop),
>      };
> -

nit: Same here!

::: browser/components/extensions/ExtensionPopups.jsm:525
(Diff revision 3)
>  
>      this.browser.swapDocShells(browser);
>      this.destroyBrowser(browser);
>  
> -    if (this.dimensions && !this.fixedWidth) {
> +    if (this.dimensions) {
> +      if (this.fixedWidth) {

Oooh, well caught! Yeah the new anim with two panels showing at the same time doesn't leave any room for lazy layouting like this.
Looks sharp, but is, well, complex ;-)

        Attachment #8919871 -
        Flags: review?(mdeboer) → review+

    
    

    
  
Comment on attachment 8919871 [details]
Bug 1402845 - Fix panelview sizing when customRectGetter is used.

https://reviewboard.mozilla.org/r/190816/#review196230

::: commit-message-c6a26:1
(Diff revision 3)
> +Bug 1402845 - Fix panelview sizing when in-transition. r?mikedeboer

Please make sure your commit message contains a bit more detail before you land this patch; talk a bit about WebExtension subviews, dynamic resizing, fixed width, etc.

    
    

    
      
  
Flags: needinfo?(kmaglione+bmo)

    
    

    
  
try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56a89f9a9d32

I'm seeing test failures like: 

browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize.js | Window height should return to its original value

Holding off landing this until I investigate.

    
    

    
      
  

    
    

    
  


  
So that test failure is flagging a real issue on central/nightly, (somehow) revealed by this patch. If you have a few items in the overflow menu, and move the browser to the bottom of the screen, the menu height is truncated to only show one or 2 rows.

    
    

    
  
> So that test failure is flagging a real issue on central/nightly, (somehow)
> revealed by this patch. If you have a few items in the overflow menu, and
> move the browser to the bottom of the screen, the menu height is truncated
> to only show one or 2 rows.

When the test runs, it ends up attempting to set a negative value on this._viewStack.style.maxHeight. I can make the test failure go away by clamping the maxHeight to >= 0, but that leaves this issue. I think we should probably investigate that separately however and see where the regression occurred.

    
    

    
  
(In reply to Sam Foster [:sfoster] from comment #29)
> When the test runs, it ends up attempting to set a negative value on
> this._viewStack.style.maxHeight. 

This seems to be because alignmentPosition always returns "after_end" at http://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm, even when the window is moved to the bottom of the screen, such that the popup should (and will) open above the anchor rather than below it.

I'm not yet sure if there are conditions we can correct to ensure alignmentPosition returning the correct value here (maybe expected popup size is 0 or wrong?) or if its up to us to more correctly predict the alignment position by examinining anchorScreenBottom, anchorScreenY etc.

    
    

    
  
Mike, in case I don't make further progress on this today, does comment #30 suggest any fix to you?
Flags: needinfo?(mdeboer)

    
  
See Also:  → 1412364

    
    

    
      
  

    
    

    
  
I opened bug 1412364 for the alignmentPosition issue.

    
    

    
  
Clearing n-i, I think we've talked about this on IRC.
Flags: needinfo?(mdeboer)

    
    

    
  
Adding bug 1412364 as a dependency - we need that alignmentPosition fixed for this patch to work and not break more tests than it fixes.
Depends on: 1412364

    
    

    
  
Personas Plus also has the wrong size in the overflow menu.
[Personas Plus](https://addons.mozilla.org/en-US/firefox/addon/personas-plus/)

    
    

    
      
  

    
    

    
  
The patch in attachment 891987 has been updated now that bug 1412364 has landed. It addresses the two-step opening/resizing issue but there are still test failures[1] in browser/components/extensions/test/browser. After investigation, I think I see the problem. The tests wait for the ViewShown event from PanelMultiView before measuring the panel and proceeding. But, when first opening the panel and the mainView, ViewShown is dispatched before the popuppositioned and popupshown events - so we measure it too early. 

  hideAllViewsExcept@resource:///modules/PanelMultiView.jsm:499:18
  showSubView/this._currentShowPromise<@resource:///modules/PanelMultiView.jsm:609:11
  async*showSubView@resource:///modules/PanelMultiView.jsm:512:33
  showMainView@resource:///modules/PanelMultiView.jsm:460:14
  this.PanelMultiView<@resource:///modules/PanelMultiView.jsm:315:7
  panelmultiview_XBL_Constructor@chrome://browser/content/customizableui/panelUI.xml:46:25
  show/<@resource:///modules/CustomizableUI.jsm:4306:24
  show@resource:///modules/CustomizableUI.jsm:4303:12

I'm not sure if this is a real bug or a test deficiency. Arguably ViewShown should only ever fire after the popup is shown - as the view isn't really shown until the panel is. But I'm not yet clear on the implications of making this so. I think I'll err on the side of simplicity here and get the tests passing and open a follow-up as necessary. 

Note, for people coming here from dupes, the proposed fix at this point will only address the *height* sizing issue as the panel opens. Mismatches between the extension's content width and the panel width are more ambiguous and won't all be fixed here. 

1. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b4c0262a3dd

    
    

    
  
More updates as I'm still trying to nail this: In the attach method, not calling resizeBrowser when we have stashed good dimensions for a fixedWidth panel seems wrong now. Removing that exclusion looks to fix visible flicker and stepped-resizing.

But, still in the browser_ext_browserAction_popup_resize.js test, I'm getting a consistent failure that seems to stem from the timeout at https://dxr.mozilla.org/mozilla-central/rev/5be384bcf00191f97d32b4ac3ecd1b85ec7b18e1/browser/components/extensions/ExtensionPopups.jsm#31 - where we wait what seems like an arbitrary number of ms before going ahead and sizing the containing panel. 

Q: Is there any way this could be tied to a meaningful event like a layout flush in the extensions's content? I appreciate this is a moving target as extensions can dynamically inject and resize stuff as they load, but a constant number of ms seems especially brittle? 

There's a similar magic number in the test itself, where we wait 500ms: 

  // Wait long enough to make sure the initial resize debouncing timer has
  // expired.
  await delay(500);

If I increase this delay, the test passes with my patch. As-is, the reference window height is captured too early, and we end up failing as the restored height later in the test doesnt match. "Fixing" the test by bumping up this value doesnt seem like the right thing to do. I'm trying to understand and fix the underlying problem if possible.

    
    

    
      
  

    
    

    
  
(In reply to Sam Foster [:sfoster] from comment #40)
> More updates as I'm still trying to nail this: In the attach method, not
> calling resizeBrowser when we have stashed good dimensions for a fixedWidth
> panel seems wrong now. Removing that exclusion looks to fix visible flicker
> and stepped-resizing.
> 
> But, still in the browser_ext_browserAction_popup_resize.js test, I'm
> getting a consistent failure that seems to stem from the timeout at
> https://dxr.mozilla.org/mozilla-central/rev/
> 5be384bcf00191f97d32b4ac3ecd1b85ec7b18e1/browser/components/extensions/
> ExtensionPopups.jsm#31 - where we wait what seems like an arbitrary number
> of ms before going ahead and sizing the containing panel. 
> 
> Q: Is there any way this could be tied to a meaningful event like a layout
> flush in the extensions's content? I appreciate this is a moving target as
> extensions can dynamically inject and resize stuff as they load, but a
> constant number of ms seems especially brittle? 
> 
> There's a similar magic number in the test itself, where we wait 500ms: 
> 
>   // Wait long enough to make sure the initial resize debouncing timer has
>   // expired.
>   await delay(500);
> 
> If I increase this delay, the test passes with my patch. As-is, the
> reference window height is captured too early, and we end up failing as the
> restored height later in the test doesnt match. "Fixing" the test by bumping
> up this value doesnt seem like the right thing to do. I'm trying to
> understand and fix the underlying problem if possible.

Pretty sure I've been guilty of "just increasing the delay" in the past. Not sure if there's something better here - Kris may know?
Flags: needinfo?(kmaglione+bmo)

    
    

    
  
The last update to the patch added a (final name needed) ViewShownTransitioned event dispatch when the _transitionViews is concluded. Blocking on that in the test before measuring seems to have improved things - and makes some sense as we add height for the header in that async function. I had hoped this would eliminate the need for the delay entirely, but sadly that seems to be still necessary. 

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6c25b049d7108d2fdc34bd1bc5f9c04b26ddc8c 

Am looking into the browser_ext_browserAction_popup.js intermittents now..

    
    

    
  
Updating flags. There are existing race conditions in PanelMultiView that are making the fix for this difficult to land. Its an annoyance, but doesnt block any feature so I'm downgrading the priority and setting fix-optional for 58. Let me know if you disagree. Meanwhile I'm continuing to work on it.

          status-firefox58:
          affected → fix-optional
Priority: P1 → P3

    
  
Depends on: 1428839

    
    

    
  
(In reply to Sam Foster [:sfoster] from comment #44)
> Updating flags. There are existing race conditions in PanelMultiView that
> are making the fix for this difficult to land. Its an annoyance, but doesnt
> block any feature so I'm downgrading the priority and setting fix-optional
> for 58. Let me know if you disagree. Meanwhile I'm continuing to work on it.

It seems the intermittent test failures exacerbated by the fix for this issue boil down to this: teardown and the popuphidden handler of the PMV is currently async, and must be refactored/re-worked to be synchronous. I've filed bug 1428839 and blocked this bug on that work.

    
    

    
  
wont-fixing for 59. This is blocked on bug 1428839 which are not changes we would want to make at this point in the cycle.

          status-firefox58:
          fix-optional → wontfix

    
    

    
  


  
FWIW, NoScript is badly affected too by this bug, as shown by this user-provided screenshot.
Blocks: webext-port-noscript
Summary: When uBlock Origin button is put in the overflow menu, uBlock panel is shown with wrong size and with a "two step resizing" → When uBlock, NoScript and other browserAction buttons are in the overflow menu, their panel is shown with wrong size and with a "two step resizing"

    
    

    
  
(In reply to Giorgio Maone [:mao] from comment #47)
> Created attachment 8942451 [details]
> NoScript's UI opened from the overflow menu
> 
> FWIW, NoScript is badly affected too by this bug, as shown by this
> user-provided screenshot.

The current patch on this bug is expected to address the 2-step *height* issue only. The width issue in attachment 8942451 [details] may need to be addressed in a separate bug. The extension API is not clear on what width an extension author can expect their popup content to render in, when opened from the overflow menu vs not. Ultimately I think we'll want extension popup content to be responsively designed to fit a range of widths, as the answer is "it depends."

    
    

    
  
(In reply to Sam Foster [:sfoster] from comment #48)
> (In reply to Giorgio Maone [:mao] from comment #47)
> > Created attachment 8942451 [details]
> > NoScript's UI opened from the overflow menu
> > 
> > FWIW, NoScript is badly affected too by this bug, as shown by this
> > user-provided screenshot.
> 
> The current patch on this bug is expected to address the 2-step *height*
> issue only. The width issue in attachment 8942451 [details] may need to be
> addressed in a separate bug. The extension API is not clear on what width an
> extension author can expect their popup content to render in, when opened
> from the overflow menu vs not. Ultimately I think we'll want extension popup
> content to be responsively designed to fit a range of widths, as the answer
> is "it depends."

Specifically, the width issue is bug 1373490.

    
    

    
  
Paolo. I'm not sure if this is still relevant or even reproducible after your work on PMV?

Clearing ni on Kris.
Flags: needinfo?(kmaglione+bmo) → needinfo?(paolo.mozmail)

    
    

    
  
I think at least the part where we try to measure the header before the view is visible is still relevant, yes.

One the patches in bug 1428839 have landed, do you want to take a look at what remains to do here? I expect that the intermittent failures that prevented this from landing will be gone.
Flags: needinfo?(paolo.mozmail)

    
    

    
      
  

    
    

    
  
Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3691d1894087561799030fb181167a32b05ff39b&selectedJob=165429423

browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize.js | Panel has not shrunk from original size (-570 <= -658) is the only failure here that is concerning. I had hoped never to see that one again :/

    
    

    
  
(In reply to Sam Foster [:sfoster] from comment #53)
> Try results:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3691d1894087561799030fb181167a32b05ff39b&selectedJob=1
> 65429423
> 
> browser/components/extensions/test/browser/test-oop-extensions/
> browser_ext_browserAction_popup_resize.js | Panel has not shrunk from
> original size (-570 <= -658) is the only failure here that is concerning. I
> had hoped never to see that one again :/

What was the base for this trypush? I think bug 1193394 ran into issues there, and that's landed and stuck on m-c now, with some of the browserAction popup tests disabled on debug and/or fixed for some of the previously-extant race conditions. It's possible you'd have better luck with a new trypush based on m-c from today.
Flags: needinfo?(sfoster)

    
    

    
      
  

    
    

    
  
(In reply to :Gijs (under the weather; responses will be slow) from comment #54)

> What was the base for this trypush? I think bug 1193394 ran into issues
> there, and that's landed and stuck on m-c now, with some of the
> browserAction popup tests disabled on debug and/or fixed for some of the
> previously-extant race conditions. It's possible you'd have better luck with
> a new trypush based on m-c from today.

Yeah, that last try push would have missed the fix from bug 1193394. New push, with fingers crossed: 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4fbc8c3ce0dd842a0023e59a846be5d14b43907
Flags: needinfo?(sfoster)

    
    

    
      
  

    
    

    
  
I cleared the review flag on this as although the patch hasn't changed, the underlying code has - a lot. 

The try results from comment 56 look mostly ok? There's a couple new-to-me intermittents there, I don't think they are related or caused by this patch. Paolo, you've no doubt been looking at a lot of try runs around this area of the codebase, do those look concerning to you?

    
    

    
  
Comment on attachment 8919871 [details]
Bug 1402845 - Fix panelview sizing when customRectGetter is used.

https://reviewboard.mozilla.org/r/190816/#review231256

(In reply to Sam Foster [:sfoster] from comment #58)
> The try results from comment 56 look mostly ok?

They look good to me, thanks! I also tested the patch locally and it works quite smoothly, thanks for fixing this issue.

::: browser/components/customizableui/PanelMultiView.jsm:878
(Diff revision 11)
>        let header = viewNode.firstChild;
> +      nextPanelView.visible = true;
> +      // until the header is visible, it has 0 height. Wait for layout before measuring it
>        if (header && header.classList.contains("panel-header")) {

style nits: "let header" near the "if" statement, and keep comment within 80 characters with first letter uppercase and a final dot.

        Attachment #8919871 -
        Flags: review?(paolo.mozmail) → review+

    
    

    
      
  

    
    

    
  
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d41993e69c7e
Fix panelview sizing when customRectGetter is used. r=mikedeboer,Paolo

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/d41993e69c7e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox60:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60

    
    

    
  
Thank you for fixing this bug. Now the panel opens in a single step and with correct height.
Regarding the width issue, is there a bug that covers this argument?

    
    

    
  
(In reply to Filippo from comment #63)
> Thank you for fixing this bug. Now the panel opens in a single step and with
> correct height.
> Regarding the width issue, is there a bug that covers this argument?

Found it, sorry for the spam.

    
    

    
  
I checked yesterday on Nightly 60.0a1 (2018-03-12) and today on Nightly 61.0a1 (2018-03-12) on windows 10.

The issue is still reproducing, meaning that the uBlock Origin is opening in 2 steps, firstly with the incorrect height.

The speed of the two steps is quicker than before the fix, but it can be observed easily.

Sam, the fix should make the uBlock open in one step, is that true?
Flags: needinfo?(sfoster)

    
    

    
  
(In reply to David Olah from comment #65)
> I checked yesterday on Nightly 60.0a1 (2018-03-12) and today on Nightly
> 61.0a1 (2018-03-12) on windows 10.
> 
> The issue is still reproducing, meaning that the uBlock Origin is opening in
> 2 steps, firstly with the incorrect height.
> 
> The speed of the two steps is quicker than before the fix, but it can be
> observed easily.
> 
> Sam, the fix should make the uBlock open in one step, is that true?

Yes, that's correct. But I'm not able to reproduce this on Windows or Linux. When uBlock Origin's icon is in the overflow menu, clicking it to open the sub-menu opens it smoothly as it animates in from the right and out to the required height. Can you give me any more detail or maybe a screen capture? I wonder if the animation is janking?
Flags: needinfo?(sfoster) → needinfo?(david.olah)

    
    

    
  

      
      
      
      

        Attached file
          
        Ublock_open.swf
        — 
      

    


  
Hi Sam,

Sure I can. Here you can find the screen capture. Please reply what you think about the behaviour.
Flags: needinfo?(david.olah) → needinfo?(sfoster)

    
    

    
  
(In reply to David Olah from comment #67) 
> Sure I can. Here you can find the screen capture. Please reply what you
> think about the behavior.

Yeah, the video shows the behavior we were getting before this patch landed. I've not seen this behavior with the patch applied, so I'm not sure what is going on here - as I assume this is a recent nightly? Can you confirm the version/date? I do see a little jank the very first time the sub-menu is opened. I wonder if that jank is worse on a different machine and is what we are seeing here. Paolo, any thoughts?
Flags: needinfo?(sfoster)
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(david.olah)

    
    

    
  
Yes, I tested it on Nightly 60.0a1 (2018-03-13) when I wrote Comment 67 and I retried now on 61.0a1 (2018-03-15), of course with a new profile and no additional add-ons and it is still reproducing for me.
Flags: needinfo?(david.olah) → needinfo?(sfoster)

    
    

    
  
I managed to reproduce the bug using an older version of Nightly (2017-09-25) on Windows 10 x64.
I retested everything using latest Nightly 61.0a1 and beta 60.0b4 on Windows 10 x64 and Ubuntu 16.04 x64. There doorhanger is displayed in just one phase, but the width of the doorhanger is still wrong. 

I couldn't verify the fix on macOS 10.13 on the other hand, because for some reason on the latest Nightly the doorhanger was not displayed. Beta 60.0b4 doesn't seem to be affected at the moment.

    
    

    
  
(In reply to Oana Botisan from comment #70)
> I managed to reproduce the bug using an older version of Nightly
> (2017-09-25) on Windows 10 x64.
> I retested everything using latest Nightly 61.0a1 and beta 60.0b4 on Windows
> 10 x64 and Ubuntu 16.04 x64. There doorhanger is displayed in just one
> phase,

Sounds like this is verified fixed, though we need to understand comment #69 etc. Can you see if you can find some circumstance in which you can reproduce those issues?

> but the width of the doorhanger is still wrong. 

This is bug 1373490 / bug 1383294.

> I couldn't verify the fix on macOS 10.13 on the other hand, because for some
> reason on the latest Nightly the doorhanger was not displayed. Beta 60.0b4
> doesn't seem to be affected at the moment.

This is bug 1445787

    
    

    
  


  
I didn't managed to reproduce the bug using uBlock Origin, but I did try other add-ons and I found out that some of them open in two phases. It depends on what exactly they have to display. For example the height of Cookie AutoDelete's display is shorter when you visit a site that has no cookies, but is longer when you are on a site with cookies. That means that the doorhanger is opened with two phase resizing on a site with cookies. I attached a .gif to this comment to see exactly what I am trying to explain. 

Is this information helpful or maybe it's just an add-on issue? I tried to use top rated and popular add-ons to investigate the issue.

    
    

    
  
I also retried to reproduce the behaviour from the attachment in Comment 67 and I still manage to reproduce it on Nightly 61.0a1 (2018-03-19) (64-bit), not only on older versions of it.

    
    

    
  
If this problem is confirmed, the place to look for timing issues is likely the code that provides the height that is later used by the customRectGetter. I haven't looked at the code, but if possible the ViewShowing event may have to delay opening the view until we have a known height, or we may need an asynchronous version of the customRectGetter.
Flags: needinfo?(paolo.mozmail)

    
    

    
  
(In reply to Oana Botisan from comment #72)
> Created attachment 8960186 [details]
> doorhanger 2 phases..gif
> 
> I didn't managed to reproduce the bug using uBlock Origin, but I did try
> other add-ons and I found out that some of them open in two phases. It
> depends on what exactly they have to display. For example the height of
> Cookie AutoDelete's display is shorter when you visit a site that has no
> cookies, but is longer when you are on a site with cookies. That means that
> the doorhanger is opened with two phase resizing on a site with cookies. I
> attached a .gif to this comment to see exactly what I am trying to explain. 
> 
> Is this information helpful or maybe it's just an add-on issue? I tried to
> use top rated and popular add-ons to investigate the issue.

This looks ok to me. Yes, it could be smoother, but I don't see the symptoms of this bug in that capture. Comment 67 remains an issue though, and until I can reproduce it its going to be difficult to pursue the suggestions in Comment 74.
Flags: needinfo?(sfoster)

    
    

    
  
The issue from Comment 67 is still reproducing in Nightly 61.0a1 (2018-04-24).
When Ublock doesn't fit in the address bar and it is being opened from the More tools dropdown (as in Comment 67), it opens in two steps.

Sam, can you take a look again? What do you think we should do with this issue?
Flags: needinfo?(sfoster)

    
    

    
  
(In reply to David Olah from comment #76)
> The issue from Comment 67 is still reproducing in Nightly 61.0a1
> (2018-04-24).
> When Ublock doesn't fit in the address bar and it is being opened from the
> More tools dropdown (as in Comment 67), it opens in two steps.
> 
> Sam, can you take a look again? What do you think we should do with this
> issue?

There is a landed patch on this bug, which seems to have helped at least a little. Can you open a new bug and carry over any pertinent details & attachments?
Flags: needinfo?(sfoster)

    
  
Flags: needinfo?(david.olah)

    
    

    
  
Marking the issue as verified. It is partially fixed, for the remaining issue I created Bug 1457389
Status: RESOLVED → VERIFIED
Flags: needinfo?(david.olah)

    
  
Flags: qe-verify+
Component: Theme → Toolbars and Customization

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: