1457389 - When addon buttons are in the overflow menu, some addons with dropdowns are opening in two steps

Status: VERIFIED FIXED

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

Description

[David Olah]

[Affected versions]: 
Nightly 61.0a1

[Affected platforms]:
Platforms: Windows 10, Windows 7, Mac OS and Ubuntu 16.04 x64.

[Steps to reproduce]:
1. Open Nightly and Install an addon (that opens a dropdown menu at click, ex: uBlock Origin)
2. Minimize the browser until, in the address bar, the "More tools" button appears
3. Open the "More tools" and click the installed addon 

[Expected result]:
- The addon dropdown opens directly

[Actual result]:
- The addon dropdown opens in two steps (the height of it changes twice)

[Notes]
- Here are two videos with examples: https://www.screencast.com/t/4hqiQwLe3P9x and https://www.screencast.com/t/Mj9OFztl7
- This issue results from partially fixing Bug 1402845

Comment 1

[Itiel]

I can also reproduce this STR consistently.
I think the reason Sam and the others (me included) couldn't reproduce this before is because the addon was added to the overflow menu manually by right clicking and selecting "Pin to Overflow Menu", whereas now the addon is forcefully pinned to it by resizing the window.

Comment 2

[:Gijs (he/him)]

Sam, are you able to investigate further based on David's steps?

Comment 3

[Sam Foster [:sfoster] (he/him)]

