1331931 - Move the popup blocked icon into the identity block

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect, P2)

Description

[Hani Yacoub, Desktop QA]

[Affected versions]: 
Nightly 53.0a1

[Affected platforms]:
All platforms: Ubuntu 16.04 x64, Windows 10 x 64, Mac OS X 10.11

[Steps to reproduce]:
1. Go to http://www.popuptest.com/popuptest1.html
2. Click on "Options" button and from the drop down select allow pop-ups for popuptest.com.

[Expected result]:
The popup icon should be displayed near the i icon.

[Actual result]:
The popup icon is displayed in the right side of the url bar.

Comment 1

[Peter Dolanjski [:pdol]]

Philipp, does it make sense to roll the pop-up options into the control center permissions?

Comment 2

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Yeah, it probably makes sense, especially since we are adding a bunch of stuff to the right side of the URL bar with Photon.

Stephen, we should probably figure out the story for privacy/permissions icons sooner rather than later, in case there are any bigger changes...

Comment 3

[Stephen Horlander [:shorlander]]

That was part of the spec AFAIK: https://cl.ly/2g2y0q3I2A2G

Comment 4

[Peter Dolanjski [:pdol]]

(In reply to Stephen Horlander [:shorlander] from comment #3)
> That was part of the spec AFAIK: https://cl.ly/2g2y0q3I2A2G

You're right.  Are we okay to move forward (with moving it into the Control Center) or would we expect change with Photon?

Comment 5

[Stephen Horlander [:shorlander]]

(In reply to Peter Dolanjski [:pdol] from comment #4)
> (In reply to Stephen Horlander [:shorlander] from comment #3)
> > That was part of the spec AFAIK: https://cl.ly/2g2y0q3I2A2G
> 
> You're right.  Are we okay to move forward (with moving it into the Control
> Center) or would we expect change with Photon?

We haven't really looked at that area, but my current thinking is that we won't be making any new recommendations there WRT to how Control Center handles notifications/permissions.

Comment 6

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 6.1; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170328095415

Please also note that if a revoke a previously granted permission for a pop-up (by clicking on the x button near the Open Pop-ups Window in the Control center) then the popup icon is not removed instantly from the URL Bar.

Comment 7

[:prathiksha]

Attachment: Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

Review commit: https://reviewboard.mozilla.org/r/142714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142714/

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review147500

This works great already, now we'll have to make sure that those indicators appear when a popup was blocked for the first time. You can probably steal some of this code https://searchfox.org/mozilla-central/rev/66d9eb3103e429127c85a7921e16c5a02458a127/browser/base/content/browser.js#609 to make that happen, e.g. show the item if gBrowser.selectedBrowser.blockedPopups.length > 0

::: browser/base/content/browser.js:7884
(Diff revision 1)
>  
> +    if (aPermission.id == "popup") {
> +      let menulist = document.createElement("menulist");
> +      let menupopup = document.createElement("menupopup");
> +      for (let state of SitePermissions.getAvailableStates(aPermission.id)) {
> +      let menuitem = document.createElement("menuitem");

Nit: Indent inside loops

::: browser/base/content/browser.js:7891
(Diff revision 1)
> +      menuitem.setAttribute("label", SitePermissions.getMultichoiceStateLabel(state));
> +      menupopup.appendChild(menuitem);
> +      }
> +
> +      menulist.appendChild(menupopup);
> +      menulist.setAttribute("value", SitePermissions.get(gBrowser.currentURI, "popup").state);

You don't have to use SitePermissions.get again, you can just use aPermission.state

::: browser/base/content/browser.js:7897
(Diff revision 1)
> +
> +      menulist.addEventListener('select', function setPermission() {
> +        if (menulist.selectedItem.value == 1)
> +          SitePermissions.set(gBrowser.currentURI, "popup", SitePermissions.ALLOW);
> +        else if (this.selectedItem.value == 2)
> +          SitePermissions.set(gBrowser.currentURI, "popup", SitePermissions.BLOCK);

Can't you reduce this to

SitePermissions.set(gBrowser.currentURI, "popup", menulist.selectedItem.value);

since you set the value to the possible permission states in the loop above?

::: browser/base/content/browser.js:7906
(Diff revision 1)
> +      container.appendChild(nameLabel);
> +      container.appendChild(menulist);
> +
> +      return container;
> +
> +    } else {

Nit: You can just leave out the "else" part since you return anyway.

::: browser/base/content/browser.xul:813
(Diff revision 1)
>                    <image data-permission-id="screen" class="blocked-permission-icon screen-icon" role="button"
>                           tooltiptext="&urlbar.screenBlocked.tooltip;"/>
>                    <image data-permission-id="persistent-storage" class="blocked-permission-icon persistent-storage-icon" role="button"
>                           tooltiptext="&urlbar.persistentStorageBlocked.tooltip;"/>
> +                  <image data-permission-id="popup" class="blocked-permission-icon popup-icon" role="button" id="page-report-button"
> +                         tooltiptext="You have blocked popups for this website."/>

We'll have to add the new text to browser/locales/en-US/chrome/browser/browser.dtd once we have the final copy.

Comment 9

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review147500

Okay. Thank you for the feedback. :)

Comment 10

[:prathiksha]

Hi Jacqueline. We need the permission icon for 'Popups' and the tiny arrow arrow icon from https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/98433851 to clear this bug. Is there someone from UX who can provide me with these ? Thanks!

Comment 11

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142714/diff/1-2/

Comment 12

[Jacqueline Savory [:jsavory] UX]

Hi Prathiksha, I'm not entirely sure who would have the assets for this, but I am sending this one to Bryan as I'm hoping he might have an answer. :)

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review150616

Great progress! When you've resolved these comments you should consider pushing to try to check for failing tests :)

::: browser/base/content/browser.js:7893
(Diff revision 2)
> +      }
> +
> +      menulist.appendChild(menupopup);
> +      menulist.setAttribute("value", aPermission.state);
> +
> +      menulist.addEventListener('select', function setPermission() {

Nit: I don't think this function needs a name.

::: browser/base/content/browser.js:7901
(Diff revision 2)
> +
> +      container.appendChild(img);
> +      container.appendChild(nameLabel);
> +      container.appendChild(menulist);
> +
> +      if (!gBrowser.selectedBrowser.blockedPopups ||

You need to do this check outside of this function as well, to make sure to show the popup permission section when a popup has been blocked.

It would probably be better to re-structure this function a bit and move the blocked-popup related parts out so that it can be called separately, e.g. around line 7838.

::: browser/base/content/browser.js:7917
(Diff revision 2)
> +
> +      let text = document.createElement("label");
> +      text.setAttribute("flex", "1");
> +      text.setAttribute("class", "identity-popup-permission-label");
> +      text.setAttribute("id", "indicator-text");
> +      text.textContent = "Show " + gBrowser.selectedBrowser.blockedPopups.length +

I would recommend moving this text to the localization files (browser/locales/en-US/chrome/browser/browser.(dtd|properties)) rather sooner than later since you won't get an r+ with hardcoded text :)

We should be able to re-use the copy from the mockup and the existing popup icon and needinfo mheubusch once everything is in place to sign the text off.

::: browser/base/content/browser.js:7919
(Diff revision 2)
> +      text.setAttribute("flex", "1");
> +      text.setAttribute("class", "identity-popup-permission-label");
> +      text.setAttribute("id", "indicator-text");
> +      text.textContent = "Show " + gBrowser.selectedBrowser.blockedPopups.length +
> +                         " blocked popups...";
> +      text.setAttribute("onclick",

Can you call text.addEventListener here instead of using onclick?

Comment 14

[bbell]

The assets you need are here now: https://drive.google.com/drive/folders/0B6F4iApRJzosVTdaZjN3UHY5NG8?usp=sharing

I also included replacements for the rest of the icons used in the Control Center. Please let me know if you have trouble implementing them.

Comment 15

[:prathiksha]

(In reply to bbell from comment #14)
> The assets you need are here now:
> https://drive.google.com/drive/folders/
> 0B6F4iApRJzosVTdaZjN3UHY5NG8?usp=sharing
> 
> I also included replacements for the rest of the icons used in the Control
> Center. Please let me know if you have trouble implementing them.

Thank you.

Comment 16

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142714/diff/2-3/

Comment 17

[Johann Hofmann [:johannh]]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review168508

::: browser/base/content/browser.js:910
(Diff revision 3)
>      var showMessage = gPrefService.getBoolPref("privacy.popups.showBrowserMessage");
>      gPrefService.setBoolPref("privacy.popups.showBrowserMessage", !showMessage);
>      gBrowser.getNotificationBox().removeCurrentNotification();
> -  }
> +  },
> +
> +  setPopupIndicatorItem() {

This method should probably rather be on gIdentityHandler, since that handles changing the identity UI. What do you think?

::: browser/base/content/browser.js:925
(Diff revision 3)
> +    text.setAttribute("flex", "1");
> +    text.setAttribute("class", "identity-popup-permission-label");
> +    text.setAttribute("id", "indicator-text");
> +    text.setAttribute("value", gNavigatorBundle.getString("popupShowBlockedPopupPrefix") +
> +                               " " + gBrowser.selectedBrowser.blockedPopups.length + " " +
> +                               gNavigatorBundle.getString("popupShowBlockedPopupPostfix"));

This won't work with localization. The text isn't guaranteed to be structured like this in other locales. You can use gNavigatorBundle.getFormattedString and pass the number of popups as a parameter.

::: browser/base/content/browser.js:7902
(Diff revision 3)
>      }
>  
> +    let blockedPopups = gBrowser.selectedBrowser.blockedPopups !== null;
> +    if (blockedPopups) {
> +      if (gBrowser.selectedBrowser.blockedPopups.length) {
> +        gPopupBlockerObserver.setPopupIndicatorItem();

This code implicitly relies on the code you added in _createPermissionItem to have run. You should make a new function for the code you added in _createPermissionItem and call it here if needed.

::: browser/base/content/browser.xul:844
(Diff revision 3)
>                           tooltiptext="&urlbar.microphoneBlocked.tooltip;"/>
>                    <image data-permission-id="screen" class="blocked-permission-icon screen-icon" role="button"
>                           tooltiptext="&urlbar.screenBlocked.tooltip;"/>
>                    <image data-permission-id="persistent-storage" class="blocked-permission-icon persistent-storage-icon" role="button"
>                           tooltiptext="&urlbar.persistentStorageBlocked.tooltip;"/>
> +                  <image data-permission-id="popup" class="blocked-permission-icon popup-icon" role="button" id="page-report-button"

Giving this an id="page-report-button" doesn't do what you want it to do.

You should go through the code at https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/browser/base/content/browser.js#635 and check what needs to change (e.g. we should set the "showing" attribute instead of "hidden") and what we can remove (essentially everything that deals with clicking the page-report-button).

Feel free to ping me on IRC if you have questions on that.

::: browser/themes/shared/notification-icons.svg:48
(Diff revision 3)
>      <path id="microphone-icon" d="m 8,14 0,4 a 8,8 0 0 0 6,7.7 l 0,2.3 -2,0 a 2,2 0 0 0 -2,2 l 12,0 a 2,2 0 0 0 -2,-2 l -2,0 0,-2.3 a 8,8 0 0 0 6,-7.7 l 0,-4 -2,0 0,4 a 6,6 0 0 1 -12,0 l 0,-4 z m 4,4 a 4,4 0 0 0 8,0 l 0,-12 a 4,4 0 0 0 -8,0 z" />
>      <path id="microphone-detailed-icon" d="m 8,18 a 8,8 0 0 0 6,7.7 l 0,2.3 -1,0 a 3,2 0 0 0 -3,2 l 12,0 a 3,2 0 0 0 -3,-2 l -1,0 0,-2.3 a 8,8 0 0 0 6,-7.7 l 0,-4 a 1,1 0 0 0 -2,0 l 0,4 a 6,6 0 0 1 -12,0 l 0,-4 a 1,1 0 0 0 -2,0 z m 4,0 a 4,4 0 0 0 8,0 l 0,-12 a 4,4 0 0 0 -8,0 z" />
>      <path id="persistent-storage-icon" d="M26 21.1H6c-1.1 0-2 .9-2 2V27c0 1.1.9 2 2 2h20c1.1 0 2-.9 2-2v-3.9c0-1.1-.9-2-2-2zM24.1 27c-1.1 0-2-.9-2-2s.9-2 2-2 2 .9 2 2-.9 2-2 2zM25 3H7C5.3 3 4 4.4 4 6.2v13.3c.6-.3 1.3-.5 2-.5h20c.7 0 1.4.2 2 .5V6.2C28 4.4 26.7 3 25 3z"/>
>      <path id="plugin-icon" d="m 2,26 a 2,2 0 0 0 2,2 l 24,0 a 2,2 0 0 0 2,-2 l 0,-16 a 2,2 0 0 0 -2,-2 l -24,0 a 2,2 0 0 0 -2,2 z m 2,-20 10,0 0,-2 a 2,2 0 0 0 -2,-2 l -6,0 a 2,2 0 0 0 -2,2 z m 14,0 10,0 0,-2 a 2,2 0 0 0 -2,-2 l -6,0 a 2,2 0 0 0 -2,2 z" />
>      <path id="popup-icon" d="m 2,24 a 4,4 0 0 0 4,4 l 8,0 a 10,10 0 0 1 -2,-4 l -4,0 a 2,2 0 0 1 -2,-2 l 0,-12 18,0 0,2 a 10,10 0 0 1 4,2 l 0,-8 a 4,4 0 0 0 -4,-4 l -18,0 a 4,4 0 0 0 -4,4 z m 12,-2.1 a 8,8 0 1 1 0,0.2 m 10.7,-4.3 a 5,5 0 0 0 -6.9,6.9 z m -5.4,8.4 a 5,5 0 0 0 6.9,-6.9 z" />
> +    <path id="popup-arrow-icon" d="M13.707 7.293l-3-3a1 1 0 0 0-1.414 1.414L10.586 7H6.012A.918.918 0 0 1 5 6V3a1 1 0 0 0-2 0v3a2.916 2.916 0 0 0 3 3h4.586l-1.293 1.293a1 1 0 1 0 1.414 1.414l3-3a1 1 0 0 0 0-1.414z" />

We're splitting this file up in bug 1371230, so it's probably best to put this in its own SVG file.

Comment 18

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review168508

> This method should probably rather be on gIdentityHandler, since that handles changing the identity UI. What do you think?

Yes. That makes more sense. :)

Comment 19

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

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

Comment 20

[Johann Hofmann [:johannh]]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review170894

Note that you will also need to add a check for gBrowser.selectedBrowser.blockedPopups && gBrowser.selectedBrowser.blockedPopups.length somewhere around https://searchfox.org/mozilla-central/rev/b52285fffc13f36eca6b47de735d4e4403b3859e/browser/base/content/browser.js#7517 to display the popup permission icon even if there's no permission set (only popups were blocked).

::: browser/base/content/browser.js:871
(Diff revision 4)
> +  },
> +
> +  checkForBlockedPopups() {
> +    let blockedPopups = gBrowser.selectedBrowser.blockedPopups !== null;
> +    if (blockedPopups) {
> +      if (gBrowser.selectedBrowser.blockedPopups.length)

You can shorten this function to return gBrowser.selectedBrowser.blockedPopups && gBrowser.selectedBrowser.blockedPopups.length, at which point I don't think it makes sense to make it a separate function...

::: browser/base/content/browser.js:7860
(Diff revision 4)
> +        !document.getElementById("popupContainer")) {
> +      let permission = {
> +        id: "popup",
> +        state: SitePermissions.BLOCK,
> +        scope: SitePermissions.SCOPE_PERSISTENT,
> +      }

Nit: semicolon

::: browser/base/content/browser.js:7897
(Diff revision 4)
>  
> +    if (aPermission.id == "popup") {
> +      let menulist = document.createElement("menulist");
> +      let menupopup = document.createElement("menupopup");
> +      let block = document.createElement("vbox");
> +      block.setAttribute("id", "popupContainer");

Please give this an id that's consistent with the other ids, such as identity-popup-popup-container.

::: browser/base/content/browser.js:8004
(Diff revision 4)
>      container.appendChild(button);
>  
>      return container;
> +  },
> +
> +  _setPopupIndicatorItem() {

I'm not sure I understand what _setPopupIndicatorItem as a function name means. Maybe _createBlockedPopupIndicator?

::: browser/base/content/browser.js:8008
(Diff revision 4)
> +
> +  _setPopupIndicatorItem() {
> +    let indicator = document.createElement("hbox");
> +    indicator.setAttribute("class", "identity-popup-permission-item");
> +    indicator.setAttribute("align", "center");
> +    indicator.setAttribute("id", "popup-indicator");

Is this id used anywhere?

::: browser/base/content/browser.js:8019
(Diff revision 4)
> +    text.setAttribute("flex", "1");
> +    text.setAttribute("class", "identity-popup-permission-label");
> +    text.setAttribute("id", "indicator-text");
> +    text.setAttribute("value",
> +      gNavigatorBundle.getFormattedString("popupShowBlockedPopupsIndicatorText",
> +        [gBrowser.selectedBrowser.blockedPopups.length]));

You will need to get the correct plural form of your string using PluralForm. You can see how the original popup blocker code does it here: https://searchfox.org/mozilla-central/rev/b52285fffc13f36eca6b47de735d4e4403b3859e/browser/base/content/browser.js#692

::: browser/base/content/browser.js:8027
(Diff revision 4)
> +    });
> +
> +    indicator.appendChild(arrowIcon);
> +    indicator.appendChild(text);
> +
> +    if (document.getElementById("popupContainer")) {

Is it possible/logical to call this function without the popup container being present?

::: browser/themes/shared/controlcenter/panel.inc.css:439
(Diff revision 4)
>    min-width: 0;
>    padding: 2px;
>    background-color: transparent;
>  }
>  
> +#indicator-text {

You don't need to style this yourself. It will apply the correct style when you give it the text-link class.

::: browser/themes/shared/notification-icons.inc.css:171
(Diff revision 4)
>  /* This icon has a block sign in it, so we don't need a blocked version. */
>  .popup-icon {
>    list-style-image: url("chrome://browser/skin/notification-icons.svg#popup");
>  }
>  
> +.arrow-icon {

arrow-icon is quite specific to the form of this thing, what if we want to change the icon? Maybe something like blocked-popup-indicator-icon would be better?

::: browser/themes/shared/notification-icons.svg:48
(Diff revision 4)
>      <path id="microphone-icon" d="m 8,14 0,4 a 8,8 0 0 0 6,7.7 l 0,2.3 -2,0 a 2,2 0 0 0 -2,2 l 12,0 a 2,2 0 0 0 -2,-2 l -2,0 0,-2.3 a 8,8 0 0 0 6,-7.7 l 0,-4 -2,0 0,4 a 6,6 0 0 1 -12,0 l 0,-4 z m 4,4 a 4,4 0 0 0 8,0 l 0,-12 a 4,4 0 0 0 -8,0 z" />
>      <path id="microphone-detailed-icon" d="m 8,18 a 8,8 0 0 0 6,7.7 l 0,2.3 -1,0 a 3,2 0 0 0 -3,2 l 12,0 a 3,2 0 0 0 -3,-2 l -1,0 0,-2.3 a 8,8 0 0 0 6,-7.7 l 0,-4 a 1,1 0 0 0 -2,0 l 0,4 a 6,6 0 0 1 -12,0 l 0,-4 a 1,1 0 0 0 -2,0 z m 4,0 a 4,4 0 0 0 8,0 l 0,-12 a 4,4 0 0 0 -8,0 z" />
>      <path id="persistent-storage-icon" d="M26 21.1H6c-1.1 0-2 .9-2 2V27c0 1.1.9 2 2 2h20c1.1 0 2-.9 2-2v-3.9c0-1.1-.9-2-2-2zM24.1 27c-1.1 0-2-.9-2-2s.9-2 2-2 2 .9 2 2-.9 2-2 2zM25 3H7C5.3 3 4 4.4 4 6.2v13.3c.6-.3 1.3-.5 2-.5h20c.7 0 1.4.2 2 .5V6.2C28 4.4 26.7 3 25 3z"/>
>      <path id="plugin-icon" d="m 2,26 a 2,2 0 0 0 2,2 l 24,0 a 2,2 0 0 0 2,-2 l 0,-16 a 2,2 0 0 0 -2,-2 l -24,0 a 2,2 0 0 0 -2,2 z m 2,-20 10,0 0,-2 a 2,2 0 0 0 -2,-2 l -6,0 a 2,2 0 0 0 -2,2 z m 14,0 10,0 0,-2 a 2,2 0 0 0 -2,-2 l -6,0 a 2,2 0 0 0 -2,2 z" />
>      <path id="popup-icon" d="m 2,24 a 4,4 0 0 0 4,4 l 8,0 a 10,10 0 0 1 -2,-4 l -4,0 a 2,2 0 0 1 -2,-2 l 0,-12 18,0 0,2 a 10,10 0 0 1 4,2 l 0,-8 a 4,4 0 0 0 -4,-4 l -18,0 a 4,4 0 0 0 -4,4 z m 12,-2.1 a 8,8 0 1 1 0,0.2 m 10.7,-4.3 a 5,5 0 0 0 -6.9,6.9 z m -5.4,8.4 a 5,5 0 0 0 6.9,-6.9 z" />
> +    <path id="popup-arrow-icon" d="M13.707 7.293l-3-3a1 1 0 0 0-1.414 1.414L10.586 7H6.012A.918.918 0 0 1 5 6V3a1 1 0 0 0-2 0v3a2.916 2.916 0 0 0 3 3h4.586l-1.293 1.293a1 1 0 1 0 1.414 1.414l3-3a1 1 0 0 0 0-1.414z" />

Did you see my previous comment? :)

This needs to be in its own file.

Comment 21

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review170894

I've done this. I think you missed it. :)

> Is this id used anywhere?

yes

Comment 22

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142714/diff/4-5/

Comment 23

[Tim Nguyen :ntim]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review174956

::: browser/themes/shared/blocked-popup-indicator-icon.svg:1
(Diff revision 5)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg fill="context-fill" fill-opacity="context-fill-opacity" xmlns="http://www.w3.org/2000/svg"
> +     width="32" height="32" viewBox="0 0 32 32">
> +
> +  <defs>
> +    <path id="blocked-popup-indicator-icon" d="M13.707 7.293l-3-3a1 1 0 0 0-1.414 1.414L10.586 7H6.012A.918.918 0 0 1 5 6V3a1 1 0 0 0-2 0v3a2.916 2.916 0 0 0 3 3h4.586l-1.293 1.293a1 1 0 1 0 1.414 1.414l3-3a1 1 0 0 0 0-1.414z" />
> +  </defs>
> +
> +  <use id="popup-indicator-icon" href="#blocked-popup-indicator-icon" />
> +
> +</svg>

Can you use this icon instead:

<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - License, v. 2.0. If a copy of the MPL was not distributed with this
   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" fill="context-fill">
  <path d="M12 8a4 4 0 1 0 4 4 4 4 0 0 0-4-4zm0 1a2.975 2.975 0 0 1 1.733.56L9.56 13.733A2.991 2.991 0 0 1 12 9zm0 6a2.975 2.975 0 0 1-1.733-.56l4.173-4.173A2.991 2.991 0 0 1 12 15z"/>
  <path d="M13 1H3a3 3 0 0 0-3 3v8a3 3 0 0 0 3 3h3a1 1 0 0 0 0-2H3a1 1 0 0 1-1-1V6h12a1 1 0 0 0 2 0V4a3 3 0 0 0-3-3zM2 5V4a1 1 0 0 1 1-1h10a1 1 0 0 1 1 1v1z"/>
</svg>

::: browser/themes/shared/notification-icons.inc.css:172
(Diff revision 5)
>  .popup-icon {
>    list-style-image: url("chrome://browser/skin/notification-icons.svg#popup");
>  }
>  
> +.blocked-popup-indicator-icon {
> +  list-style-image: url("chrome://browser/skin/blocked-popup-indicator-icon.svg#popup-indicator-icon");

no need for the #popup-indicator-icon hash at the end

Comment 24

[Johann Hofmann [:johannh]]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review174956

> Can you use this icon instead:
> 
> <!-- This Source Code Form is subject to the terms of the Mozilla Public
>    - License, v. 2.0. If a copy of the MPL was not distributed with this
>    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" fill="context-fill">
>   <path d="M12 8a4 4 0 1 0 4 4 4 4 0 0 0-4-4zm0 1a2.975 2.975 0 0 1 1.733.56L9.56 13.733A2.991 2.991 0 0 1 12 9zm0 6a2.975 2.975 0 0 1-1.733-.56l4.173-4.173A2.991 2.991 0 0 1 12 15z"/>
>   <path d="M13 1H3a3 3 0 0 0-3 3v8a3 3 0 0 0 3 3h3a1 1 0 0 0 0-2H3a1 1 0 0 1-1-1V6h12a1 1 0 0 0 2 0V4a3 3 0 0 0-3-3zM2 5V4a1 1 0 0 1 1-1h10a1 1 0 0 1 1 1v1z"/>
> </svg>

Note that this was added in bug 1392416, so I don't think you need to do anything.

Comment 25

[Dão Gottwald [:dao]]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review182292

drive-by review:

::: browser/base/content/browser.js:7440
(Diff revision 5)
>  
> +    // Show blocked popup icon in the identity-box when popups are blocked
> +    // irrespective of popup permission capability value.
> +    let blockedPopups = gBrowser.selectedBrowser.blockedPopups !== null;
> +    if (blockedPopups) {
> +      if (gBrowser.selectedBrowser.blockedPopups.length > 0) {

if (gBrowser.selectedBrowser.blockedPopups &&
    gBrowser.selectedBrowser.blockedPopups.length) {

::: browser/locales/en-US/chrome/browser/browser.properties:252
(Diff revision 5)
>  popupAllow=Allow pop-ups for %S
>  popupBlock=Block pop-ups for %S
>  popupWarningDontShowFromMessage=Don’t show this message when pop-ups are blocked
> -popupWarningDontShowFromLocationbar=Don’t show info bar when pop-ups are blocked
>  popupShowPopupPrefix=Show ‘%S’
> +popupShowBlockedPopupsIndicatorText=Show #1 blocked popup...;Show #1 blocked popups...

… instead of ...

Comment 26

[Johann Hofmann [:johannh]]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review182508

I'm really sorry for the long wait, I should have delegated this during my PTO. I heard that at least your exams were keeping you busy :)

There should be some minor merge conflicts when rebasing this on central.

The way that you show the indicator looks good to me now (just a couple of nits), but I'm still unclear on how you intend to show the blocked icon in the identity block when a popup was blocked (without the permission being modified or the identity popup being shown). For that we probably need to call gIdentityHandler.refreshIdentityBlock() in gPopupBlockerObserver.handleEvent.

Also, please do a try run for this. I can guarantee that you will have breaking tests and those need to be fixed up (and you should consider writing new ones for the identity popup parts).

Again, let me know if there's anything I can help you with.

Thank you for your work!

::: browser/base/content/browser.js:7803
(Diff revision 5)
>            }
>          }
>        }
>      }
> +
> +    let blockedPopups = gBrowser.selectedBrowser.blockedPopups !== null;

Nit: I'd find it a bit nicer if you wrote:

let blockedPopups = gBrowser.selectedBrowser.blockedPopups;

then you can continue to use blockedPopups as a bool-ish value but could avoid spelling out gBrowser.selectedBrowser.blockedPopups everywhere else.

But I'd leave that up to you, your version wfm as well.

::: browser/base/content/browser.js:7809
(Diff revision 5)
>      for (let permission of permissions) {
>        let item = this._createPermissionItem(permission);
>        this._permissionList.appendChild(item);
> +
> +      if (permission.id == "popup" && blockedPopups) {
> +        if (gBrowser.selectedBrowser.blockedPopups.length > 0) {

Nit: you don't need to have two if clauses here

::: browser/base/content/browser.js:7815
(Diff revision 5)
> +          this._createBlockedPopupIndicator();
> +        }
> +      }
> +    }
> +
> +    if (blockedPopups && !document.getElementById("blocked-popup-indicator-item")) {

As a suggestion, instead of checking this id you could use a simple boolean variable "hasBlockedPopupIndicator" and set that to true in the if condition above. That sounds a little simpler to me, personally.

::: browser/base/content/browser.js:7816
(Diff revision 5)
> +        }
> +      }
> +    }
> +
> +    if (blockedPopups && !document.getElementById("blocked-popup-indicator-item")) {
> +        if (gBrowser.selectedBrowser.blockedPopups.length > 0) {

Nit: two if clauses

::: browser/themes/shared/blocked-popup-indicator-icon.svg:5
(Diff revision 5)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg fill="context-fill" fill-opacity="context-fill-opacity" xmlns="http://www.w3.org/2000/svg"
> +     width="32" height="32" viewBox="0 0 32 32">

If you replace the 32 with 16 you should be able to remove the -moz-image-region CSS rule.

::: browser/themes/shared/blocked-popup-indicator-icon.svg:11
(Diff revision 5)
> +
> +  <defs>
> +    <path id="blocked-popup-indicator-icon" d="M13.707 7.293l-3-3a1 1 0 0 0-1.414 1.414L10.586 7H6.012A.918.918 0 0 1 5 6V3a1 1 0 0 0-2 0v3a2.916 2.916 0 0 0 3 3h4.586l-1.293 1.293a1 1 0 1 0 1.414 1.414l3-3a1 1 0 0 0 0-1.414z" />
> +  </defs>
> +
> +  <use id="popup-indicator-icon" href="#blocked-popup-indicator-icon" />

You can remove this and just put the path out of the defs

::: browser/themes/shared/notification-icons.inc.css:174
(Diff revision 5)
>  }
>  
> +.blocked-popup-indicator-icon {
> +  list-style-image: url("chrome://browser/skin/blocked-popup-indicator-icon.svg#popup-indicator-icon");
> +  margin-left: 32px;
> +  -moz-image-region: rect(0px, 16px, 16px, 0px);

As mentioned above, you can remove this.

Comment 27

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142714/diff/5-6/

Comment 28

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142714/diff/6-7/

Comment 29

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142714/diff/7-8/

Comment 30

[Johann Hofmann [:johannh]]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review188706

No more larger issues, thanks for including the test!

There's one more thing we need to change, unfortunately.

You might have noticed bug 1404262, which causes menulists to trigger a select even on creation, which in turn means that we're creating a BLOCK entry for popups whenever the user opens the control center after a popup was blocked. We should probably avoid using the "select" event until that issue is cleared up. I also think we should set the permission state to UNKNOWN instead of BLOCK when the "Block" option is selected (which should have the same effect).

Let me know what you think.

Thank you!

::: browser/base/content/browser.js:7528
(Diff revision 8)
> +    // Show blocked popup icon in the identity-box if popups are blocked
> +    // irrespective of popup permission capability value.
> +    if (gBrowser.selectedBrowser.blockedPopups &&
> +        gBrowser.selectedBrowser.blockedPopups.length) {
> +      let icon = permissionAnchors["popup"];
> +      if (icon) {

Nit: I don't think there's a way that this icon can not exist, right?

::: browser/base/content/test/popups/browser.ini:13
(Diff revision 8)
>  [browser_popup_frames.js]
>  support-files =
>    popup_blocker.html
>    popup_blocker_a.html
>    popup_blocker_b.html
> +[browser_popup_blocker2.js]

How about calling this one browser_popup_identity_block or something? :)

::: browser/base/content/test/popups/browser_popup_blocker2.js:57
(Diff revision 8)
> +
> +  // Check if blocked popup icon is visible in the identity block.
> +  Assert.equal(icon.getAttribute("showing"), "true");
> +
> +  gBrowser.removeTab(tab);
> +});

I think this file should also check that the menulist has the correct value initially and that it can be flipped to change the popup blocking behavior.

It should also test that you can select the "Show ... popups" link, which should show the popup.

::: browser/base/content/test/popups/popup_blocker2.html:8
(Diff revision 8)
> +  <head>
> +    <meta charset="UTF-8">
> +    <title>Page creating a popup</title>
> +  </head>
> +  <body>
> +    <button id="pop" onclick='window.setTimeout(() => {window.open("popup_blocker_a.html", "a");}, 1000);'>Open Popup</button>

Nit: I don't think that you need such a large timeout with the correct prefs set in the test file (you might not need a timeout at all).

::: browser/themes/shared/jar.inc.mn:78
(Diff revision 8)
>    skin/classic/browser/notification-icons/persistent-storage.svg            (../shared/notification-icons/persistent-storage.svg)
>    skin/classic/browser/notification-icons/plugin-badge.svg                  (../shared/notification-icons/plugin-badge.svg)
>    skin/classic/browser/notification-icons/plugin-blocked.svg                (../shared/notification-icons/plugin-blocked.svg)
>    skin/classic/browser/notification-icons/plugin.svg                        (../shared/notification-icons/plugin.svg)
>    skin/classic/browser/notification-icons/popup.svg                         (../shared/notification-icons/popup.svg)
> +  skin/classic/browser/notification-icons/blocked-popup-indicator-icon.svg  (../shared/notification-icons/blocked-popup-indicator-icon.svg)

What do you think about renaming this icon? indicator-icon doesn't really describe its purpose well enough for me.

The original file name was subitem-16.svg, how about naming this one popup-subitem.svg or similar?

::: browser/themes/shared/notification-icons.inc.css:157
(Diff revision 8)
>  
> +.blocked-popup-indicator-icon {
> +  list-style-image: url("chrome://browser/skin/notification-icons/blocked-popup-indicator-icon.svg");
> +  margin-inline-start: 30px;
> +  -moz-context-properties: fill;
> +  fill: GrayText;

You should probably share the latter part of that rule with https://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/browser/themes/shared/notification-icons.inc.css#8

::: browser/themes/shared/notification-icons/blocked-popup-indicator-icon.svg:5
(Diff revision 8)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" fill="context-fill" fill-opacity="context-fill-opacity">
> +  <path fill="context-fill" d="M13.707 7.293l-3-3a1 1 0 0 0-1.414 1.414L10.586 7H6.012A.918.918 0 0 1 5 6V3a1 1 0 0 0-2 0v3a2.916 2.916 0 0 0 3 3h4.586l-1.293 1.293a1 1 0 1 0 1.414 1.414l3-3a1 1 0 0 0 0-1.414z"/>

Nit: you don't need context-fill on the path if you have it globally on the svg already.

Comment 31

[Tim Nguyen :ntim]

> What do you think about renaming this icon? indicator-icon doesn't really describe its purpose well enough for me.

> The original file name was subitem-16.svg, how about naming this one popup-subitem.svg or similar?

Why is this icon used in the first place ?

Comment 32

[Johann Hofmann [:johannh]]

(In reply to Tim Nguyen :ntim from comment #31)
> > What do you think about renaming this icon? indicator-icon doesn't really describe its purpose well enough for me.
> 
> > The original file name was subitem-16.svg, how about naming this one popup-subitem.svg or similar?
> 
> Why is this icon used in the first place ?

Do display that there is a sub-item in the permissions section of the identity popup, it's part of the design spec.

Comment 33

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142714/diff/8-9/

Comment 34

[Johann Hofmann [:johannh]]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review196744

Just a bit of cleanup and we should be good to go! Thanks!!

::: browser/base/content/browser.js:7554
(Diff revision 9)
>  
> +    // Show blocked popup icon in the identity-box if popups are blocked
> +    // irrespective of popup permission capability value.
> +    if (gBrowser.selectedBrowser.blockedPopups &&
> +        gBrowser.selectedBrowser.blockedPopups.length) {
> +      let icon = permissionAnchors["popup"];

ESLint complains that you should write this in dot notation (permissionAnchors.popup). I don't really care (I actually find this syntax nicer since permissionAnchors is more of a dictionary), but we shouldn't fight ESLint over this. :)

::: browser/base/content/browser.js:7909
(Diff revision 9)
>            }
>          }
>        }
>      }
> +
> +    let hasIndicator;

I'd give this a little more descriptive name, such as hasBlockedPopupIndicator and initialize it with = false.

::: browser/base/content/browser.js:7933
(Diff revision 9)
> +        scope: SitePermissions.SCOPE_PERSISTENT,
> +      };
> +      let item = this._createPermissionItem(permission);
> +      this._permissionList.appendChild(item);
> +      this._createBlockedPopupIndicator();
> +      hasIndicator = true;

Why are you setting hasIndicator here? Is it used afterwards?

::: browser/base/content/browser.js:7980
(Diff revision 9)
> +        menupopup.appendChild(menuitem);
> +      }
> +
> +      menulist.appendChild(menupopup);
> +      if (aPermission.state == 0) {
> +        menulist.setAttribute("value", 2);

I don't think you don't have to do this anymore, because my changes from bug 1379560 made it so that SitePermissions.set will automatically remove the permission if its passed the default permission.

https://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/browser/modules/SitePermissions.jsm#384

::: browser/base/content/browser.js:7985
(Diff revision 9)
> +        menulist.setAttribute("value", 2);
> +      } else {
> +        menulist.setAttribute("value", aPermission.state);
> +      }
> +
> +      // Avoiding listening to the "select" event on purpose. See Bug 1404262.

Thanks for that comment! :)

::: browser/base/content/browser.js:7988
(Diff revision 9)
> +      }
> +
> +      // Avoiding listening to the "select" event on purpose. See Bug 1404262.
> +      menulist.addEventListener("command", () => {
> +        if (menulist.selectedItem.value == 2) {
> +          SitePermissions.remove(gBrowser.currentURI, "popup");

See above, I believe you can simplify this code again. Can you check if that works? :)

::: browser/base/content/test/popups/browser_popup_blocker_identity_block.js:80
(Diff revision 9)
> +  // Wait for popup block.
> +  await BrowserTestUtils.waitForCondition(() =>
> +    gBrowser.getNotificationBox().getNotificationWithValue("popup-blocked"));
> +
> +  // Store the popup that opens in this array.
> +  let popup = [];

Why are you using an array instead of a simple variable here?

::: browser/base/content/test/popups/browser_popup_blocker_identity_block.js:92
(Diff revision 9)
> +  await openIdentityPopup();
> +  let e = document.getElementById("blocked-popup-indicator-item");
> +  let text = e.getElementsByTagName("label")[0];
> +  text.click();
> +
> +  await BrowserTestUtils.waitForCondition(() =>

Can't you do something like:

let event = await BTU.waitForEvent(gBrowser.tabContainer, "TabOpen");

await BTU.waitForCondition(popup[0].linkedBrowser.currentURI.spec != "about:blank");

instead?

I'm ok with this code as it is but I'd say using BTU.waitForEvent looks a little cleaner to me. Up to you :)

::: browser/base/content/test/popups/browser_popup_blocker_identity_block.js:132
(Diff revision 9)
> +
> +  state = SitePermissions.get(URI, "popup", gBrowser).state;
> +  Assert.equal(state, SitePermissions.ALLOW);
> +
> +  // Store the popup that opens in this array.
> +  let popup = [];

See above, why are you using an array instead of a simple variable here?

Comment 35

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142714/diff/9-10/

Comment 36

[Johann Hofmann [:johannh]]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review196888

There are some nits left but nothing that would prevent an r+

Thank you so much for all your work on this.

::: browser/base/content/browser.js:7985
(Diff revision 10)
>            }
>          }
>        }
>      }
> +
> +    let hasBlockedPopupIndicator;

