1282189 - menu panel popup dynamic resize behavior is incorrect

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Hans Meyer]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.84 Safari/537.36



Actual results:

Popups opened in the menu panel currently have the same resizing behavior as toolbar-based popups, leading to excessive white space to the right and bottom of popup content that does not fill the menu panel popup's fixed area.



Expected results:

Instead, a menu panel popup's unique resize behavior should be:

- width: fix to fit width of panel. do not resize dynamically.
- height: limit to min height of menu. allow to grow dynamically.

Comment 1

[Kris Maglione [:kmag]]

This is actually meant to be handled by the current code. The problem is that the flex sizing that's meant to apply in the menu panel seems to break when we change the width properties after it's laid out.

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1282189: Improve resizing behavior for browser action views in menu panel.

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

Comment 3

[:Gijs (he/him)]

Comment on attachment 8773581 [details]
Bug 1282189: Improve resizing behavior for browser action views in menu panel.

https://reviewboard.mozilla.org/r/66294/#review63158

::: browser/components/extensions/ext-utils.js:181
(Diff revision 1)
>      // starts out smaller than 30px by 10px. This isn't an issue now, but it
>      // will be if and when we popup debugging.
>  
>      // This overrides the content's preferred size when displayed in a
> -    // fixed-size, slide-in panel.
> +    // fixed-width, slide-in panel.
>      this.browser.setAttribute("flex", "1");

Why not just remove the flex? Seems like a much simpler solution...

::: browser/components/extensions/ext-utils.js:237
(Diff revision 1)
> +      let doc = this.browser.contentDocument;
> +      if (!doc) {
> +        return;
> +      }
> +
> +      let root = doc.documentElement;

What happens if the document doesn't have a documentElement?

::: browser/components/extensions/ext-utils.js:344
(Diff revision 1)
> +    // Calculate the extra height available on the screen below the bottom of
> +    // the menu panel. Use that to calculate the maximum size that the sub-view
> +    // is allowed to grow to.
> +    let popup = win.PanelUI.panel;
> +    let extraHeight = (win.screen.height -
> +                       (win.mozInnerScreenY +
> +                        popup.getBoundingClientRect().bottom));

Why do we need to do this?

Comment 4

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/66294/#review63158

> Why not just remove the flex? Seems like a much simpler solution...

I'm not sure what that would solve. The flex is necessary.

> What happens if the document doesn't have a documentElement?

This code never runs before the load event fires. There's always a documentElement at that point. HTML documents will generate one if it's missing. XML documents will just be replaced with a parse error document.

> Why do we need to do this?

We need some way to determine a maximum height for the browser, otherwise long documents will just extend off the screen. It's hard to come up with a reasonable static value that makes sense everywhere, so going with "the space available on the screen" seems to make the most sense.

Comment 5

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #4)
> > What happens if the document doesn't have a documentElement?
> 
> This code never runs before the load event fires. There's always a
> documentElement at that point. HTML documents will generate one if it's
> missing. XML documents will just be replaced with a parse error document.

This is trivally false:

data:text/html,<script>document.documentElement.remove()</script>

I don't know what the consequences are in this case, but we should make sure that they are at least non-harmful.

Comment 6

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #4)
> > Why do we need to do this?
> 
> We need some way to determine a maximum height for the browser, otherwise
> long documents will just extend off the screen. It's hard to come up with a
> reasonable static value that makes sense everywhere, so going with "the
> space available on the screen" seems to make the most sense.

Why is long documents extending beyond the screen bad? We should just show a scrollbar, like we do for e.g. the history view. This is effectively the same thing that you're doing, except the scrollbar is inside the browser instead of outside it, and it requires a lot of extra code.

Comment 7

[Kris Maglione [:kmag]]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Kris Maglione [:kmag] from comment #4)
> > > What happens if the document doesn't have a documentElement?
> > 
> > This code never runs before the load event fires. There's always a
> > documentElement at that point. HTML documents will generate one if it's
> > missing. XML documents will just be replaced with a parse error document.
> 
> This is trivally false:
> 
> data:text/html,<script>document.documentElement.remove()</script>
> 
> I don't know what the consequences are in this case, but we should make sure
> that they are at least non-harmful.

Hm. I hadn't considered that. Probably not likely to happen in practice, but I
guess it makes sense to check.

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Kris Maglione [:kmag] from comment #4)
> > > Why do we need to do this?
> > 
> > We need some way to determine a maximum height for the browser, otherwise
> > long documents will just extend off the screen. It's hard to come up with a
> > reasonable static value that makes sense everywhere, so going with "the
> > space available on the screen" seems to make the most sense.
> 
> Why is long documents extending beyond the screen bad? We should just show a
> scrollbar, like we do for e.g. the history view. This is effectively the
> same thing that you're doing, except the scrollbar is inside the browser
> instead of outside it, and it requires a lot of extra code.

