Closed Bug 1269081 Opened 5 years ago Closed 5 years ago

Pop up display problem

Categories

(Firefox :: Extension Compatibility, defect)

48 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 49
Tracking Status
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 --- verified
firefox49 --- verified

People

(Reporter: ameenvengara, Assigned: bwinton)

References

Details

(Keywords: regression)

Attachments

(3 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160429004052

Steps to reproduce:

I just load my add-on through about:debugging window with load temporary add-on option. 


Actual results:

The problem is with  Firefox developer edition 48.0a2...... I was developing an extension which is a text editor pop up for Malayalam Unicode typing. It was working in a very good manner, until the last update of Firefox. After Firefox update the whole pop up settings ruined. But it works as a web page, but not as a pop up. The same extension works in Mozilla Firefox 45.0.2 Normal edition without any problem. 


Expected results:

The pop up should work like in Firefox 45.0.2. That is what I need. Even the CSS is not working correctly in the pop up.
Please see the attached pdf file. I have added the screenshots and description
Please provide the add-on.
Component: Untriaged → Extension Compatibility
Flags: needinfo?(ameenvengara)
It is an .xpi file. I can't attach it. Where will I upload the addon?
You should be able to attach it using https://bugzilla.mozilla.org/attachment.cgi?bugid=1269081&action=enter, or any cloud storage service, or https://addons.mozilla.org/ if it has.
I have attached. Please check above :) .xpi didn't uploaded. So renamed to .zip.
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cdea1598a698b6ba2024ad8c77a093800a6d80ae&tochange=4f41a072c0ca0523a9e211c521c21190d3b6dc4d
Blocks: 1225633
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
Flags: needinfo?(ameenvengara)
Keywords: regression
So, to fix this for now, you can add "body { display:block }" to your css file, but I'm starting to agree that we should probably do something better by default.
As I mentioned above, I'm starting to be convinced that we maybe shouldn't style popups differently from HTML pages by default, since it seems to be surprising add-on authors, and makes it hard to figure out what your page is going to look like in the browser.  Stephen, Kris: What do you think about having the new styles only apply when people use `<body class='browser-panel'>`?  (It should be a pretty quick patch to write, and would let us revert the change in bug 1263709 to get better buttons…)
Flags: needinfo?(shorlander)
Flags: needinfo?(kmaglione+bmo)
body { display: block } solves the buttons' line up problem. But my JavaScript transliteration doesn't work in pop up mode. It works in webpage. And in Firefox 45.0.2 no problem at all.
That sounds like a different problem, and unlikely to be caused by the CSS…  Do you see any errors in the Browser Console when you debug your add-on?  (Try going to "about:debugging".)
Errors from Browser Console
------------------------------
Content Security Policy: The page's settings blocked the loading of a resource at self ("script-src moz-extension://b20e4da8-45b8-4fdd-9efa-6b81f631dfce").

Error: call to eval() blocked by CSP
choose_lang.js:190:22
The object cannot be linked to the inspector without a toolbox
console-output.js:3266
The object cannot be linked to the inspector without a toolbox
Problem gets from file choose_lang.js, line number 190 

conversionHash = eval("("+opr+")");

