Fixes Issue #871 : port the XUL menu pupup to a WebExtension browserAction popup

[jrrdev]

Tasks status:

• [x] Try to use only HTML elements in the <popupset id="rpcontinuedPopupset"> tree in xul-trees.js
• [ ] Localize the HTML pages (see #881)

Main changelog:

• Creates a set of HTML/CSS/JS files uder src/content/ui/popupto handle the popup. The old XUL element are translated this way:
  • <hbox> and <vbox> elements become <div> with proper CSS to handle size/position
  • <label> elements become <span> or <a>
  • events attributes are now handled by calling addEventListener in popup.js
• The HTML page is embedded in the <menupopup> as a XUL iframe for the legacy extension. Some specific code is present in popup.js to handle correctly the iframe size (to avoid scrollbars)
• In menu.js and overlay.js:
  • Changes $iddefinition to handle seamlessly elements in the iframe's content or in the parent document
  • Uses textContent HTML standard property to write into the <span> elements instead of value property of XUL labels
  • Moves away some properties assignment from init because the iframe's content isn't actually loaded where to XUL page is loaded (i.e loadevent) but when the popup is showing

Tests:

• make lint => OK on the modified files
• make mocha => OK
• make marionette => Didn't run it because of issue #874

Instead of marionette tests, I performed manual tests on Firefox ESR 52.5.1 Nightly & Firefox 56.0b12 Developer Edition. Results were compared with RPC 1.0beta13.2 on my daily Firefox ESR 52.5.0.

• Look'n feels is the same with the modifications
• Popup window's size is properly handled regarding its content
• All allow/deny (permanently/temporarily) links work properly with origin selection
• All allow/deny (permanently/temporarily) links work properly with other origin selection
• All allow/deny (permanently/temporarily) links work properly with destination selection
• All footer links work properly
• Revoke temporarily permission work properly

I did find one bug in Firefox 56.0b12 DE (not in ESR 52.5.1 Nightly), but it is unrelated with this PR because I also encounter it with the develop branch. I may open on issue later on upon further testing.

Notes:

• For the moment, some parts of the UI are unlocalized (i.e in English) because this will be handle later by issue #873
• For the moment, popup.js is just a soft wrapper to handle the popup in a XUL env because the main business code is in menu.jsand overlay.js. popup.html may just work fine out out-of-the-box with WE upon correct entries in manifest.json. But an important point is that menu.js and overlay.js rely way to much on the UI to perform business logic (like retrieving values from the page content to determine which item was click on instead of setting it directly in the listeners). I think this cannot work in WE environment because of the separation between background scripts and UI scripts.

To perform the migration, a lot of it must be rewrite to use the messaging API. I didn't do it for the moment because I don't know all the core code will look like. When performing the business code migration, one option will be:

• In background scripts, sends messages to tell the UI when requests are blocked and catch it in popup.js to populate the UI. Maybe the messages would look like:

  {
  command: "requestBlocked",
  origin: "www.maindomain.test",
  destination: "www.otherdomain.test"
  }

  This way, popup.js would be able to populate all lists on its own. All UI logic may be in this file.

• In popup.js, sends message to tell background scripts to change the policies according to user's interactions:

  {
  command: "allowTemporarily",
  origin: "www.maindomain.test",
  destination: "www.otherdomain.test"
  }
  {
  command: "allowPermanently",
  origin: "www.maindomain.test",
  destination: "<all_urls>"
  }

  Background scripts would then change the policie in the storage. Maybe built-in Match patterns can be used in the messages.

[myrdd]

Hi @jrrdev, thank you for all your work so far. I'm going to review your work and comments this afternoon. Keep it up! :)

[myrdd]

A quick first note: Please keep commit messages short. See git-scm.com.

The last thing to keep in mind is the commit message. Getting in the habit of creating quality commit messages makes using and collaborating with Git a lot easier. As a general rule, your messages should start with a single line that’s no more than about 50 characters and that describes the changeset concisely, followed by a blank line, followed by a more detailed explanation. [...]

Short (50 chars or less) summary of changes

More detailed explanatory text, if necessary.  Wrap it to
about 72 characters or so.  In some contexts, the first
line is treated as the subject of an email and the rest of
the text as the body.  The blank line separating the
summary from the body is critical (unless you omit the body
entirely); tools like rebase can get confused if you run
the two together.

Further paragraphs come after blank lines.

  - Bullet points are okay, too

  - Typically a hyphen or asterisk is used for the bullet,
    preceded by a single space, with blank lines in
    between, but conventions vary here

Could you please reword the commit messages of this PR using git rebase -i develop to use less than 70 characters on each first line? Thanks.

[myrdd]

Could you please reword the commit messages of this PR using git rebase -i develop to use less than 70 characters on each first line?

Never mind about that now, I will maybe squash some commits.

[jrrdev]

Sorry about that, I do realize now at looking at the PR that there is a lot of unnecessary commits in there :-( I can take care of it if you want but only tomorrow.

[myrdd]

Hi Jérard, no worries. The number of commits doesn't matter. Making the commit history "nice" can take a lot of time. (My main motivation for a "good" commit history is to help git detect renamed files. Example: 962dedb9c6ddac2c64a30a31edcbe7360dae9de6, b246f872d10700ecc14c5756daf5fee5b9bcda3e and bdd81736ab536878ad6077c711a02bd92b8fc71e followed by ec544202ab20c2ec5cccd0b0fb58a46091f120fe.)

Let me simply check if your changes are ok (i.e., it works), then we take the commits as they are (only rewording them to reduce line length).

[jrrdev]

I performed the rebasing to reword all commit messages. I also marked some commits as fixup because they were minor fixes of some previous commits. Thanks to this review, I also fixed one minor bug in the new history:

• In popup.html, it was:

  <a href="">Revoke all temporary permissions</a>

• Instead of:

  <a href="#">Revoke all temporary permissions</a>

[myrdd]

Thank you @jrrdev.

For the moment, some parts of the UI are unlocalized (i.e in English) because this will be handle later by issue #873

Let's resolve #873 before merging in this PR.

Regarding messaging between the background page and the popup: note that the popup script can directly access the background page without the need of sending messages. See https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/extension/getBackgroundPage

In background scripts, sends messages to tell the UI when requests are blocked

All data should stay in the background page. The recorded request data is not only needed to populate the menu. In any case, afaik the Browser Action (BA) scripts, i.e. popup.js, is only active when the menu is open.

popup.js would be able to populate all lists on its own. All UI logic may be in this file.

Yes, all code regarding menu population may be in popup.js, or in a module loaded only by popup.js.

an important point is that menu.js and overlay.js rely way to much on the UI to perform business logic

Yes, some refactoring is definitely necessary.

I'll let you know about the next steps.

[myrdd]

Hi @jrrdev Thank you a lot for your work. All your work is now included in the pre-dev release: https://addons.mozilla.org/en-US/firefox/addon/requestpolicy-continued/versions/1.0.beta13.2.2128.r67d9fd50e.pre