Closed
Bug 1329942
Opened 8 years ago
Closed 8 years ago
Misaligned icon and webextension name in permissions doorhanger
Categories
(WebExtensions :: Frontend, defect, P3)
Tracking
(firefox50 unaffected, firefox51 unaffected, firefox52 unaffected, firefox53 wontfix, firefox54 fix-optional, firefox55 verified)
VERIFIED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox50 | --- | unaffected |
firefox51 | --- | unaffected |
firefox52 | --- | unaffected |
firefox53 | --- | wontfix |
firefox54 | --- | fix-optional |
firefox55 | --- | verified |
People
(Reporter: vtamas, Assigned: mstriemer)
References
Details
(Whiteboard: [permissions] triaged)
Attachments
(2 files)
[Note]
This is a follow-up bug for Bug 1308309
[Affected versions]:
Firefox 53.0a1 (2017-01-09)
[Affected platforms]:
Windows 10 64-bit
Ubuntu 16.04 32-bit
[Steps to reproduce]:
1.Launch Firefox with a clean profile.
2.Navigate to about:config and create extensions.webextPermissionPrompts pref setting it to true.
3.Restart the browser.
4.Go to Panel Menu [≡] → Add-ons → Get Add-ons.
5.Install Awesome Screenshot add-on.
[Expected Results]:
The icon and the webextension name are properly aligned.
[Actual Results]:
The icon and the webextension name are not correctly positioned: http://screencast.com/t/hAup1RhpZo6
Updated•8 years ago
|
Assignee: nobody → aswan
Component: WebExtensions: General → WebExtensions: Frontend
Priority: -- → P3
Whiteboard: triaged
Updated•8 years ago
|
Whiteboard: triaged → [permissions] triaged
Updated•8 years ago
|
Comment 1•8 years ago
|
||
I think there are two things here:
1. The popup includes this <hbox>: http://searchfox.org/mozilla-central/rev/7da3c9dcf467964f2fb82f3a4c63972ee79bf696/toolkit/content/widgets/notification.xml#491
which includes this <description>: http://searchfox.org/mozilla-central/rev/7da3c9dcf467964f2fb82f3a4c63972ee79bf696/toolkit/content/widgets/notification.xml#496
which is empty but it still gets 5px of margin (1px on top, 4px on bottom)
2. The text content is inside a <popupnotificationcontent> that has a margin-top (osx style is here but the other platforms have the same rule): http://searchfox.org/mozilla-central/rev/7da3c9dcf467964f2fb82f3a4c63972ee79bf696/toolkit/themes/osx/global/global.css#279-281
I'm not sure what the right thing to do is here, I could have the eventCallback hide the first hbox? And override the margin-top on the <popupnotificationcontent> for the permissions notification? Florian can you help me out? Am I on the wrong track here?
Flags: needinfo?(florian)
Comment 2•8 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #1)
> I'm not sure what the right thing to do is here, I could have the
> eventCallback hide the first hbox? And override the margin-top on the
> <popupnotificationcontent> for the permissions notification? Florian can
> you help me out? Am I on the wrong track here?
Could we instead display the first line of text using that description element?
Flags: needinfo?(florian)
Comment 3•8 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> Could we instead display the first line of text using that description
> element?
I don't think so, PopupNotifications.jsm populates that element and does it as raw text, but the visual design calls for the addon name in bold text.
I guess that the showing event handler could go outside the <popupnotificationcontent> to find the existing <description> element and inject the right markup there instead of inside the <popupnotificationcontent>. That sounds hacky though, would you r+ that?
Flags: needinfo?(florian)
Comment 4•8 years ago
|
||
Hm, my understanding of XBL is quite limited, but it looks like the <description> element in question actually comes from XBL:
http://searchfox.org/mozilla-central/rev/8fa84ca6444e2e01fb405e078f6d2c8da0e55723/toolkit/content/widgets/notification.xml#496
And from some casual poking around in the browser toolbox, it looks like I can't actually navigate those elements with standard DOM techniques (e.g., parentElement, el.getElementByFoo(), etc)
Is there some clever way to use the binding as-is to inject raw markup into the <description>? The alternative seems to be modifying the binding somehow and then, probably, modifying PopupNotifications.jsm to use the modified binding to inject raw markup.
Can you point me in the right direction Florian? (And no urgency, this can wait until next week)
Comment 5•8 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #4)
> Hm, my understanding of XBL is quite limited, but it looks like the
> <description> element in question actually comes from XBL:
> http://searchfox.org/mozilla-central/rev/
> 8fa84ca6444e2e01fb405e078f6d2c8da0e55723/toolkit/content/widgets/
> notification.xml#496
> And from some casual poking around in the browser toolbox, it looks like I
> can't actually navigate those elements with standard DOM techniques (e.g.,
> parentElement, el.getElementByFoo(), etc)
> Is there some clever way to use the binding as-is to inject raw markup into
> the <description>? The alternative seems to be modifying the binding
> somehow
The <description> element at http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/toolkit/content/widgets/notification.xml#496 doesn't have an anonid attribute, so you would need to add one.
Then (optionally) you could add a description field like http://searchfox.org/mozilla-central/rev/e95e4ed8b5229a29dacc32c0b90968df5e7a6ff3/toolkit/content/widgets/notification.xml#542 to expose it cleanly for use outside the binding.
You would then do something like:
doc.getElementById("addon-webext-permissions-notification").description.innerHTML = ...
It's hackish, but I think I would r+ it.
If you don't want to go this way, then the other way I see is to use CSS to override what's in your way. So either display: none, or margin: O, ...
Flags: needinfo?(florian)
Comment 6•8 years ago
|
||
Mark would this be something you could take a look at please?
Assignee: aswan → mstriemer
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•8 years ago
|
||
There were some additional requests for alignment updates from ux in bug 1342452. I've included them in this commit. All of the CSS is related to bug 1342452, moving the heading into the description fixed the alignment issue for this bug.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8841733 -
Flags: review?(florian)
Assignee | ||
Comment 10•8 years ago
|
||
Bringing in emanuela's description from bug 1342452 since that got fixed at the same time:
> Hi guys,
>
> some frontend enhancement here.
>
> 1) .addon-webext-perm-header should have font-size:1em (currently 1.3)
> 2) The ul #addon-webext-perm-list should have padding-inline-start: 15px (currently 40px)
> 3) Currently, the arrow panel has a lot element of space on the top and on the bottom. The structure was too complex for letting me play with the browser inspector. However, I think it makes sense to take a look at the work done for the permissions arrow panels for this matter.
>
> Attached how the arrow panel looks just applying the first two points.
Assignee | ||
Comment 12•8 years ago
|
||
Here's a screenshot after the updates.
Comment 13•8 years ago
|
||
(In reply to Mark Striemer [:mstriemer] from comment #12)
> Created attachment 8842219 [details]
> updated-dialog-screenshot.png
>
> Here's a screenshot after the updates.
It looks great! Great job
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8841733 [details]
Bug 1329942 - Fix alignment of webextensions permissions doorhanger
https://reviewboard.mozilla.org/r/115866/#review117786
::: browser/modules/ExtensionsUI.jsm:283
(Diff revision 2)
>
> showPermissionsPrompt(browser, strings, icon) {
> function eventCallback(topic) {
> - if (topic == "showing") {
> - let doc = this.browser.ownerDocument;
> + let doc = this.browser.ownerDocument;
> - doc.getElementById("addon-webext-perm-header").innerHTML = strings.header;
> + if (topic == "showing" || topic == "shown") {
Why do we need to set the descrition on both the showing and shown events? From looking at the code in PopupNotifications.jsm, I see _refreshPanel is called between the showing and shown event, so I would expect anything we do here during the showing event to be overwritten.
::: browser/modules/ExtensionsUI.jsm:284
(Diff revision 2)
> showPermissionsPrompt(browser, strings, icon) {
> function eventCallback(topic) {
> - if (topic == "showing") {
> - let doc = this.browser.ownerDocument;
> + let doc = this.browser.ownerDocument;
> - doc.getElementById("addon-webext-perm-header").innerHTML = strings.header;
> -
> + if (topic == "showing" || topic == "shown") {
> + const popup = doc.getElementById("addon-webext-permissions-notification");
nit: while 'const' isn't incorrect, in these cases we would usually use 'let', and that would be more consistent with the surrounding code.
::: browser/modules/ExtensionsUI.jsm:291
(Diff revision 2)
> + description.classList.add("addon-webext-perm-header");
> + description.classList.add("plain");
> + description.removeAttribute("value");
> +
> + const parseHTMLDiv = doc.createElement("div");
> + parseHTMLDiv.innerHTML = strings.header;
Is description.innerHTML not working? Despite the 'HTML' in the name of the field, it usually works on XUL elements too.
Attachment #8841733 -
Flags: review?(florian) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8841733 [details]
Bug 1329942 - Fix alignment of webextensions permissions doorhanger
https://reviewboard.mozilla.org/r/115866/#review117786
> Why do we need to set the descrition on both the showing and shown events? From looking at the code in PopupNotifications.jsm, I see _refreshPanel is called between the showing and shown event, so I would expect anything we do here during the showing event to be overwritten.
I recorded some videos of this and stepped through frame-by-frame and there appears to be no difference for also setting this on "showing". I updated it to only set it on "shown".
While looking at the videos I noticed that there is actually a flash where there is no heading before it appears (1-2 frames on these videos). Since the popup resizes it is somewhat noticable that something weird happened.
I updated the creation of the popup to set the heading to plain text. It is then upgraded to the HTML version with the bold add-on name here. You can still notice on video but it is hard to spot in real-time (on my laptop anyway).
> Is description.innerHTML not working? Despite the 'HTML' in the name of the field, it usually works on XUL elements too.
I tried using `description.innerHTML` in the console to see what was happening. Giving it a string without HTML in it works, but including something like `Foo <span>Bar</span>` throws with `XML Parsing Error: prefix not bound to a namespace`.
Using the div to set the HTML and pull out elements, the `innerHTML` of the div is `Foo <span xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">Bar</span>`. Attempting to set `description.innerHTML = parseHTMLDiv.innerHTML` throws the same error.
Comment 17•8 years ago
|
||
(In reply to Mark Striemer [:mstriemer] from comment #16)
> I tried using `description.innerHTML` in the console to see what was
> happening. Giving it a string without HTML in it works, but including
> something like `Foo <span>Bar</span>` throws with `XML Parsing Error: prefix
> not bound to a namespace`.
Have you tried <html:span> instead of <span>?
Assignee | ||
Comment 18•8 years ago
|
||
> Have you tried <html:span> instead of <span>?
Yeah, it yields the same error.
Assignee | ||
Comment 19•8 years ago
|
||
This string is also localised so I would imagine adding namespaces would be just as clunky as copying the elements over from a div.
Comment 20•8 years ago
|
||
(In reply to Mark Striemer [:mstriemer] from comment #19)
> This string is also localised so I would imagine adding namespaces would be
> just as clunky as copying the elements over from a div.
Are you saying we have the <span> in a localized string?
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8841733 [details]
Bug 1329942 - Fix alignment of webextensions permissions doorhanger
https://reviewboard.mozilla.org/r/115866/#review118290
::: browser/modules/ExtensionsUI.jsm:288
(Diff revisions 2 - 3)
> - const description = popup.description;
> + // With value="" the inner HTML is ignored.
> - description.classList.add("addon-webext-perm-header");
> - description.classList.add("plain");
> description.removeAttribute("value");
> -
> - const parseHTMLDiv = doc.createElement("div");
> + // Remove the old content (we pre-populate with the plain text).
> + while (description.childNodes.length > 0) {
while (description.firstChild) {
description.firstChild.remove();
}
::: browser/modules/ExtensionsUI.jsm:292
(Diff revisions 2 - 3)
> -
> - const parseHTMLDiv = doc.createElement("div");
> + // Remove the old content (we pre-populate with the plain text).
> + while (description.childNodes.length > 0) {
> + description.removeChild(description.childNodes[0]);
> + }
> + // Setting HTML with description.innerHTML throws an XML Parsing Error. Setting it on
> + // a div works though, so do that then move the child elements over.
So how is this different from http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/browser/modules/ExtensionsUI.jsm#305 ?
::: browser/modules/ExtensionsUI.jsm:296
(Diff revisions 2 - 3)
> + // Setting HTML with description.innerHTML throws an XML Parsing Error. Setting it on
> + // a div works though, so do that then move the child elements over.
> + let parseHTMLDiv = doc.createElement("div");
> parseHTMLDiv.innerHTML = strings.header;
> - const childNodes = Array.prototype.slice.call(parseHTMLDiv.childNodes);
> - childNodes.forEach((child) => {
> + while (parseHTMLDiv.childNodes.length > 0) {
> + description.appendChild(parseHTMLDiv.childNodes[0]);
This should also just use .firstChild, both for the test and to access the node you append.
::: browser/modules/ExtensionsUI.jsm:347
(Diff revisions 2 - 3)
> },
> ];
>
> - win.PopupNotifications.show(browser, "addon-webext-permissions", "",
> + // Get the text value of strings.header to pre-populate the header. This will get
> + // overwritten with the HTML version later.
> + let escapeHeader = browser.ownerDocument.createElement('div');
Use double quotes here (I think eslint will flag this).
::: browser/modules/ExtensionsUI.jsm:349
(Diff revisions 2 - 3)
>
> - win.PopupNotifications.show(browser, "addon-webext-permissions", "",
> + // Get the text value of strings.header to pre-populate the header. This will get
> + // overwritten with the HTML version later.
> + let escapeHeader = browser.ownerDocument.createElement('div');
> + escapeHeader.innerHTML = strings.header;
> + win.PopupNotifications.show(browser, "addon-webext-permissions", escapeHeader.textContent,
nit: wrap this long line.
::: browser/themes/osx/browser.css:3104
(Diff revisions 2 - 3)
>
> .addon-webext-perm-text {
> margin-inline-start: 0;
> }
>
> +#addon-webext-permissions-notification .popup-notification-description {
Can we avoid this descendant selector?
The popup-notification-description element seems to have a popupid attribute, can we use it in the selector?
Is this needed only on Mac?
Attachment #8841733 -
Flags: review?(florian) → review-
Assignee | ||
Comment 22•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8841733 [details]
Bug 1329942 - Fix alignment of webextensions permissions doorhanger
https://reviewboard.mozilla.org/r/115866/#review118290
> So how is this different from http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/browser/modules/ExtensionsUI.jsm#305 ?
To be honest this is my first real encounter with XUL and I don't know for sure.
I would guess because the XML files these elements are defined in define different namespaces. The `#addon-webext-perm-header` `description` is defined in popup-notifications.inc [1] that gets included in browser.xul [2] whereas the header `description` is in notification.xml [3]. notification.xml does not seem to define the HTML namespace but browser.xul does, so maybe that's why?
[1] http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/browser/base/content/popup-notifications.inc#78
[2] http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/browser/base/content/browser.xul#436
[3] http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/toolkit/content/widgets/notification.xml
Comment 23•8 years ago
|
||
(In reply to Mark Striemer [:mstriemer] from comment #22)
> I would guess because the XML files these elements are defined in define
> different namespaces. The `#addon-webext-perm-header` `description` is
> defined in popup-notifications.inc [1] that gets included in browser.xul [2]
> whereas the header `description` is in notification.xml [3].
> notification.xml does not seem to define the HTML namespace but browser.xul
> does, so maybe that's why?
If it lets us simplify the code, adding xmlns:html="http://www.w3.org/1999/xhtml" in notification.xml is fine.
Assignee | ||
Comment 24•8 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #23)
> (In reply to Mark Striemer [:mstriemer] from comment #22)
>
> If it lets us simplify the code, adding
> xmlns:html="http://www.w3.org/1999/xhtml" in notification.xml is fine.
This doesn't actually seem to fix it, so I don't really know what's up. I tried setting innerHTML to `<span>Foo</span>` and `<html:span>Foo</html:span>` and neither worked.
Comment hidden (mozreview-request) |
Comment 26•8 years ago
|
||
mozreview-review |
Comment on attachment 8841733 [details]
Bug 1329942 - Fix alignment of webextensions permissions doorhanger
https://reviewboard.mozilla.org/r/115866/#review119698
Sorry for the delay here, I wanted to take time to understand the innerHTML issue.
::: browser/modules/ExtensionsUI.jsm:289
(Diff revisions 3 - 4)
> let description = doc.getElementById("addon-webext-permissions-notification").description;
> - // With value="" the inner HTML is ignored.
> + // The inner HTML is ignored if there's a value, so remove it.
> description.removeAttribute("value");
> // Remove the old content (we pre-populate with the plain text).
> - while (description.childNodes.length > 0) {
> - description.removeChild(description.childNodes[0]);
> + while (description.firstChild) {
> + description.removeChild(description.firstChild);
This should be description.firstChild.remove() (already said in comment 21).
But if we can use innerHTML, then we don't need this anymore anyway.
::: browser/modules/ExtensionsUI.jsm:291
(Diff revisions 3 - 4)
> description.removeAttribute("value");
> // Remove the old content (we pre-populate with the plain text).
> - while (description.childNodes.length > 0) {
> - description.removeChild(description.childNodes[0]);
> + while (description.firstChild) {
> + description.removeChild(description.firstChild);
> }
> // Setting HTML with description.innerHTML throws an XML Parsing Error. Setting it on
I've finally taken time to figure this out.
Using a debug build, I could see that the string we are failing to parse is:
<window xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns:svg="http://www.w3.org/2000/svg" xmlns:html="http://www.w3.org/1999/xhtml" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"><popupset xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"><panel xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"><popupnotification xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"><xul:hbox><xul:vbox><xul:hbox><xul:vbox><xul:description>
This strings seems to be automatically generated to give context to the XML parser. I would guess it's generated including the current node's element name, and then the parent, etc... until the root node of the current document.
The problem here is that the current node is a xul:description, and given that the document is browser.xul where xul is the default namespace, the xul: prefix isn't defined on the <browser> tag.
Adding xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" in browser.xul is all it takes to fix the error.
I think we should do this; if only to save the confusion for the next person attempting to use .innerHTML inside an xbl binding.
Attachment #8841733 -
Flags: review?(florian) → review-
Comment 27•8 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> ::: browser/modules/ExtensionsUI.jsm:289
> > + while (description.firstChild) {
> > + description.removeChild(description.firstChild);
>
> This should be description.firstChild.remove() (already said in comment 21).
I filed bug 1345253 to clean this up throughout the tree.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8841733 [details]
Bug 1329942 - Fix alignment of webextensions permissions doorhanger
https://reviewboard.mozilla.org/r/115866/#review119698
> This should be description.firstChild.remove() (already said in comment 21).
>
> But if we can use innerHTML, then we don't need this anymore anyway.
Oops, I just saw `firstChild` instead of `childNodes[0]`. Looks like `innerHTML` works now though.
> I've finally taken time to figure this out.
>
> Using a debug build, I could see that the string we are failing to parse is:
>
> <window xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns:svg="http://www.w3.org/2000/svg" xmlns:html="http://www.w3.org/1999/xhtml" xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"><popupset xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"><panel xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"><popupnotification xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"><xul:hbox><xul:vbox><xul:hbox><xul:vbox><xul:description>
>
> This strings seems to be automatically generated to give context to the XML parser. I would guess it's generated including the current node's element name, and then the parent, etc... until the root node of the current document.
>
> The problem here is that the current node is a xul:description, and given that the document is browser.xul where xul is the default namespace, the xul: prefix isn't defined on the <browser> tag.
>
> Adding xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" in browser.xul is all it takes to fix the error.
>
> I think we should do this; if only to save the confusion for the next person attempting to use .innerHTML inside an xbl binding.
Thanks for looking into this! Makes the code much clearer and will likely save someone some pain down the road.
Comment 30•8 years ago
|
||
mozreview-review |
Comment on attachment 8841733 [details]
Bug 1329942 - Fix alignment of webextensions permissions doorhanger
https://reviewboard.mozilla.org/r/115866/#review120056
Thanks! :-)
::: browser/modules/ExtensionsUI.jsm:284
(Diff revision 5)
> showPermissionsPrompt(browser, strings, icon) {
> function eventCallback(topic) {
> - if (topic == "showing") {
> - let doc = this.browser.ownerDocument;
> + let doc = this.browser.ownerDocument;
> - doc.getElementById("addon-webext-perm-header").innerHTML = strings.header;
> -
> + if (topic == "shown") {
> + let description = doc.getElementById("addon-webext-permissions-notification").description;
nit: this line is too long, and the description variable isn't helping readability, this can be simplified to:
doc.getElementById("addon-webext-permissions-notification")
.description.innerHTML = strings.header;;
Attachment #8841733 -
Flags: review?(florian) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 32•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5a7fca706daa
Fix alignment of webextensions permissions doorhanger r=florian
Keywords: checkin-needed
Comment 33•8 years ago
|
||
Backed out for failing browser-chrome browser_bug553455.js:
https://hg.mozilla.org/integration/autoland/rev/9dff76f7dd99dcee3ef3ffb1890b026c7c3c09b2
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5a7fca706daab4f187b38c6918d73006178d2df4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=82512065&repo=autoland
[task 2017-03-08T17:37:10.262777Z] 17:37:10 INFO - Running test_whitelistedInstall
[task 2017-03-08T17:37:10.266144Z] 17:37:10 INFO - Waiting for addon-progress notification
[task 2017-03-08T17:37:10.268604Z] 17:37:10 INFO - Waiting for addon-install-confirmation notification
[task 2017-03-08T17:37:10.273099Z] 17:37:10 INFO - Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/installtrigger.html?%7B%22XPI%22%3A%22amosigned.xpi%22%7D" line: 0}]
[task 2017-03-08T17:37:10.275653Z] 17:37:10 INFO - Saw a notification
[task 2017-03-08T17:37:10.280475Z] 17:37:10 INFO - TEST-PASS | browser/base/content/test/general/browser_bug553455.js | Panel should be open -
[task 2017-03-08T17:37:10.285191Z] 17:37:10 INFO - TEST-PASS | browser/base/content/test/general/browser_bug553455.js | Should be the right number of notifications -
[task 2017-03-08T17:37:10.287619Z] 17:37:10 INFO - TEST-PASS | browser/base/content/test/general/browser_bug553455.js | Should have seen the right notification -
[task 2017-03-08T17:37:10.289743Z] 17:37:10 INFO - Buffered messages finished
[task 2017-03-08T17:37:10.294040Z] 17:37:10 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug553455.js | The install button should be disabled -
[task 2017-03-08T17:37:10.298315Z] 17:37:10 INFO - Stack trace:
[task 2017-03-08T17:37:10.301096Z] 17:37:10 INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug553455.js:waitForProgressNotification/<:80
[task 2017-03-08T17:37:10.304099Z] 17:37:10 INFO - waitForProgressNotification@chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug553455.js:41:10
[task 2017-03-08T17:37:10.306386Z] 17:37:10 INFO - test_whitelistedInstall/<@chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug553455.js:322:27
[task 2017-03-08T17:37:10.308965Z] 17:37:10 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:319:42
[task 2017-03-08T17:37:10.311869Z] 17:37:10 INFO - TaskImpl@resource://gre/modules/Task.jsm:277:3
[task 2017-03-08T17:37:10.317722Z] 17:37:10 INFO - asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-03-08T17:37:10.321017Z] 17:37:10 INFO - Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-03-08T17:37:10.322918Z] 17:37:10 INFO - test_whitelistedInstall@chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug553455.js:315:10
[task 2017-03-08T17:37:10.324834Z] 17:37:10 INFO - @chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug553455.js:1084:11
[task 2017-03-08T17:37:10.327392Z] 17:37:10 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:319:42
[task 2017-03-08T17:37:10.329407Z] 17:37:10 INFO - process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
[task 2017-03-08T17:37:10.333603Z] 17:37:10 INFO - walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
[task 2017-03-08T17:37:10.336173Z] 17:37:10 INFO - Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
[task 2017-03-08T17:37:10.338712Z] 17:37:10 INFO - schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
[task 2017-03-08T17:37:10.340683Z] 17:37:10 INFO - completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
[task 2017-03-08T17:37:10.343457Z] 17:37:10 INFO - checkForCompletion@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:567:9
[task 2017-03-08T17:37:10.346177Z] 17:37:10 INFO - resolver@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:574:29
[task 2017-03-08T17:37:10.348158Z] 17:37:10 INFO - promise callback*Promise.all/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:577:9
[task 2017-03-08T17:37:10.352239Z] 17:37:10 INFO - Promise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:390:5
[task 2017-03-08T17:37:10.354121Z] 17:37:10 INFO - Promise.all@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:554:10
[task 2017-03-08T17:37:10.357004Z] 17:37:10 INFO - removeTab@chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug553455.js:184:10
[task 2017-03-08T17:37:10.359186Z] 17:37:10 INFO - test_blockedInstall/<@chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug553455.js:310:11
[task 2017-03-08T17:37:10.361888Z] 17:37:10 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:319:42
[task 2017-03-08T17:37:10.364558Z] 17:37:10 INFO - process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
[task 2017-03-08T17:37:10.366972Z] 17:37:10 INFO - walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
[task 2017-03-08T17:37:10.369774Z] 17:37:10 INFO - Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
[task 2017-03-08T17:37:10.372886Z] 17:37:10 INFO - schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
[task 2017-03-08T17:37:10.375007Z] 17:37:10 INFO - completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
[task 2017-03-08T17:37:10.377071Z] 17:37:10 INFO - eventListener@chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug553455.js:113:9
[task 2017-03-08T17:37:10.379399Z] 17:37:10 INFO - PopupNotifications_showPanel/</this._popupshownListener@resource://gre/modules/PopupNotifications.jsm:1000:9
[task 2017-03-08T17:37:10.381583Z] 17:37:10 INFO - EventListener.handleEvent*PopupNotifications_showPanel/<@resource://gre/modules/PopupNotifications.jsm:1003:7
[task 2017-03-08T17:37:10.384007Z] 17:37:10 INFO - process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
[task 2017-03-08T17:37:10.385989Z] 17:37:10 INFO - walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
[task 2017-03-08T17:37:10.388307Z] 17:37:10 INFO - Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
[task 2017-03-08T17:37:10.390703Z] 17:37:10 INFO - schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
[task 2017-03-08T17:37:10.393262Z] 17:37:10 INFO - completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
[task 2017-03-08T17:37:10.395168Z] 17:37:10 INFO - get _worker/worker.onmessage@resource://gre/modules/PromiseWorker.jsm:231:9
[task 2017-03-08T17:37:10.397426Z] 17:37:10 INFO - EventHandlerNonNull*get _worker@resource://gre/modules/PromiseWorker.jsm:217:5
[task 2017-03-08T17:37:10.399493Z] 17:37:10 INFO - postMessage@resource://gre/modules/PromiseWorker.jsm:291:9
[task 2017-03-08T17:37:10.401496Z] 17:37:10 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:319:42
[task 2017-03-08T17:37:10.404963Z] 17:37:10 INFO - process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
[task 2017-03-08T17:37:10.406905Z] 17:37:10 INFO - walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
[task 2017-03-08T17:37:10.408819Z] 17:37:10 INFO - Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
[task 2017-03-08T17:37:10.410716Z] 17:37:10 INFO - schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
[task 2017-03-08T17:37:10.412750Z] 17:37:10 INFO - Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:455:5
[task 2017-03-08T17:37:10.414898Z] 17:37:10 INFO - push@resource://gre/modules/osfile/osfile_async_front.jsm:375:19
[task 2017-03-08T17:37:10.416816Z] 17:37:10 INFO - post@resource://gre/modules/osfile/osfile_async_front.jsm:407:12
[task 2017-03-08T17:37:10.419288Z] 17:37:10 INFO - exists@resource://gre/modules/osfile/osfile_async_front.jsm:1104:10
[task 2017-03-08T17:37:10.421400Z] 17:37:10 INFO - AutoMigrate.canUndo<@resource://app/modules/AutoMigrate.jsm:200:26
[task 2017-03-08T17:37:10.423896Z] 17:37:10 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:319:42
[task 2017-03-08T17:37:10.425894Z] 17:37:10 INFO - TaskImpl@resource://gre/modules/Task.jsm:277:3
[task 2017-03-08T17:37:10.428203Z] 17:37:10 INFO - asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-03-08T17:37:10.430161Z] 17:37:10 INFO - AutoMigrate.maybeShowUndoNotification<@resource://app/modules/AutoMigrate.jsm:325:17
[task 2017-03-08T17:37:10.435630Z] 17:37:10 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:319:42
[task 2017-03-08T17:37:10.438324Z] 17:37:10 INFO - TaskImpl@resource://gre/modules/Task.jsm:277:3
[task 2017-03-08T17:37:10.441611Z] 17:37:10 INFO - asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-03-08T17:37:10.443385Z] 17:37:10 INFO - init/<@resource://app/modules/AboutNewTab.jsm:36:16
[task 2017-03-08T17:37:10.445110Z] 17:37:10 INFO - callListeners@resource://gre/modules/RemotePageManager.jsm:35:9
[task 2017-03-08T17:37:10.448715Z] 17:37:10 INFO - portMessageReceived@resource://gre/modules/RemotePageManager.jsm:110:5
[task 2017-03-08T17:37:10.451365Z] 17:37:10 INFO - callListeners@resource://gre/modules/RemotePageManager.jsm:35:9
[task 2017-03-08T17:37:10.453368Z] 17:37:10 INFO - ChromeMessagePort.prototype.message@resource://gre/modules/RemotePageManager.jsm:338:3
[task 2017-03-08T17:37:10.455933Z] 17:37:10 INFO - Console message: [JavaScript Error: "NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsISupports.QueryInterface]" {file: "chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug553455.js" line: 1027}]
[task 2017-03-08T17:37:10.459690Z] 17:37:10 INFO - 1488994628849 addons.xpi DEBUG Download started for http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/amosigned.xpi to file /tmp/tmp-jdz.xpi
[task 2017-03-08T17:37:10.462093Z] 17:37:10 INFO - 1488994628854 addons.xpi DEBUG Download of http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/amosigned.xpi completed.
[task 2017-03-08T17:37:10.464470Z] 17:37:10 INFO - Console message: 1488994628849 addons.xpi DEBUG Download started for http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/amosigned.xpi to file /tmp/tmp-jdz.xpi
[task 2017-03-08T17:37:10.466952Z] 17:37:10 INFO - Console message: 1488994628854 addons.xpi DEBUG Download of http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/amosigned.xpi completed.
[task 2017-03-08T17:37:10.470153Z] 17:37:10 INFO - 1488994628955 addons.repository DEBUG cacheAddons: enabled false IDs ["unsigned-xpi@tests.mozilla.org"]
[task 2017-03-08T17:37:10.473544Z] 17:37:10 INFO - Console message: 1488994628955 addons.repository DEBUG cacheAddons: enabled false IDs ["unsigned-xpi@tests.mozilla.org"]
[task 2017-03-08T17:37:10.475617Z] 17:37:10 INFO - Saw a addon-install-confirmation notification
[task 2017-03-08T17:37:10.477705Z] 17:37:10 INFO - TEST-PASS | browser/base/content/test/general/browser_bug553455.js | Panel should be open -
Flags: needinfo?(mstriemer)
Assignee | ||
Comment 34•8 years ago
|
||
mozreview-review |
Comment on attachment 8841733 [details]
Bug 1329942 - Fix alignment of webextensions permissions doorhanger
https://reviewboard.mozilla.org/r/115866/#review120170
Re-opening since this was backed out.
Attachment #8841733 -
Flags: review?(mstriemer)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(mstriemer)
Attachment #8841733 -
Flags: review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 36•8 years ago
|
||
mozreview-review |
Comment on attachment 8841733 [details]
Bug 1329942 - Fix alignment of webextensions permissions doorhanger
https://reviewboard.mozilla.org/r/115864/#review120172
::: toolkit/modules/PopupNotifications.jsm:847
(Diff revision 6)
> } else {
> this._setNotificationUIState(popupnotification, checkbox.uncheckedState);
> }
> } else {
> popupnotification.setAttribute("checkboxhidden", "true");
> + this._setNotificationUIState(popupnotification);
I misread the `if` above here and this didn't work the way I expected. It looked safe but I guess it broke some other tests (I couldn't get try to work, probably should have figured that out, lesson learned).
I feel like this still needs to clear the warning but for some reason even checking `checkbox.checked` and passing in the checked/unchecked state doesn't fix the failing test.
The warning is now hidden using CSS, I don't see any code in ExtensionsUI.jsm that sets the warning so that should be fine.
Assignee | ||
Updated•8 years ago
|
Attachment #8841733 -
Flags: review?(florian)
Comment 37•8 years ago
|
||
Comment on attachment 8841733 [details]
Bug 1329942 - Fix alignment of webextensions permissions doorhanger
(In reply to Mark Striemer [:mstriemer] from comment #36)
> The warning is now hidden using CSS, I don't see any code in
> ExtensionsUI.jsm that sets the warning so that should be fine.
Well, ExtensionsUI.jsm doesn't care, but unless I'm missing something you are hiding the warning element unconditionally here, so other notifications will be affected.
Attachment #8841733 -
Flags: review?(florian) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8841733 -
Flags: review- → review?(florian)
Comment 40•8 years ago
|
||
mozreview-review |
Comment on attachment 8841733 [details]
Bug 1329942 - Fix alignment of webextensions permissions doorhanger
https://reviewboard.mozilla.org/r/115866/#review130870
::: browser/modules/ExtensionsUI.jsm:352
(Diff revision 9)
> + .description.innerHTML = strings.header;
> + } else if (topic == "showing") {
> + // Hide the warning label since it is shown even if there's no
> + // associated checkbox.
> + doc.getElementById("addon-webext-permissions-notification")
> + .setAttribute("warninghidden", "true");
Can you refresh my memory on this?
- What was the problem with this warning element in the first place?
- Why didn't the code hiding it from PopupNotifications.jsm work?
Attachment #8841733 -
Flags: review?(florian)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 42•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8841733 [details]
Bug 1329942 - Fix alignment of webextensions permissions doorhanger
https://reviewboard.mozilla.org/r/115866/#review130870
> Can you refresh my memory on this?
>
> - What was the problem with this warning element in the first place?
> - Why didn't the code hiding it from PopupNotifications.jsm work?
The warning element has some margin on it and it adds extra whitespace to the popup even if there's no text in it.
I think hiding it in PopuoNotifications.jsm didn't work because I used the `_setNotificationUIState` helper which seemed reasonable but I guess broke something unrelated. I just tried hiding it without using the helper and the tests that failed before are now passing. I searched for `warninglabel` and `warningLabel` and it looks like nothing is setting the attribute directly so hiding it in PopupNotifications.jsm looks like it should be fine to me.
Comment 43•8 years ago
|
||
mozreview-review |
Comment on attachment 8841733 [details]
Bug 1329942 - Fix alignment of webextensions permissions doorhanger
https://reviewboard.mozilla.org/r/115866/#review131350
Attachment #8841733 -
Flags: review?(florian) → review+
Assignee | ||
Comment 44•8 years ago
|
||
Try build looks okay to me https://treeherder.mozilla.org/#/jobs?repo=try&revision=c88841c6aa7afda8bb7a4a19d1b7b9588f4d0967.
Marking checkin-needed.
Keywords: checkin-needed
Comment 45•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/174e08cd1f55
Fix alignment of webextensions permissions doorhanger r=florian
Keywords: checkin-needed
Comment 46•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•8 years ago
|
Reporter | ||
Comment 47•8 years ago
|
||
Tested this bug on Firefox 55.0a1 (2017-04-19) under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.12.1 and noticed the following potential issues:
- Permissions pop-up flickers while installing from disco pane/local file - I managed to capture the first state of the pop-up https://www.screencast.com/t/wHaUbqb2lAR . Seems that “More” word initially appears at the end of the first line and only after that is displayed on the second line. I suspect that this happens because the webextension name is bolded with delay. (I did not encounter this issue on Ubuntu 16.04 32-bit and Mac OS X 10.12.1)
- The icon and webextension name are still not aligned in confirmation doorhanger: https://www.screencast.com/t/oycdtPK8UyF
Any thoughts about these issues? Should I file new bugs for both of them?
Flags: needinfo?(mstriemer)
Assignee | ||
Comment 48•8 years ago
|
||
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #47)
> - Permissions pop-up flickers while installing from disco pane/local file -
> I managed to capture the first state of the pop-up
> https://www.screencast.com/t/wHaUbqb2lAR . Seems that “More” word initially
> appears at the end of the first line and only after that is displayed on the
> second line. I suspect that this happens because the webextension name is
> bolded with delay. (I did not encounter this issue on Ubuntu 16.04 32-bit
> and Mac OS X 10.12.1)
That's unfortunate. There is a delay on the add-on name being bolded. I was never able to have it be noticeable on my computer without recording a video and stepping frame by frame, but it isn't ideal. I'd say we can file for this and I can take a look at it.
> - The icon and webextension name are still not aligned in confirmation
> doorhanger: https://www.screencast.com/t/oycdtPK8UyF
This issue is tracked in bug 1351255.
Flags: needinfo?(mstriemer)
Assignee | ||
Comment 49•8 years ago
|
||
Actually that second point is a slightly different issue. I'll add a note to that bug and fix them both at the same time.
Reporter | ||
Comment 50•8 years ago
|
||
(In reply to Mark Striemer [:mstriemer] from comment #48)
> That's unfortunate. There is a delay on the add-on name being bolded. I was
> never able to have it be noticeable on my computer without recording a video
> and stepping frame by frame, but it isn't ideal. I'd say we can file for
> this and I can take a look at it.
Filed Bug 1358431
Based on Comment 47 and Comment 48 I am marking this bug as Verified since the other issues are tracked separately.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•