1215025 - Popup size does not respond to content change.

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[mozilla]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36

Steps to reproduce:

I am attempting to port my web extension from Chrome to Firefox (Nightly 44.0a1), I have defined in the manifest.json a default_popup of popup.html.






Actual results:

When clicking on the extensions icon the popup appears as expected, however when the content of the popup.html changes size (in this case increases in width via JavaScript embedded in the page) the actual popup window does not resize but instead displays scroll bars.

It appears the popup window is fixed at its initial size and does not respond to content changes.


Expected results:

In chrome the popup window dynamically changes (grows / shrinks) to suit changes to the content up to a maximum size of 800 x 600 px.

Comment 1

[mamta]

The popup is appearing perfect in developer browser. But it appears too large in firefox 43.0 browsers.

Comment 2

[multhazim123]

Hi, 

I'm a student at University of Toronto's CSC302 Course. I'd like to work on this project and submit a patch by the 26th. 

Zim

Comment 3

[Kris Maglione [:kmag]]

Hello.

If you haven't built Firefox before, that's the place to start. The simplest way to do that is explained here:

https://developer.mozilla.org/en-US/docs/Artifact_builds

Once you've got a working build, you can start changing code. The relevant code is here:

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-utils.js#133

The changes should be pretty straightforward. You just need to add an event listener, to check when the scrolled area changes, and then call `resizeBrowser` to resize the popup.

The relevant event is "MozScrolledAreaChanged". You should add the event listener at the end of the `createBrowser` method, along with the other listeners, and remove it in the `destroy` method, along with the other listeners.

The `handleEvent` method gets called when the event is fired, so that's where you'd need to call `this.resizeBrowser()` from.

Comment 4

[Talal Bustami]

Hello, I would like to work on this bug and I plan to submit it in the next coming days (most probably Feb. 26th). I will look into the relevant code and try to apply the changes listed above. Please let me know if there are any updates or recent information which I need to be aware of. Thank you!

Comment 5

[multhazim123]

Hi Kris, 

Thanks for the info. I'm almost done with the fix, i'll upload a patch soon. 

Zim

Comment 6

[drhen123]

