The default bug view has changed. See this FAQ.

Popup size does not respond to content change.

VERIFIED FIXED in Firefox 50

Status

()

Toolkit
WebExtensions: Untriaged
VERIFIED FIXED
2 years ago
5 months ago

People

(Reporter: mozilla, Assigned: freaktechnik, Mentored)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla50
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions -

Firefox Tracking Flags

(firefox50 verified)

Details

(Whiteboard: [good first bug][berlin]triaged)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
OS: Unspecified → Windows 8.1
Hardware: Unspecified → x86_64
(Reporter)

Updated

2 years ago
Summary: Web Extension Popup (default_popup) widnow size does not respond to content change. → Web Extension Popup (default_popup) window size does not respond to content change.
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit

Updated

a year ago
Flags: blocking-webextensions-
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → kmaglione+bmo
Blocks: 1237377

Comment 1

a year ago
The popup is appearing perfect in developer browser. But it appears too large in firefox 43.0 browsers.
Assignee: kmaglione+bmo → nobody
Mentor: kmaglione+bmo@mozilla.com
OS: Windows 8.1 → Unspecified
Hardware: x86_64 → Unspecified
Summary: Web Extension Popup (default_popup) window size does not respond to content change. → Popup size does not respond to content change.
Whiteboard: [good first bug]
Version: 44 Branch → unspecified

Updated

a year ago
Whiteboard: [good first bug] → [good first bug]triaged

Comment 2

a year ago
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
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

a year ago
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

a year ago
Hi Kris, 

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

Zim
Whiteboard: [good first bug]triaged → [good first bug][berlin]triaged

Comment 6

a year ago
(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

a year ago
Hello,

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

Talal
Doesn't look like it, go for it Talal.
Are either of you still planning to submit a patch for this? Do you have any questions about the process?

Thanks!
Flags: needinfo?(tbusta93)
Flags: needinfo?(multhazim123)

Comment 10

a year ago
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!
Flags: needinfo?(tbusta93)

Comment 11

11 months ago
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!

Updated

11 months ago
Duplicate of this bug: 1269323

Comment 13

11 months ago
Hello Kris Maglione, this bug is already fix right?
Flags: needinfo?(kmaglione+bmo)

Comment 14

11 months ago
It's not, I'm still having this issue with the WebExt build of Easy Passwords in the current nightly (2016-05-12).
Flags: needinfo?(kmaglione+bmo)

Comment 15

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

Comment 16

11 months ago
(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

11 months ago
@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.
Flags: needinfo?(tbusta93)

Updated

10 months ago
Duplicate of this bug: 1222454

Comment 19

10 months ago
Have the same issue. Firefox 50 nightly on windows 10

Updated

10 months ago
Blocks: 1253374

Updated

10 months ago
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 20

9 months ago
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.
Attachment #8763268 - Flags: review?(kmaglione+bmo)

Comment 21

9 months ago
(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 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.
Attachment #8763268 - Flags: review?(kmaglione+bmo) → review+

Updated

9 months ago
Flags: needinfo?(tbusta93)
Flags: needinfo?(multhazim123)
(Assignee)

Comment 23

9 months ago
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
Attachment #8763268 - Attachment is obsolete: true
Attachment #8764035 - Flags: review+

Updated

9 months ago
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 24

9 months ago
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
Attachment #8764035 - Attachment is obsolete: true
Attachment #8764183 - Flags: review?(kmaglione+bmo)
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!
Attachment #8764183 - Flags: review?(kmaglione+bmo) → review+

Updated

9 months ago
Assignee: nobody → martin
(Assignee)

Updated

9 months ago
Attachment #8741452 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Keywords: checkin-needed

Comment 26

9 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e96476b8d49
Resize popup when content size changes. r=kmag
Keywords: checkin-needed

Comment 27

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e96476b8d49
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Flags: qe-verify+
QA Contact: vasilica.mihasca

Comment 28

9 months ago
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.
Flags: needinfo?(martin)
(Assignee)

Comment 29

9 months ago
(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?
Flags: needinfo?(martin)

Comment 30

9 months ago
I think a follow-up bug would be ideal.

Comment 31

9 months ago
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.
(Assignee)

Comment 32

9 months ago
(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.
(Assignee)

Updated

9 months ago
See Also: → bug 1282171

Comment 33

9 months ago
> 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.

Updated

8 months ago
Duplicate of this bug: 1287045

Comment 35

8 months ago
(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.
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?
Flags: needinfo?(kmaglione+bmo)

Updated

8 months ago
See Also: → bug 1293036

Comment 37

8 months ago
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.

Updated

8 months ago
See Also: → bug 1294440

Updated

8 months ago
See Also: → bug 1294442

Comment 38

8 months ago
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.
Flags: needinfo?(kmaglione+bmo)

Updated

7 months ago
Depends on: 1282189

Updated

7 months ago
Keywords: dev-doc-needed
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.
Flags: needinfo?(kmaglione+bmo)
Yeah, looks good to me. We should link to that from the relevant parts of the browserAction/pageAction documentation, though.
Flags: needinfo?(kmaglione+bmo)
Keywords: dev-doc-needed → dev-doc-complete
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?
Flags: needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1309143
(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.
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
Flags: qe-verify+
Depends on: 1310947
You need to log in before you can comment on or make changes to this bug.