Added support for firefox on android #1516

lemnis · 2017-07-19T02:50:54Z

Tested in Firefox for Android 56.0a1 (2017-07-18), should work from 55+. Did check chrome and firefox desktop, didn't see anything unexpected.

See for the full conversation #1084

popup.css :29, :30, :298; :298 fixes the issues that the overlay became wider than the popup, as result :29 and :30 is not needed anymore and reenables the possibility to scroll (needed on mobile) :21 - : 30; making sure that the popup is rendered on the max width of the device :194; fixes a issue where the header didn’t keep the correct place when zoomed in within firefox options.html :24; makes the page render on device width with the options page (personal preference)

* add comments * removed code that reduced the ux on other browsers

cowlicks · 2017-07-19T04:05:14Z

This is awesome @lemnis ! I'll test it out tomorrow.

Happened because of the remove of `overflow-y: hidden`

lemnis · 2017-07-19T08:50:38Z

src/skin/popup.css

  box-sizing: border-box;
 }
+@media screen and (min--moz-device-pixel-ratio:0) {


firefox specific css, otherwise the popup would render too wide

lemnis · 2017-07-19T08:52:39Z

src/skin/popup.css

  max-width: 100%;
  font-size: 12px;
  background: #fefefe;
  color: #333;
  font-family: helvetica, arial, sans-serif;
  padding: 8px 15px;
-  width: 400px;


creates a whitespace within chrome, use min-width instead

cowlicks · 2017-07-19T23:41:50Z

@lemnis would you mind sharing your workflow for installing this on mobile? I haven't done this before.

lemnis · 2017-07-20T13:06:07Z

On your phone, open Firefox and go to about:config
Change xpinstall.signature.required to true
Build the app on your computer, I personally use web-ext build
Change the file extension from .zip to .xpi
Make the .xpi accessible from the phone, use one of the methods below:
- Use python -m SimpleHTTPServer on your computer and open the server with your phone
- Use adb push and open the file location on your phone
For debugging, use the Firefox webIDE. All the logs will be shown within the Main Process

Remarks:

v55+ has a minimum support of browser_action that is needed
I couldn't install the extension on the current beta-build (55.0b10), got a message that the extension appears to be corrupt. Ignore this version...
Within FF nightly 56.0a1 theirs an issue that after install, the menu item appears 2x times or/and when you press on it, it redirects you to the "report site issue" page. If this happens force shut down the app, after a fresh start the app will behave as expected.

It's a hard life to live on the cutting edge 🙁

I don’t see the need for it and FF android doesn’t support it.

accidentally changed the width of the popup and fixed a issue where vertical scrollbars appeared on firefox

lemnis · 2017-07-20T14:48:31Z

All changes in src/skin/popup/css should resolve all zoom and unscrollable issues, but could create a vertical scrollbar if needed. Maybe it's best to wait for #1445

cowlicks · 2017-07-21T00:25:09Z

Some extra steps I needed. After transferring the file to my device:

grant storage permissions to firefox
in firefox navigate to the url file:///mnt/sdcard this should open a file browser, select the xpi file

I'm reviewing now

cowlicks · 2017-07-21T00:35:35Z

cowlicks · 2017-07-21T00:40:13Z

cowlicks · 2017-07-21T00:41:18Z

related to #1276 I ran the pbtest.org tests a few months ago this way and they all failed

This is great! I'll take a closer look at the changes asap. Thank you @lemnis

cowlicks

Great work! Let me know if you have any questions about my comments.

cowlicks · 2017-07-21T01:45:49Z

src/js/background.js

+  if(!chrome.browserAction.getPopup) {
+    chrome.browserAction.onClicked.addListener(() => {
+      chrome.tabs.query({active: true, lastFocusedWindow: true}, (tabs) => {
+        var url = chrome.runtime.getManifest().browser_action.default_popup + "?id=" + tabs[0].id;


Is the "?id=" + tabs[0].id part needed?

Are we trying to find if any popup is open? In which case having the tab included might make us miss it in the url query.

cowlicks · 2017-07-21T02:05:35Z

src/js/popup.js

+  var regex = new RegExp("[?&]" + key + "(=([^&#]*)|&|#|$)"), results = regex.exec(url);
+  if (!results) { return null; }
+  if (!results[2]) { return ''; }
+  return decodeURIComponent(results[2].replace(/\+/g, " "));


ah I see what this is for

cowlicks · 2017-07-21T02:11:23Z

src/js/background.js

+            chrome.tabs.update(popupTabs[0].id, {active: true});
+            chrome.tabs.reload(popupTabs[0].id);
+          } else {
+            chrome.tabs.create({url, index: tabs[0].index + 1});


I think we can get around the passing a url parameter. After you load the popup, you could try doing:

chrome.tabs.executeScript(tabId, {code: 'getTab = callback => {callback(' + tabId + ')}'});

This would override the getTab function in popup with something that works.

Actually, I don't think that would work... Another idea is using sendMessage. But if this is working well there is no need to fix it.

I agree there should be a neater solution, what do you think is the best behavior?

the current, update an existing popup with same parent tab or create a new popup

always create a new popup and close the popup directly after it was deactivated

keep at max 1 popup open and update its content

I think that option 2 is the best! It also mimics the behavior of current popups (it isn't long running). And adding code for deactivating the popup should be simpler than adding the updating code for 3. And I think it will avoid the confusion a user might have when they switch to a popup, and don't know what tab the popup is for.

cowlicks · 2017-07-21T02:21:22Z

src/js/background.js

+        chrome.browserAction.setBadgeBackgroundColor({tabId: tabId, color: "#cc0000"});
+      }
+    }
+    if(chrome.browserAction.setBadgeText){


Could we move all of these if (chrome.browserAction....) { guards to the top of this function?

Also, could we combine these into a global variable like isMobile?

Moved feature checks.

For what would you use that global variable? All my checks are just feature checks. The best way to determine if it's mobile is chrome.runtime.getPlatformInfo((platformInfo) => {console.log(platformInfo.os)});, but i don't see a specific use case.

The use case is to isolate browser specific code into its own region so developers can reason about it more easily.

For example, I'm imagining someone might be trying to fix some bug on chrome, if they come across this code, it might not be obvious that this intended for use on mobile.

Eventually, it would nice if browser specific code was isolated to independent files, then we could have a shim API. That is out of scope for this PR. But to make it easier to do in the future, it would be nice to have browser specific pieces clearly distinguished.

re: way to determine if it is mobile

I think we should use feature existence as a way to determine if its a mobile browser, so we could do something like:

let isMobile = !(chrome.browserAction.getPopup && chrome.browserAction.setBadgeText && etc);

This also means that we won't have to consider edge cases where some of these API's may exist while others do not.

The downside is we won't immediately get new features if these api's are added. But I think think that is okay. And when they are added all we have to do is remove them from the isMobile check.

added the boolean to the badger constructor, it's only the browserAction API where it is checked against.

Added in another pull request other browser specific code 😞 , bloating the source code 😇

cowlicks · 2017-07-21T02:21:57Z

src/js/background.js

-        }
+    chrome.tabs.query({}, function (tabs) {
+      for (var i = 0; i < tabs.length; i++) {
+        badger.refreshIconAndContextMenu(tabs[i]);


cowlicks · 2017-07-25T22:18:55Z

Sorry for the delay! I left a few comments, I haven't thought of a better way to pass the id to the popup. I'll let you know if I do, but I think a URL parameter is fine for now. It might also make some selenium tests easier.

cowlicks · 2017-07-25T22:31:56Z

src/js/popup.js

+* @return {String}     Value of the key
+*/
+function getParameterByName(key, url) {
+  if (!url) { url = window.location.href; }


I think we can use the URL API to do this nicely:

getTabFromUrl(url) { return parseInt(new URL(url).searchParams.get('tabId')); }

The nice thing about working on web extensions is that we only run on newer browsers, so we can use newer API's.

I used that API before but didn't bother to read. Changed it know, should have read it before though 💤

lemnis · 2017-08-14T19:06:43Z

We are both slow until nobody is unpleasantly waiting don't feel pressured to excuse yourself or keep up to a certain pace. As a Dutch girl, I'm not a 'mierenneuker' 😉

The code is updated so that a former popup will not close if the user changed the URL. I doubted if the code for onUpdated should be in his own event listener or added to to the current. I have eventually chosen the second.

As of bugzilla#1386786 is fixed, we can decide to add the number of blocked resources to the title between parenthesis. I removed the line that did set the title within this pull request background.js:765, because it was a fixed string of text.

There are 3 options:

Just don't bother to add the code
Only set a 'custom' title for FF android
Set a 'custom' title for every browser, so when you hover over the icon, you can read a number of blocked resources

The benefit would maybe disappear for FF mobile in the future if Mozilla decides to use icons. Also, this added information could benefit blind users, but I don't know if the current plugin is usable for them.

cowlicks · 2017-08-15T18:20:24Z

src/js/background.js

@@ -762,7 +768,6 @@ Badger.prototype = {
    }

    chrome.browserAction.setIcon({tabId: tab.id, path: iconFilename});
-    chrome.browserAction.setTitle({tabId: tab.id, title: "Privacy Badger"});


Nice, now this is localized properly by the "default_title" key in the manifest

cowlicks · 2017-08-15T20:58:07Z

src/js/popup.js

@@ -512,6 +512,14 @@ function syncUISelections() {
 * seems to be that it is synchronous.
 */
 function getTab(callback) {
+  // Temporary fix for Firefox Android while it doesn't support `browser_action.default_popup`
+  if(badger.isFirefoxMobile){
+    chrome.tabs.query({active: true, lastFocusedWindow: true}, function(focusedTab) {


I think we can use chrome.tabs.getCurrent here instead

getCurrent unfortunately doesn't work

cowlicks · 2017-08-15T21:06:35Z

src/js/background.js

@@ -776,12 +781,45 @@ function startBackgroundListeners() {
    }
  }, {urls: ["http://*/*", "https://*/*"]}, []);

+  if(badger.isFirefoxMobile) {


Could you refactor these to be like:

if (!isMobile) { startRegularListeners(); // with an exposed named function } else { startMobileListeners(); }

This way the mobile logic is separate from the regular logic. So we're able to test them independently .

cowlicks

This looks great. I've tested it, and everything is working as expected. I've just left a few notes about refactoring.

lemnis · 2017-08-16T03:02:39Z

(Finally) moved all the code to a separate file. Maybe not the neatest code, because I am putting functions for both the popup.js and the background.js inside the same file. Besides that, feel free to change the function names, the ones that I have chosen are maybe not the best for knowing where you should use the functions.

cowlicks · 2017-08-16T17:55:05Z

To fix the tests you just need to include the firefoxandroid.js file in src/tests/index.html. Otherwise LGTM

cowlicks · 2017-08-20T20:25:06Z

@lemnis great work! Thank you.

strugee · 2017-08-20T21:56:14Z

Does this mean that Android support is "done"/stable? Or just much closer?

Context: prism-break/prism-break#1648

cowlicks · 2017-08-21T02:28:06Z

@strugee "just much closer" I think. It hasn't been tested much. And I think we should consider revamping some UI before we officially support it.

strugee · 2017-08-21T04:55:31Z

👍 thanks for clarifying!

ghostwords · 2017-08-21T18:30:11Z

The domain list key/legend icons seem misaligned in the popup and the options page:
.

This is coincidentally fixed in my WIP flexible popup layout PR (#1445) since I needed to change how the key/legend icons get rendered.

ghostwords · 2017-08-22T21:07:08Z

It seems that we also introduced a body scrollbar in case of long-enough domain lists in Firefox:

lemnis · 2017-08-23T01:09:02Z

@ghostwords #1516 (comment) 😉

But, maybe adding temporarily body { overflow: hidden; } wouldn't be a terrible idea, it will remove the scrollbars and nobody will notice it besides the single user that already uses Privacybadger on their phone (a.k.a. me)

I overlooked the misalignment of the key/legend icons...

cowlicks · 2017-08-26T20:50:35Z

@ghostwords would you mind creating a separate issue so this can be more easily tracked?

Introduced in #1516.

lemnis added 7 commits July 16, 2017 06:24

making options page accessible within android

5328e70

faking popup when clicked on browser action

b330291

extra checks if specific function is available

f222299

fix for android to get tab id

fdaa6fb

Merge remote-tracking branch 'EFForg/master'

d39af45

cleanup

c61c0a8

* add comments * removed code that reduced the ux on other browsers

lemnis mentioned this pull request Jul 19, 2017

Android support? #1084

Closed

Removes whitespace at bottom of a popup in chrome

0b865e8

Happened because of the remove of `overflow-y: hidden`

lemnis commented Jul 19, 2017

View reviewed changes

lemnis added 2 commits July 20, 2017 16:13

removed chrome.windows api

ab5d902

I don’t see the need for it and FF android doesn’t support it.

small fixes caused by container size

5a8d934

accidentally changed the width of the popup and fixed a issue where vertical scrollbars appeared on firefox

cowlicks added the Android label Jul 21, 2017

cowlicks reviewed Jul 21, 2017

View reviewed changes

Moved feature checks to top of function

323580e

cowlicks reviewed Jul 25, 2017

View reviewed changes

lemnis added 4 commits July 26, 2017 14:34

Moved to URL api

3bda018

changed fake popup behavior + cleanup

ea950fd

update feature check

7884298

removed whitespace

700e92c

cowlicks reviewed Aug 15, 2017

View reviewed changes

cowlicks mentioned this pull request Aug 15, 2017

Document Android / mobile development process #1578

Closed

lemnis added 4 commits August 16, 2017 04:30

Moved all FF Android functions to sep. file

2c986a2

Merge branch 'EFForg/master'

e73d12d

small bug fixes

98fbb56

improved comments

2443260

fix for selenium test

1f230cf

cowlicks merged commit 35223a3 into EFForg:master Aug 20, 2017

strugee mentioned this pull request Aug 21, 2017

Please recommend Privacy Badger for Firefox on Android prism-break/prism-break#1648

Closed

This was referenced Aug 28, 2017

Domain list key/legend icons misaligned in popup and on options page #1609

Closed

Unwanted popup scrollbars in Firefox #1610

Closed

cowlicks mentioned this pull request Sep 5, 2017

Show better info when no 3rd party resources present. #1627

Closed

ghostwords added a commit that referenced this pull request Sep 12, 2017

Fix extraneous bottom padding.

564f688

Introduced in #1516.

ghostwords mentioned this pull request Sep 14, 2017

Update homepage to note Android support #1668

Closed

This was referenced Oct 26, 2017

Badger popup UI is cut off on the bottom in some locales #1077

Closed

Popup cut off when page zoom is set to more than 100% #1113

Open

Added support for firefox on android #1516

Added support for firefox on android #1516

Conversation

lemnis commented Jul 19, 2017 • edited Loading

cowlicks commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowlicks commented Jul 19, 2017

lemnis commented Jul 20, 2017

lemnis commented Jul 20, 2017

cowlicks commented Jul 21, 2017

cowlicks commented Jul 21, 2017

cowlicks commented Jul 21, 2017

cowlicks commented Jul 21, 2017

cowlicks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowlicks commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemnis commented Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowlicks Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

cowlicks left a comment

Choose a reason for hiding this comment

lemnis commented Aug 16, 2017

cowlicks commented Aug 16, 2017

cowlicks commented Aug 20, 2017

strugee commented Aug 20, 2017

cowlicks commented Aug 21, 2017

strugee commented Aug 21, 2017

ghostwords commented Aug 21, 2017

ghostwords commented Aug 22, 2017

lemnis commented Aug 23, 2017

cowlicks commented Aug 26, 2017

lemnis commented Jul 19, 2017 •

edited

Loading

lemnis commented Aug 14, 2017 •

edited

Loading

cowlicks Aug 15, 2017 •

edited

Loading