Last Comment Bug 1215025 - Popup size does not respond to content change.
: Popup size does not respond to content change.
Status: VERIFIED FIXED
[good first bug][berlin]triaged
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: WebExtensions: Untriaged (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal with 5 votes (vote)
: mozilla50
Assigned To: Martin Giger [:freaktechnik]
: Vasilica Mihasca, QA [:vasilica_mihasca]
: Andy McKay [:andym]
Mentors: Kris Maglione [:kmag]
: 1222454 1269323 1287045 (view as bug list)
Depends on: 1309143 1282189 1310947
Blocks: webext-popups webext-port-blackmenu
  Show dependency treegraph
 
Reported: 2015-10-15 00:54 PDT by mozilla
Modified: 2016-10-18 01:45 PDT (History)
28 users (show)
amckay: blocking‑webextensions-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
1215025.patch (1.45 KB, patch)
2016-04-14 11:25 PDT, Talal Bustami
no flags Details | Diff | Splinter Review
bug1215025-v1.patch (8.24 KB, patch)
2016-06-17 10:04 PDT, Martin Giger [:freaktechnik]
kmaglione+bmo: review+
Details | Diff | Splinter Review
bug1215025-v2.patch (7.91 KB, patch)
2016-06-21 13:40 PDT, Martin Giger [:freaktechnik]
martin: review+
Details | Diff | Splinter Review
bug1215025-v3.patch (7.95 KB, patch)
2016-06-22 02:55 PDT, Martin Giger [:freaktechnik]
kmaglione+bmo: review+
Details | Diff | Splinter Review

Description User image mozilla 2015-10-15 00:54:13 PDT
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 User image mamta 2016-01-07 05:19:00 PST
The popup is appearing perfect in developer browser. But it appears too large in firefox 43.0 browsers.
Comment 2 User image multhazim123 2016-02-22 11:04:09 PST
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 User image Kris Maglione [:kmag] 2016-02-22 11:42:06 PST
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 User image Talal Bustami 2016-02-24 08:29:39 PST
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 User image multhazim123 2016-02-24 08:35:01 PST
Hi Kris, 

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

Zim
Comment 6 User image drhen123 2016-03-24 03:40:11 PDT
(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 User image Talal Bustami 2016-04-07 15:14:28 PDT
Hello,

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

Talal
Comment 8 User image Andy McKay [:andym] 2016-04-08 07:08:06 PDT
Doesn't look like it, go for it Talal.
Comment 9 User image Kris Maglione [:kmag] 2016-04-14 11:10:32 PDT
Are either of you still planning to submit a patch for this? Do you have any questions about the process?

Thanks!
Comment 10 User image Talal Bustami 2016-04-14 11:25:06 PDT
Created attachment 8741452 [details] [diff] [review]
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 User image Kris Maglione [:kmag] 2016-04-16 14:10:31 PDT
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 12 User image Kris Maglione [:kmag] 2016-05-02 09:56:23 PDT
*** Bug 1269323 has been marked as a duplicate of this bug. ***
Comment 13 User image Dinarte Jesus [:jdj] 2016-05-12 11:15:14 PDT
Hello Kris Maglione, this bug is already fix right?
Comment 14 User image Wladimir Palant 2016-05-12 11:18:33 PDT
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 User image Maciej Sitarz 2016-05-15 13:21:05 PDT
I confirm, the issue is still present.
Checked on firefox-nightly-49.0a1.20160515 with add-on I'm porting from Chrome.
Comment 16 User image drhen123 2016-05-15 14:53:41 PDT
(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 User image Maciej Sitarz 2016-05-16 01:18:15 PDT
@drhen123@googlemail.com: 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 18 User image Kris Maglione [:kmag] 2016-05-16 09:54:49 PDT
*** Bug 1222454 has been marked as a duplicate of this bug. ***
Comment 19 User image mail 2016-06-11 04:30:28 PDT
Have the same issue. Firefox 50 nightly on windows 10
Comment 20 User image Martin Giger [:freaktechnik] 2016-06-17 10:04:51 PDT
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.
Comment 21 User image drhen123 2016-06-21 00:55:47 PDT
(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 User image Kris Maglione [:kmag] 2016-06-21 03:10:28 PDT
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 User image Martin Giger [:freaktechnik] 2016-06-21 13:40:20 PDT
Created attachment 8764035 [details] [diff] [review]
bug1215025-v2.patch

Addressed the nits and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b48b37d427ca
Comment 24 User image Martin Giger [:freaktechnik] 2016-06-22 02:55:08 PDT
Created attachment 8764183 [details] [diff] [review]
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 User image Kris Maglione [:kmag] 2016-06-22 02:58:01 PDT
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 User image Pulsebot 2016-06-22 20:36:10 PDT
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e96476b8d49
Resize popup when content size changes. r=kmag
Comment 27 User image Carsten Book [:Tomcat] 2016-06-23 05:59:49 PDT
https://hg.mozilla.org/mozilla-central/rev/6e96476b8d49
Comment 28 User image Andrew Swan [:aswan] 2016-06-24 13:52:42 PDT
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 User image Martin Giger [:freaktechnik] 2016-06-24 14:04:03 PDT
(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 User image Andrew Swan [:aswan] 2016-06-24 14:29:48 PDT
I think a follow-up bug would be ideal.
Comment 31 User image Hans Meyer 2016-06-24 15:01:07 PDT
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 User image Martin Giger [:freaktechnik] 2016-06-24 15:13:27 PDT
(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 User image Hans Meyer 2016-06-24 18:14:24 PDT
> 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 34 User image Kris Maglione [:kmag] 2016-07-15 09:35:57 PDT
*** Bug 1287045 has been marked as a duplicate of this bug. ***
Comment 35 User image oleksii.levzhynskyi 2016-07-26 06:56:49 PDT
(In reply to Pulsebot from comment #26)
> Pushed by ryanvm@gmail.com:
> 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 User image Vasilica Mihasca, QA [:vasilica_mihasca] 2016-07-27 08:18:45 PDT
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 User image Wladimir Palant 2016-08-11 07:25:02 PDT
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 User image Wladimir Palant 2016-08-11 07:54:28 PDT
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 User image Will Bamberg [:wbamberg] 2016-10-04 14:21:35 PDT
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 User image Kris Maglione [:kmag] 2016-10-06 11:22:16 PDT
Yeah, looks good to me. We should link to that from the relevant parts of the browserAction/pageAction documentation, though.
Comment 41 User image Vasilica Mihasca, QA [:vasilica_mihasca] 2016-10-10 08:41:33 PDT
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 User image Kris Maglione [:kmag] 2016-10-10 09:50:39 PDT
(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 User image Vasilica Mihasca, QA [:vasilica_mihasca] 2016-10-11 01:47:29 PDT
(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.

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