The problem is when the outer menu panel gets large enough to extend off
screen. The size restriction only applies to the height of the browser. Longer
documents scroll when they hit the max size. The panel itself just doesn't
grow larger.

I don't know what triggers the maximum height for the history view, but in
these browsers, the menu panel grows to accommodate them, whatever their
height. It's true that we could let them grow to any height and use the
scrollbar in the menu rather than in the browser, but that means that, among
other things, the document in the browser has no idea of its current scroll
state.

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8773581 [details]
Bug 1282189: Improve resizing behavior for browser action views in menu panel.

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

Comment 9

[:Gijs (he/him)]

Comment on attachment 8773581 [details]
Bug 1282189: Improve resizing behavior for browser action views in menu panel.

https://reviewboard.mozilla.org/r/66294/#review63246

::: browser/components/extensions/ext-utils.js:226
(Diff revision 2)
> +      // If we're in a fixed-width area (namely a slide-in subview of the main
> +      // menu panel), we need to calculate the view height based on the
> +      // preferred height of the document's root scrollable element at the
> +      // current width, rather than the complete preferred dimensions of the
> +      // window.

Which window/document does this refer to? The one inside the <browser> ? Could you clarify this?

::: browser/components/extensions/ext-utils.js:246
(Diff revision 2)
> +      let getHeight = elem => elem.getBoundingClientRect(elem).height;
> +
> +      let height = Math.ceil(body.scrollHeight +
> +                             // Compensate for any offsets between the scroll
> +                             // area of the body and the outer height of the
> +                             // document.
> +                             getHeight(root) - getHeight(body));
> +
> +      height = Math.min(height, this.maxHeight);
> +      this.browser.style.height = `${height}px`;

While in principle I like whitespace and comments, the contents of this `if` block contain 16 lines of actual code (and that's counting the lonely closing braces!) and yet the block stretches over 37 lines. Can we make that discrepancy slightly less extreme?

::: browser/components/extensions/ext-utils.js:252
(Diff revision 2)
> +
> +      let height = Math.ceil(body.scrollHeight +
> +                             // Compensate for any offsets between the scroll
> +                             // area of the body and the outer height of the
> +                             // document.
> +                             getHeight(root) - getHeight(body));

Move the getHeight helper up, and then compute this difference in an else for the `if` check for quirks/ lack of body (in which case this discrepancy is 0 anyway) ? Then the result can be in, say, `bodyRootHeightDiff` and this `Math.ceil` call just fits on one line and is easier to read.

::: browser/components/extensions/ext-utils.js:289
(Diff revision 2)
>  
> -    this.browser.style.width = `${width}px`;
> +      this.browser.style.width = `${width}px`;
> -    this.browser.style.height = `${height}px`;
> +      this.browser.style.height = `${height}px`;
> +    }
> +
> +    let event = new this.window.CustomEvent("PopupResized");

So, I left this comment last time, but for some reason it didn't get publicized? I don't really understand why. I've seen similar issues where comments would no longer show in the diff view, I'd leave another one, and then when I submit a review I suddenly have two comments on that line (and if I go back to the diff, remove the duplicate, the diff still indicates no more comments but the submit review page still has 1 copy of the comment). I need to work out STR for this stuff and file mozreview bugs, because it makes this process more frustrating for everybody... in any case:

Feels like the event should have a more unique name, like "WebExtBrowserViewPopupResized" or whatever.

::: browser/components/extensions/ext-utils.js:348
(Diff revision 2)
> +    let extraHeight = (win.screen.height -
> +                       (win.mozInnerScreenY +
> +                        popup.getBoundingClientRect().bottom));

This calculation will go awry if the popup opens towards the top rather than the bottom (ie if there is more space above than below the popup's anchor and the popup doesn't fit below it). We should probably also have tests for that case.

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:18
(Diff revision 2)
> +  // Debouncing code makes this a bit racy.
> +  // Try to skip the first, early resize, and catch the resize event we're
> +  // looking for, but don't wait longer than a few seconds.

This smells really buggy. What "debouncing" code is this and why do we need to do this?

Note also that the 5000ms wait means that if we actually hit that wait, this is likely to already time out on debug infra, purely by the number of calls to awaitResize multiplied by 5 seconds coming very close to the 45s cut-off for test runs.

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:84
(Diff revision 2)
> +  let docType = "";
> +  if (standardsMode) {
> +    docType = "<!DOCTYPE html>";
> +  }

Nit:
```
let docType = standardsMode ? "<!DOCTYPE html>" : "";
```

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:177
(Diff revision 2)
> +  // Wait long enough to make sure the initial resize debouncing timer has
> +  // expired.
> +  yield new Promise(resolve => setTimeout(resolve, 100));

Again, what is this? This is asking to intermittent-fail on infra.

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:182
(Diff revision 2)
> +  // Wait long enough to make sure the initial resize debouncing timer has
> +  // expired.
> +  yield new Promise(resolve => setTimeout(resolve, 100));
> +
> +  // If the browser's preferred height is smaller than the initial height of the
> +  // panel, then it will still take up the full avaiable vertical space. Even

