[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

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?

(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?

(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?

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)

(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, ...

Mark would this be something you could take a look at please?

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.

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.

Here's a screenshot after the updates.

(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 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.

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.

(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>?

> Have you tried <html:span> instead of <span>? Yeah, it yields the same error.

This string is also localised so I would imagine adding namespaces would be just as clunky as copying the elements over from a div.

(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 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?

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

(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.

(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 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.

(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 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 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;;

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5a7fca706daa Fix alignment of webextensions permissions doorhanger r=florian

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 -

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.

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.

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.

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?

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 on attachment 8841733 [details] Bug 1329942 - Fix alignment of webextensions permissions doorhanger https://reviewboard.mozilla.org/r/115866/#review131350

Try build looks okay to me https://treeherder.mozilla.org/#/jobs?repo=try&revision=c88841c6aa7afda8bb7a4a19d1b7b9588f4d0967. Marking checkin-needed.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/174e08cd1f55 Fix alignment of webextensions permissions doorhanger r=florian

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?

(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.

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.

(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.