(In reply to multhazim123 from comment #5)
> Hi Kris, 
> 
> Thanks for the info. I'm almost done with the fix, i'll upload a patch soon. 
> 
> Zim

Has this been submitted yet the problem still seems to persist?

Comment 7

[Talal Bustami]

Hello,

I just wanted to check whether someone has submitted a patch as I do have a fix ready.

Talal

Comment 8

[Andy McKay]

Doesn't look like it, go for it Talal.

Comment 9

[Kris Maglione [:kmag]]

Are either of you still planning to submit a patch for this? Do you have any questions about the process?

Thanks!

Comment 10

[Talal Bustami]

Attachment: 1215025.patch

Ran automated tests, no failures. Tested using web extension attached to linked bug 1222454. The extension successfully responded to change in popup size.

Please let me know if there is anything else needed for this bug/patch.

Thanks!

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8741452 [details] [diff] [review]
1215025.patch

Review of attachment 8741452 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

In the future, you should add the review? flag to patches, so someone knows to look at them.

This is definitely a good start. We just need some basic tests to go with it. They should probably go in this file:

https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_pageAction_popup.js

And they should probably work something like the resizing tests for add-on options pages:

https://dxr.mozilla.org/mozilla-central/rev/1da1937a9e03154ae7c60089f2dcf5ad9ee20fa3/toolkit/mozapps/extensions/test/browser/browser_inlinesettings_browser.js#111

Our test system has some quirks that take some getting used to, so let me know if you need any help.

Thanks!

Comment 13

[Dinarte Jesus [:jdj]]

Hello Kris Maglione, this bug is already fix right?

Comment 14

[Wladimir Palant]

It's not, I'm still having this issue with the WebExt build of Easy Passwords in the current nightly (2016-05-12).

Comment 15

[Maciej Sitarz]

I confirm, the issue is still present.
Checked on firefox-nightly-49.0a1.20160515 with add-on I'm porting from Chrome.

Comment 16

[drhen123]

(In reply to Maciej Sitarz from comment #15)
> I confirm, the issue is still present.
> Checked on firefox-nightly-49.0a1.20160515 with add-on I'm porting from
> Chrome.

Can confirm this.

I thought a patch was written for this bug and subbmitted?

Comment 17

[Maciej Sitarz]

@[e-mail]: Looks like the patch did not make it to the repo, as there are some automated test missing. Hopefully Talal will provide those tests with the patch.

Comment 19

[mail]

Have the same issue. Firefox 50 nightly on windows 10

Comment 20

[Martin Giger [:freaktechnik]]

Attachment: bug1215025-v1.patch

Based on the previous patch, but includes tests for browserAction and pageAction and respects the content getting smaller.

Comment 21

[drhen123]

(In reply to Martin Giger [:freaktechnik] from comment #20)
> Created attachment 8763268 [details] [diff] [review]
> bug1215025-v1.patch
> 
> Based on the previous patch, but includes tests for browserAction and
> pageAction and respects the content getting smaller.

In terms of workability i can confirm that this does fix the issue with the patch in the latest nightly update.

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8763268 [details] [diff] [review]
bug1215025-v1.patch

Review of attachment 8763268 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, aside from a few nits. Thanks!

::: browser/components/extensions/ext-utils.js
@@ +260,4 @@
>          // that we calculate the size after the entire event cycle has completed
>          // (unless someone spins the event loop, anyway), and hopefully after
>          // the content has made any modifications.
> +        this.window.setTimeout(() => this._resizeBrowser(), 0);

Let's change this to:

    Promise.resolve().then(() => {
      this.resizeBrowser();
    });

::: browser/components/extensions/test/browser/browser_ext_browserAction_popup_resize.js
@@ +21,5 @@
> +  clickBrowserAction(extension, window);
> +
> +  let {target: panelDocument} = yield BrowserTestUtils.waitForEvent(document, "load", true, (event) => {
> +    info(`Loaded ${event.target.location}`);
> +    return event.target.location && event.target.location.toString().endsWith("popup.html");

s/.toString()/.href/

::: browser/components/extensions/test/browser/browser_ext_pageAction_popup_resize.js
@@ +16,5 @@
> +
> +    files: {
> +      "popup.html": "<html><head><meta charset=\"utf-8\"></head></html>",
> +      "data/background.html": "<html><head><meta charset=\"utf-8\"><script src=\"background.js\"></script></head></html>",
> +      "data/background.js": function() {

No need for `data/background.js` and `data/background.html`. Just add a `background` property to the top-level object.

@@ +32,5 @@
> +    extension.onMessage("action-shown", resolve);
> +  });
> +
> +  yield extension.startup();
> +  yield actionShown;

No need for `actionShown`. Just `yield extension.awaitMessage("action-shown")`

@@ +74,5 @@
> +
> +  yield extension.unload();
> +});
> +
> +add_task(forceGC);

This shouldn't be necessary in either of these tests.

Comment 23

[Martin Giger [:freaktechnik]]

Attachment: bug1215025-v2.patch

Addressed the nits and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b48b37d427ca

Comment 24

[Martin Giger [:freaktechnik]]

Attachment: bug1215025-v3.patch

The panel for browserAction was not closed and broke a following test. This patch closes the panel at the end of the test.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c0b5f0a87a2

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8764183 [details] [diff] [review]
bug1215025-v3.patch

Review of attachment 8764183 [details] [diff] [review]:
-----------------------------------------------------------------

No need to re-request review after addressing nits when you already have an r+. You can just set the r+ yourself.

Thanks!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e96476b8d49
Resize popup when content size changes. r=kmag

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e96476b8d49

Comment 28

[Andrew Swan [:aswan]]

I've been seeing a bunch of these errors in try runs since this landed:

 09:55:56 INFO - JavaScript error: chrome://browser/content/ext-utils.js, line 326: ReferenceError: setTimeout is not defined 

I think you need to either import Timer.jsm or get setTimeout from the window object.

Comment 29

[Martin Giger [:freaktechnik]]

(In reply to Andrew Swan [:aswan] from comment #28)
> I've been seeing a bunch of these errors in try runs since this landed:
> 
>  09:55:56 INFO - JavaScript error: chrome://browser/content/ext-utils.js,
> line 326: ReferenceError: setTimeout is not defined 
> 
> I think you need to either import Timer.jsm or get setTimeout from the
> window object.

Right, I totally overlooked that and hadn't noticed in my try run. It should be this.window.setTimeout, like it was in the load event listener. Would this need a follow-up bug, or should I attach a fix to this bug?

Comment 30

[Andrew Swan [:aswan]]

I think a follow-up bug would be ideal.

Comment 31

[Hans Meyer]

This patch seems to have broken rendering for MENU PANEL based popups, which should not be subject to dynamic resizing. 

Eg, the body tag should span the (static) width of the menu panel popup by default.

Comment 32

[Martin Giger [:freaktechnik]]

(In reply to Hans Meyer from comment #31)
> This patch seems to have broken rendering for MENU PANEL based popups, which
> should not be subject to dynamic resizing. 
> 
> Eg, the body tag should span the (static) width of the menu panel popup by
> default.

I would think it should still have some resizing, just not the width and only adjust the height with a min-height specific to the menu panel.

However it has already been possible before this patch to do resizing that shouldn't happen, it's just way easier to create, as the size is not just calculated on load. The patch did not touch any of the actual resizing mechanics, just added more reasons to run the resize algorithm.

Comment 33

[Hans Meyer]

> I would think it should still have some resizing, just not the width and
> only adjust the height with a min-height specific to the menu panel.

That's a good point.
 
> However it has already been possible before this patch to do resizing that
> shouldn't happen, it's just way easier to create, as the size is not just
> calculated on load. The patch did not touch any of the actual resizing
> mechanics, just added more reasons to run the resize algorithm.

I've created bug 1282189 to address the menu panel resizing behavior.

Comment 35

[oleksii.levzhynskyi]

(In reply to Pulsebot from comment #26)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6e96476b8d49
> Resize popup when content size changes. r=kmag

Hey, Martin. Is it possible to merge this patch to upcoming FF48? According to the release schedule, version 50 would be released in November. This issue blocks us from migrating chrome extension to Firefox.

Comment 36

[Vasilica Tamas, QA [:vtamas]]

Verified this issue on Firefox 50.01 (2016-07-27) under Windows 10 64-bit and the popup issue seems to be fixed for several webextensions (e.g https://addons-dev.allizom.org/en-US/firefox/addon/test-extension/ , https://addons-dev.allizom.org/en-US/firefox/addon/eyecare-protect-your-vision/ , https://addons-dev.allizom.org/en-US/firefox/addon/google-scolar-button-ica-test/ ), but I’ve noticed a few potential isseus for some webextensions:

 - UI glitches while expanding the panel for https://addons-dev.allizom.org/en-US/firefox/addon/bubble-cursor1/  , see video http://screencast.com/t/WHRNREEI

 - The content is incorrectly displayed for https://addons-dev.allizom.org/en-US/firefox/addon/vsecure/ , see screenshot: http://screencast.com/t/gvgZugFIVPaO

 - Unnecessary scrollbars appear for  https://addons-dev.allizom.org/en-US/firefox/addon/quick-vat-calculator-ica-test/ , see screenshot: http://i.imgur.com/wfuixtB.jpg
 
 - the panel scrollbars do not work properly for https://addons-dev.allizom.org/en-US/firefox/addon/gemoji-chrome/


Kris, any thoughts about these issues? Should I file separate bugs for each of them?

Comment 37

[Wladimir Palant]

Indeed, when testing this with the WebExt build of Easy Passwords on Mac OS X (retina, meaning window.devicePixelRatio == 2) I see very broken behavior in the current nightly. The problem isn't recognizing changes, it's rather docShell.contentViewer.getContentSize() returning nonsensical results. Height is correct but the width returned there is way too low.

I tried logging from the extension:

> console.error([
>   document.documentElement.scrollWidth,
>   Math.min(document.documentElement.offsetHeight, document.documentElement.scrollHeight)
>]);

At the same time I would log width and height variables calculated in BasePopup._resizeBrowser() with the debugger. What I got:

> 600,171 <=> 291.5,171
> 600,211 <=> 328.5,211
> 600,278 <=> 328.5,278

The document element has min-width:600px; style here so the width reported from the extension is correct. docShell.contentViewer.getContentSize() seems to ignore that CSS rule - it still produces the same result if I remove it, only that it's correct then. It seems that moving min-width rule to the body element is the work-around here, automatic resizing seems to work mostly correctly for me then.

Comment 38

[Wladimir Palant]

I filed bug 1294440 and bug 1294442 on the remaining popup sizing issues I found, these might be causing the issues listed in comment 36 - or there might be more bugs of course.

Comment 39

[Will Bamberg [:wbamberg]]

Kris, I decided to split out a separate page on UI components, instead of trying to crowbar this into "Anatomy of a WebExtension". So now there's this:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/User_interface_components
that has a section on popup sizing:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/User_interface_components#Popup_resizing

Let me know if this covers it.

Comment 40

[Kris Maglione [:kmag]]

Yeah, looks good to me. We should link to that from the relevant parts of the browserAction/pageAction documentation, though.

Comment 41

[Vasilica Tamas, QA [:vtamas]]

Tested again all the webextensions mentioned in Comment 36, and I concluded that only Bubble Cursor (https://addons-dev.allizom.org/en-US/firefox/addon/bubble-cursor1/) modifies its content: http://screencast.com/t/87rNBwyk (the other ones are incorrectly displayed from the beginning).

Kris, do you have any idea if this is a webextension specific bug or should I file a new one for the panel opening issue?

Comment 42

[Kris Maglione [:kmag]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #36)
>  - UI glitches while expanding the panel for
> https://addons-dev.allizom.org/en-US/firefox/addon/bubble-cursor1/  , see
> video http://screencast.com/t/WHRNREEI

This is expected, at least for now. We only update the panel size 10 times per
second, at most, so we don't support these kinds of animations.

>  - The content is incorrectly displayed for
> https://addons-dev.allizom.org/en-US/firefox/addon/vsecure/ , see
> screenshot: http://screencast.com/t/gvgZugFIVPaO

This is a bug in the add-on. It specifies a min-width on the root <html>
element, but it needs to be specified on the <body> instead.

>  - Unnecessary scrollbars appear for 
> https://addons-dev.allizom.org/en-US/firefox/addon/quick-vat-calculator-ica-
> test/ , see screenshot: http://i.imgur.com/wfuixtB.jpg

This is happening because of the margin of the <div> inside the body and, as
far as I can tell, the effect of CSS margin collapsing on size calculation.
This is technically probably a bug in the layout code, but it's also probably
worth fixing.

>  - the panel scrollbars do not work properly for
> https://addons-dev.allizom.org/en-US/firefox/addon/gemoji-chrome/

This is an issue with the extension. The visible scrollbars are for one part
of the page, but there are additional scrollbars for another part of the page
hidden beneath them.

Comment 43

[Vasilica Tamas, QA [:vtamas]]

(In reply to Kris Maglione [:kmag] from comment #42)

Thanks Kris for the detailed reply!  

> This is happening because of the margin of the <div> inside the body and, as
> far as I can tell, the effect of CSS margin collapsing on size calculation.
> This is technically probably a bug in the layout code, but it's also probably
> worth fixing.

Filed Bug 1309143 for this.

Verified again on Firefox 52.0a1 (2016-10-10), Firefox 51.0a2 (2016-10-10), Firefox 50.0b5 (20161005190701) under Windows 10 64-bit and Ubuntu 14.04 64-bit and based on this testing and the previous comments(Comment 36 and Comment 42) I am marking this bug as verified since the other issue is tracked separately.

Comment 44

[Paul Marks]

This doesn't yet work correctly for "tall" content.  It gets both vertical and horizontal scrollbars, instead of just vertical: https://bugzilla.mozilla.org/show_bug.cgi?id=1395025