The default bug view has changed. See this FAQ.

Misaligned icon and webextension name in permissions doorhanger

NEW
Assigned to

Status

()

Toolkit
WebExtensions: Frontend
P3
minor
3 months ago
11 days ago

People

(Reporter: vasilica_mihasca, Assigned: mstriemer)

Tracking

(Blocks: 1 bug)

53 Branch
Points:
---

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52 unaffected, firefox53 affected)

Details

(Whiteboard: [permissions] triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

[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

2 months ago
Assignee: nobody → aswan
Component: WebExtensions: General → WebExtensions: Frontend
Priority: -- → P3
Whiteboard: triaged

Updated

2 months ago
Whiteboard: triaged → [permissions] triaged

Updated

2 months ago
Blocks: 1308292
No longer blocks: 1308309

Comment 1

2 months 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)
(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

2 months 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

2 months 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)
(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

2 months ago
Mark would this be something you could take a look at please?
Assignee: aswan → mstriemer
Comment hidden (mozreview-request)
(Assignee)

Comment 8

29 days 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

28 days ago
Attachment #8841733 - Flags: review?(florian)
(Assignee)

Comment 10

28 days 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)

Updated

28 days ago
Duplicate of this bug: 1342452
(Assignee)

Comment 12

28 days ago
Created attachment 8842219 [details]
updated-dialog-screenshot.png

Here's a screenshot after the updates.

Comment 13

28 days 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

28 days 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

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

27 days ago
> Have you tried <html:span> instead of <span>?

Yeah, it yields the same error.
(Assignee)

Comment 19

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

27 days 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

27 days 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
(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

27 days 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

22 days 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-
(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

21 days 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

21 days 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

21 days ago
Keywords: checkin-needed

Comment 32

21 days 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
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

21 days 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

21 days ago
Flags: needinfo?(mstriemer)
Attachment #8841733 - Flags: review+
Comment hidden (mozreview-request)
(Assignee)

Comment 36

21 days 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

11 days ago
Attachment #8841733 - Flags: review?(florian)
You need to log in before you can comment on or make changes to this bug.