typo

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js:243
(Diff revision 2)
> +  yield closeBrowserAction(extension);
> +
> +  yield extension.unload();
> +}
> +
> +add_task(function* testBrowserActionMenuResizeStandards() {

Could just use: testPopupSize.bind(null, true)

and ditto for false in the next add_task ?

::: browser/themes/shared/customizableui/panelUI.inc.css:280
(Diff revision 2)
> +panelview[id^=PanelUI-webext-] {
> +  overflow: hidden;
> +}

Shouldn't this only happen in the mainview/non-mainview case? If not, what is this fixing, because it seems to be different from the bug this is being fixed as part of?

Comment 10

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/66294/#review63158

> I'm not sure what that would solve. The flex is necessary.

Your earlier comment (comment #1) made it sound like the flex was causing the problem. I guess it can alternatively be read as "just the flex is not enough" / "doesn't work", but that really wasn't clear.

Comment 11

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/66294/#review63246

> Which window/document does this refer to? The one inside the <browser> ? Could you clarify this?

Yeah, the one inside the browser.

> Move the getHeight helper up, and then compute this difference in an else for the `if` check for quirks/ lack of body (in which case this discrepancy is 0 anyway) ? Then the result can be in, say, `bodyRootHeightDiff` and this `Math.ceil` call just fits on one line and is easier to read.

Yeah, they're the same element in quirks mode, so the result is always 0. But I'd rather keep the quirks/non-quirks mode as similar as possible. I like the idea moving this to a variable, though.

> This calculation will go awry if the popup opens towards the top rather than the bottom (ie if there is more space above than below the popup's anchor and the popup doesn't fit below it). We should probably also have tests for that case.

Hm. That's a good point...

> This smells really buggy. What "debouncing" code is this and why do we need to do this?
> 
> Note also that the 5000ms wait means that if we actually hit that wait, this is likely to already time out on debug infra, purely by the number of calls to awaitResize multiplied by 5 seconds coming very close to the 45s cut-off for test runs.

The dynamic resize code is currently limited to 1 resize every 100ms. Whenever we detect a change that requires a resize, we immediately resize the browser, and then schedule the next resize for 100ms later (since the first DOM change we observe isn't likely to be the one that leads to the final size). That means that we're expecting two resize events for every change we make, and the second is the one that we expect to have the final, correct size for the current state of the document.

In practice, the 5000ms wait shouldn't be an issue. I chose it because sometimes debug builds are really slow, and I wanted to avoid triggering intermittents as much as possible. But so I haven't managed a test run where the listeners fail and we fall back to the timeout, so if it happens at all, it should be a rare enough occurrence that it won't cause timeouts.

> Again, what is this? This is asking to intermittent-fail on infra.

When we first show the panel, we immediately resize it, and also immediately schedule another size calculation after the debounce interval. This timeout just makes sure we've made it past that first interval before we start any of the resizing tests. I don't think it's possible for this to cause intermittents, since by the time we get to this point, the resize timer we're waiting for will already have started. And since both timers are tied to the same browser window and have the same interval, this one will always fire after it.

> Could just use: testPopupSize.bind(null, true)
> 
> and ditto for false in the next add_task ?

I could, but I generally find having the test function name in the logs useful, so I'd prefer to leave it as-is.

> Shouldn't this only happen in the mainview/non-mainview case? If not, what is this fixing, because it seems to be different from the bug this is being fixed as part of?

There's a brief lag between the browser being resized and the panel adjusting to compensate, which briefly causes scrollbars to appear when it's enlarged. That only really causes problems in the menu panel, since we're not using a fixed-width browser there, and it therefore makes the browser slightly narrower. But it still makes sense to have this in other popups, if only to prevent a possible, brief visual glitch during resize.

Comment 12

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/66294/#review63158

> Your earlier comment (comment #1) made it sound like the flex was causing the problem. I guess it can alternatively be read as "just the flex is not enough" / "doesn't work", but that really wasn't clear.

I'll try to rewrite the comment to make it clearer.

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8773581 [details]
Bug 1282189: Improve resizing behavior for browser action views in menu panel.

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

Comment 14

[:Gijs (he/him)]

Comment on attachment 8773581 [details]
Bug 1282189: Improve resizing behavior for browser action views in menu panel.

https://reviewboard.mozilla.org/r/66294/#review63958

r=me assuming green try

Comment 15

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/66294/#review63958

Try is green, except for Linux buildbot tests, where for some reason the panel winds up with side="top" rather than side="bottom". It works locally, so I suspect it should be simple enough to fix.

Comment 16

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/7771811dec587c7bba6c92061a7046772524cb90
Bug 1282189: Improve resizing behavior for browser action views in menu panel. r=Gijs

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7771811dec58

Comment 18

[Vasilica Tamas, QA [:vtamas]]

Hans, could you please provide the webextension which reproduced the bug in order to verify this issue?