Nit: please initialize this to false.

::: browser/base/content/browser.js:8009
(Diff revision 10)
> +        scope: SitePermissions.SCOPE_PERSISTENT,
> +      };
> +      let item = this._createPermissionItem(permission);
> +      this._permissionList.appendChild(item);
> +      this._createBlockedPopupIndicator();
> +      hasBlockedPopupIndicator = true;

Nit: Please remove this :)

::: browser/base/content/browser.js:8049
(Diff revision 10)
> +      menulist.setAttribute("class", "identity-popup-popup-menulist");
> +      menulist.setAttribute("id", "identity-popup-popup-menulist");
> +
> +      for (let state of SitePermissions.getAvailableStates(aPermission.id)) {
> +        let menuitem = document.createElement("menuitem");
> +        if (state == SitePermissions.getDefault(aPermission.id)) {

Are you sure that you have to do this? SitePermissions.set should be smart enough to handle non 0 default values and remove the permission...

https://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/browser/modules/SitePermissions.jsm#384

::: browser/locales/en-US/chrome/browser/browser.properties:268
(Diff revision 10)
>  popupAllow=Allow pop-ups for %S
>  popupBlock=Block pop-ups for %S
>  popupWarningDontShowFromMessage=Don’t show this message when pop-ups are blocked
> -popupWarningDontShowFromLocationbar=Don’t show info bar when pop-ups are blocked
>  popupShowPopupPrefix=Show ‘%S’
> +popupShowBlockedPopupsIndicatorText=Show #1 blocked popup…;Show #1 blocked popups…

Nit: You should add a localization note here.

Comment 37

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

https://reviewboard.mozilla.org/r/142714/#review196888

> Are you sure that you have to do this? SitePermissions.set should be smart enough to handle non 0 default values and remove the permission...
> 
> https://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/browser/modules/SitePermissions.jsm#384

Keeping this in place so we can easily set the menulist value as "Block" when the permission state is UNKNOWN and popups are blocked.

Comment 38

[:prathiksha]

Comment on attachment 8871231 [details]
Bug 1331931 - Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142714/diff/10-11/

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/328f5e2f6939
Move the blocked popup icon into the identity block and add a blocked popup indicator item to the identity popup. r=johannh

Comment 40

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/328f5e2f6939

Comment 41

[Francesco Lodolo [:flod]]

Hi Stephen, asking you since you put the link in comment 3 (feel free to redirect the NI)
https://cl.ly/2g2y0q3I2A2G

> You have blocked popups for this website.
> Show #1 blocked popup…;Show #1 blocked popups…

This is not how "pop-up" has been spelled in Firefox so far, and I wonder if anything changed or this is a consistency error.

https://transvision.mozfr.org/?recherche=pop-up&repo=gecko_strings&sourcelocale=en-US&locale=en-US&search_type=strings_entities

The only occurrences of popup are in devtools (1), mobile (1), dom (2, but for menus, not windows)

No mention of pop-up or popup here either
http://design.firefox.com/photon/copy/word-list.html

Comment 42

[Tim Nguyen :ntim]

Needinfo'ing :mheubusch who might be able to answer comment 41.

Comment 43

[mheubusch]

Use "pop-up" for adjectives and nouns. Use "pop up" (space and no hyphen)  for verbs.  

I will add this to the style guide. Note that this is also the preferred spelling in our standard online dictionary https://www.merriam-webster.com/ as well as most other dictionaries.  

Thanks for looping me in, Flod!

Comment 44

[Johann Hofmann [:johannh]]

Great, thanks for clarifying! Anyone mind filing a follow-up bug?