add-on has attached above
(Kris?  Does the CSP stuff sound similar to something you've run into?)
(Stephen: I'm assuming ui-r=you, since this just reverts the look back to how it was in bug 1225633 when the add-on author adds `class="panel"`, or the default styles when they don't.)
(In reply to Blake Winton (:bwinton) (:☕️) from comment #16)
> (Stephen: I'm assuming ui-r=you, since this just reverts the look back to
> how it was in bug 1225633 when the add-on author adds `class="panel"`, or
> the default styles when they don't.)

WFM
Flags: needinfo?(shorlander)
Assignee: nobody → bwinton
(In reply to ameenvengara from comment #13)
> Problem gets from file choose_lang.js, line number 190 
> 
> conversionHash = eval("("+opr+")");
> 
> add-on has attached above

(In reply to Blake Winton (:bwinton) (:☕️) from comment #16)
> (Stephen: I'm assuming ui-r=you, since this just reverts the look back to
> how it was in bug 1225633 when the add-on author adds `class="panel"`, or
> the default styles when they don't.)

Yes. The current CSP policy blocks eval in extension documents. It should be simple enough to change your code so that it doesn't need it.

(In reply to Blake Winton (:bwinton) (:☕️) from comment #9)
> As I mentioned above, I'm starting to be convinced that we maybe shouldn't
> style popups differently from HTML pages by default, since it seems to be
> surprising add-on authors, and makes it hard to figure out what your page is
> going to look like in the browser.  Stephen, Kris: What do you think about
> having the new styles only apply when people use `<body
> class='browser-panel'>`?  (It should be a pretty quick patch to write, and
> would let us revert the change in bug 1263709 to get better buttons…)

I'm not sure. I think that if we're going to include these styles available, we should do it in a way that makes their existence more obvious to authors. Chrome has a feature like this for options_ui pages, and adds a "chrome_style" manifest key. We should probably use a similar solution here ("browser_style"), and if we're going to default to not applying the styles, then we should probably emit a console warning when the manifest doesn't specify the key at all.

Anyway, I think this is a separate issue than is being reported in this bug.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8747724 [details]
MozReview Request: Bug 1269081 - Only apply Firefox's default styles if the add-on author sets "browser_style" to true in the manifest. r=kmag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50013/diff/1-2/
Attachment #8747724 - Attachment description: MozReview Request: Bug 1269081 - Only apply Firefox's default styles if the add-on author adds a class of "panel" to the body. r=kmag. → MozReview Request: Bug 1269081 - Only apply Firefox's default styles if the add-on author sets "browser_style" to true in the manifest. r=kmag.
(I didn't emit the console warning, because I'm not sure how to do that, and I need to fix the tests, but I figured I'ld post this version to see if it was close to what you were thinking…  :)
Comment on attachment 8747724 [details]
MozReview Request: Bug 1269081 - Only apply Firefox's default styles if the add-on author sets "browser_style" to true in the manifest. r=kmag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50013/diff/2-3/
Attachment #8747724 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8747724 [details]
MozReview Request: Bug 1269081 - Only apply Firefox's default styles if the add-on author sets "browser_style" to true in the manifest. r=kmag.

https://reviewboard.mozilla.org/r/50013/#review47069

::: browser/components/extensions/ext-browserAction.js:39
(Diff revision 3)
>      title: options.default_title || extension.name,
>      badgeText: "",
>      badgeBackgroundColor: null,
>      icon: IconDetails.normalize({path: options.default_icon}, extension),
>      popup: options.default_popup || "",
> +    browserStyle: options.browser_style || false,

This isn't a per-tab setting, so it should be on the BrowserAction object rather than in the defaults.

::: browser/components/extensions/ext-pageAction.js:27
(Diff revision 3)
>    this.defaults = {
>      show: false,
>      title: options.default_title || extension.name,
>      icon: IconDetails.normalize({path: options.default_icon}, extension),
>      popup: options.default_popup || "",
> +    browserStyle: options.browser_style || false,

Same here. Not tab-specific, so store it on the PageAction instance directly.

::: browser/components/extensions/ext-utils.js:216
(Diff revision 3)
>        case this.DESTROY_EVENT:
>          this.destroy();
>          break;
>  
>        case "DOMWindowCreated":
> -        if (event.target === this.browser.contentDocument) {
> +        if (event.target === this.browser.contentDocument && this.browserStyle) {

Nit: checking `this.browserStyle` is much cheaper than the other test, so please do it first.

::: browser/components/extensions/test/browser/browser_ext_pageAction_popup.js:22
(Diff revision 3)
>  
>      files: {
>        "popup-a.html": scriptPage("popup-a.js"),
>        "popup-a.js": function() {
> +        window.onload = () => {
> +          if (window.getComputedStyle(document.body).backgroundColor == "transparent") {

We can just do `browser.test.assertEq("transparent", ...)` here, and unconditionally send the "from-popup-a".
Comment on attachment 8747724 [details]
MozReview Request: Bug 1269081 - Only apply Firefox's default styles if the add-on author sets "browser_style" to true in the manifest. r=kmag.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50013/diff/3-4/
https://reviewboard.mozilla.org/r/50013/#review47069

> We can just do `browser.test.assertEq("transparent", ...)` here, and unconditionally send the "from-popup-a".

(That's pretty awesome, and cleans up the test quite a bit!  :D  )
https://hg.mozilla.org/mozilla-central/rev/f9054ecf5f24
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment on attachment 8747724 [details]
MozReview Request: Bug 1269081 - Only apply Firefox's default styles if the add-on author sets "browser_style" to true in the manifest. r=kmag.

Approval Request Comment
[Feature/regressing bug #]: Fixing a problem with bug 1225633.
[User impact if declined]: Many questions/complaints from add-on authors about styles being different in popups vs. in browser.
[Describe test coverage new/current, TreeHerder]: Manual testing + currently baking on mozilla-central.
[Risks and why]: Semi-low risk.  Mostly CSS, but some reasonably small code changes.
[String/UUID change made/needed]: none

We may need a new patch for aurora, depending on what else gets uplifted.  I'm (obviously) more than happy to provide it if necessary!
Attachment #8747724 - Flags: approval-mozilla-aurora?
Comment on attachment 8747724 [details]
MozReview Request: Bug 1269081 - Only apply Firefox's default styles if the add-on author sets "browser_style" to true in the manifest. r=kmag.

Regression from 48, please uplift.
Attachment #8747724 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I was able to reproduce the initial issue on Firefox 49.0a1 (2016-04-29) under Windows 10 64-bit.

Tested on Firefox 49.0a1 (2016-05-12) and Firefox 48.0a2 (2016-05-13) under Windows 10 64-bit and Mac OS X 10.10.4 and noticed the following:
- the pop-up styling looks good now and the buttons are correctly displayed ( http://i.imgur.com/lzUCFOX.jpg )
- the following error message appears in Browser Console each time a letter is typed: http://pastebin.com/B6b5nMUt
- Malayalam Unicode is still not working (Ctrl+Q command does not toggle between English and Malayalam)

Should I file a separate bug for the Malayalam Unicode issue?


Additional info about the webextension behaviour on Ubuntu 16.04 32-bit:
- The Malayalam letters are incorrectly displayed: http://i.imgur.com/S8gCuSw.png
- Ctrl+Q command closes the browser.

Any thoughts about this?
Flags: needinfo?(bwinton)
Yes, please file a new bug for the Malayalam Unicode issue.

For the incorrect letters, I wonder if the default font on Ubuntu doesn't have those characters?  Eitehr way, that also seems like a different bug.

Thanks!
Flags: needinfo?(bwinton)
I don't think the other issues require bugs.

The main problem is that the add-on uses eval(), which our CSP blocks.

The problems on Linux are not really relevant. Ctrl+q is expected to quit the browser there, and the font issue is just because the system doesn't have the necessary fonts for that charset installed.
Is there a way around the CSP/eval problem documented somewhere on MDN that you know of?
In this case, the extension just needs to stop using eval. It could use JSON.parse, but even that's not necessary. It should switch to using ordinary object literals.
Thanks developers. I have removed the eval code and found another work around for it. And my extension works fine. No problems now. Thanks for your support. Can I remove my name from the email list?
Thanks Blake and Kris for clarifying this!

Based on the above comments (Comment 30, Comment 31 and Comment 32) I am marking this bug as Verified Fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.