Closed
Bug 1269081
Opened 8 years ago
Closed 8 years ago
Pop up display problem
Categories
(Firefox :: Extension Compatibility, defect)
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)
1.17 MB,
application/pdf
|
Details | |
12.19 KB,
application/zip
|
Details | |
58 bytes,
text/x-review-board-request
|
kmag
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
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.
Reporter | ||
Comment 1•8 years ago
|
||
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)
Reporter | ||
Comment 3•8 years ago
|
||
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.
Reporter | ||
Comment 5•8 years ago
|
||
Reporter | ||
Comment 6•8 years ago
|
||
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
Assignee | ||
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
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)
Reporter | ||
Comment 10•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
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".)
Reporter | ||
Comment 12•8 years ago
|
||
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
Reporter | ||
Comment 13•8 years ago
|
||
Problem gets from file choose_lang.js, line number 190 conversionHash = eval("("+opr+")"); add-on has attached above
Assignee | ||
Comment 14•8 years ago
|
||
(Kris? Does the CSP stuff sound similar to something you've run into?)
Assignee | ||
Comment 15•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50013/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50013/
Attachment #8747724 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 16•8 years ago
|
||
(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.)
Comment 17•8 years ago
|
||
(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 | ||
Updated•8 years ago
|
Assignee: nobody → bwinton
Comment 18•8 years ago
|
||
(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)
Assignee | ||
Comment 19•8 years ago
|
||
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.
Assignee | ||
Comment 20•8 years ago
|
||
(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… :)
Updated•8 years ago
|
status-firefox46:
--- → unaffected
status-firefox47:
--- → unaffected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
QA Contact: vasilica.mihasca
Assignee | ||
Comment 21•8 years ago
|
||
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/
Updated•8 years ago
|
Attachment #8747724 -
Flags: review?(kmaglione+bmo) → review+
Comment 22•8 years ago
|
||
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".
Assignee | ||
Comment 23•8 years ago
|
||
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/
Assignee | ||
Comment 24•8 years ago
|
||
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 )
Comment 26•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f9054ecf5f24
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 27•8 years ago
|
||
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 28•8 years ago
|
||
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+
Comment 29•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/00a32da7e4d6
Comment 30•8 years ago
|
||
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)
Assignee | ||
Comment 31•8 years ago
|
||
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)
Comment 32•8 years ago
|
||
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.
Assignee | ||
Comment 33•8 years ago
|
||
Is there a way around the CSP/eval problem documented somewhere on MDN that you know of?
Comment 34•8 years ago
|
||
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.
Reporter | ||
Comment 35•8 years ago
|
||
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?
Comment 36•8 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•