(In reply to :Gijs (he/him) from comment #2)
> Sam, are you able to investigate further based on David's steps?

yeah I will be looking into this.

Comment 4

[Sam Foster [:sfoster] (he/him)]

We do get the correct dimension for the popup content from a Browser:Resized event in ExtensionPopup.jsm - before even attach() is called. This gets stashed in this.dimensions.
And it looks like our first call to resizeBrowser() happens before fixedWidth is set. The result is that lastCalculatedInViewHeight doesnt yet have a value when PMV calls the customRectGetter, so it receives the initial this.viewNode.boxObject.height

So one fix would be to consult the stashed dimensions in the customRectGetter. I'll get a patch together and push to try.

Comment 5

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1457389 - Check if extension is in the overflow menu when setting fixedWidth on the popup.

Review commit: https://reviewboard.mozilla.org/r/240780/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240780/

Comment 6

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8972056 [details]
Bug 1457389 - Check if extension is in the overflow menu when setting fixedWidth on the popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240780/diff/1-2/

Comment 7

[:Gijs (he/him)]

Comment on attachment 8972056 [details]
Bug 1457389 - Check if extension is in the overflow menu when setting fixedWidth on the popup.

https://reviewboard.mozilla.org/r/240780/#review246902

I don't think this is the right fix, unfortunately. It does seem to work, as does my suggestion below, though weird things still happen if you put the window closer to the bottom of the screen, so that once the overflow menu expands it doesn't fit properly anymore (I see flickering in that case).

::: browser/components/extensions/ExtensionPopups.jsm:555
(Diff revision 2)
>      this.viewNode.customRectGetter = () => {
> -      return {height: this.lastCalculatedInViewHeight || this.viewHeight};
> +      if (this.lastCalculatedInViewHeight) {
> +        return {height: this.lastCalculatedInViewHeight};
> +      }
> +      return {
> +        height: Math.max((this.dimensions && this.dimensions.height) || 0, this.viewHeight),

The issue seems to be that `lastCalculatedInViewHeight` is not defined sometimes, even though `this.dimensions` *is* defined.

In fact, looking at that code more closely, I think the root cause is that `fixedWidth` is not true for these items, even though it should be:

https://dxr.mozilla.org/mozilla-central/rev/d2a4720d1c334b64d88a51678758c27ba8f03c89/browser/components/extensions/parent/ext-browserAction.js#399-400

You can fix this there by adding another case where we set that to true:

```js
let fixedWidth =
  this.widget.areaType == CustomizableUI.TYPE_MENU_PANEL ||
  this.widget.forWindow(window).overflowed;
```

I think that'd be the better approach because it seems there are style differences as well if `fixedWidth` is true? And it's probably best if the style in these popups is aligned if they're in the overflow menu (however they came to be there).

Comment 8

[Sam Foster [:sfoster] (he/him)]

(In reply to :Gijs (he/him; out until May 8) from comment #7)
> Comment on attachment 8972056 [details]
> Bug 1457389 - Fall back to stashed dimensions in customRectGetter when
> opening extension popup.
> 
> I don't think this is the right fix, unfortunately. It does seem to work, as
> does my suggestion below, though weird things still happen if you put the
> window closer to the bottom of the screen, so that once the overflow menu
> expands it doesn't fit properly anymore (I see flickering in that case).

Is that a new regression from the patch? Will look into it. 
 
> You can fix this there by adding another case where we set that to true:
> 
> ```js
> let fixedWidth =
>   this.widget.areaType == CustomizableUI.TYPE_MENU_PANEL ||
>   this.widget.forWindow(window).overflowed;
> ```

I like it. Agreed fixedWidth should be correct and consistent. I had noticed it was incorrectly set by the time the first resizeBrowser was called, but not why.

Comment 9

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] from comment #8)
> (In reply to :Gijs (he/him; out until May 8) from comment #7)
> > Comment on attachment 8972056 [details]
> > Bug 1457389 - Fall back to stashed dimensions in customRectGetter when
> > opening extension popup.
> > 
> > I don't think this is the right fix, unfortunately. It does seem to work, as
> > does my suggestion below, though weird things still happen if you put the
> > window closer to the bottom of the screen, so that once the overflow menu
> > expands it doesn't fit properly anymore (I see flickering in that case).
> 
> Is that a new regression from the patch? Will look into it. 

I thought so at first, but then I tested some alternatives and couldn't get rid of it so I somewhat doubt it (or at least, it's not worse than the current situation). It's an edgecase, and I think when I checked the suggestion I made, it had the same issues, so I would be happy for this to land without getting that addressed (ie leave that issue for a follow-up bug).

Comment 10

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8972056 [details]
Bug 1457389 - Check if extension is in the overflow menu when setting fixedWidth on the popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240780/diff/2-3/

Comment 11

[Sam Foster [:sfoster] (he/him)]

The new patch is just Gijs' suggestion. I looked to see if there was a way to add test coverage for this. Unfortunately this two-step and the incorrect fixedWidth param seems to get lost in the debounce pause at https://searchfox.org/mozilla-central/rev/9769f2300a17d3dfbebcfb457b1244bd624275e3/browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js#190 

So, i'm open to suggestions, but suspect the effort would out-weigh the gain here?

Comment 12

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] from comment #11)
> The new patch is just Gijs' suggestion.

Passing the review on because of this - probably healthy if someone other than me reviews what was essentially my suggestion. :-)

> I looked to see if there was a way
> to add test coverage for this. Unfortunately this two-step and the incorrect
> fixedWidth param seems to get lost in the debounce pause at
> https://searchfox.org/mozilla-central/rev/
> 9769f2300a17d3dfbebcfb457b1244bd624275e3/browser/components/extensions/test/
> browser/browser_ext_browserAction_popup_resize.js#190 
> 
> So, i'm open to suggestions, but suspect the effort would out-weigh the gain
> here?

Yeah, if it's not reliably testable then maybe it's not worth adding a test specifically for this case.

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8972056 [details]
Bug 1457389 - Check if extension is in the overflow menu when setting fixedWidth on the popup.

https://reviewboard.mozilla.org/r/240780/#review251430

The change looks good and reasonable to me, especially because we do check esplicitly if the widget is in the "More tools..." area to set the fixedWidth flag and so it seems more than reasonable that we do also espicitly check if the widget is being overflowed because of the window width.

I briefly tried locally this patch and it definitely improve the behavior of the "overflow-ed" browserAction popups (which currently present a number of rendering issues, tracked by the Bug 1459046 meta bug), also while looking into the other Bug 1459046 dependencies, I noticed that Bug 1383294 comment 0 seems to describe the same issue, so now it may be a duplicate of this bugzilla issue (and fixed once this patch would land), :Gijs you reported Bug 1383294, what do you think?

I also agree that (unfortunately) testing this fix on its own seems tricky, because it is also affected by the timing of the events exchanged between the main and the extension processes, and so testing it consistently from the automated tests is not that easy (but it seems worth to have the qa verify it once landed).

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf62f7d895c4
Check if extension is in the overflow menu when setting fixedWidth on the popup. r=rpl

Comment 16

[Sam Foster [:sfoster] (he/him)]

Flagging for verification. See STR in Comment #0. You can also manually movie e.g. uBlock Origin's icon to the overflow menu via the customize screen.

Comment 17

[Stefan Hindli [:stefan_hindli]]

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

Comment 18

[David Olah]

I retested the issue on Windows 10, Windows 7, Mac OS and Ubuntu 16.04 x64, Nightly 62.0a1 (2018-05-28) and it is no more reproducing.

Marking it as Verified.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Thanks for the verification, David!

Sam, is there a strong enough user impact here that you feel we should consider this for backport or can it ride the trains?

Comment 20

[Sam Foster [:sfoster] (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> Thanks for the verification, David!
> 
> Sam, is there a strong enough user impact here that you feel we should
> consider this for backport or can it ride the trains?

No, its pretty subtle. It should just ride the trains.