Closed Bug 1215025 Opened 9 years ago Closed 8 years ago

Popup size does not respond to content change.

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox50 verified)

VERIFIED FIXED
mozilla50
Tracking Status
firefox50 --- verified

People

(Reporter: mozilla, Assigned: freaktechnik, Mentored)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [good first bug][berlin]triaged)

Attachments

(1 file, 3 obsolete files)

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.
OS: Unspecified → Windows 8.1
Hardware: Unspecified → x86_64
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
Flags: blocking-webextensions-
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → kmaglione+bmo
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
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
Whiteboard: [good first bug] → [good first bug]triaged
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.
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!
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
(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?
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)
Attached patch 1215025.patch (obsolete) — Splinter Review
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 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!
Hello Kris Maglione, this bug is already fix right?
Flags: needinfo?(kmaglione+bmo)
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)
I confirm, the issue is still present.
Checked on firefox-nightly-49.0a1.20160515 with add-on I'm porting from Chrome.
(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?
@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)
Have the same issue. Firefox 50 nightly on windows 10
Flags: needinfo?(ntim.bugs)
Attached patch bug1215025-v1.patch (obsolete) — Splinter Review
Based on the previous patch, but includes tests for browserAction and pageAction and respects the content getting smaller.
Attachment #8763268 - Flags: review?(kmaglione+bmo)
(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+
Flags: needinfo?(tbusta93)
Flags: needinfo?(multhazim123)
Attached patch bug1215025-v2.patch (obsolete) — Splinter Review
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+
Flags: needinfo?(ntim.bugs)
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+
Assignee: nobody → martin
Attachment #8741452 - Attachment is obsolete: true
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/6e96476b8d49
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Flags: qe-verify+
QA Contact: vasilica.mihasca
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)
(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)
I think a follow-up bug would be ideal.
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.
(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.
See Also: → 1282171
> 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.
(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)
See Also: → 1293036
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.
See Also: → 1294440
See Also: → 1294442
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)
Depends on: 1282189
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)
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
Flags: qe-verify+
Depends on: 1310947
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
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: