New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install via pop up window, not tab #2641

Closed
arantius opened this Issue Nov 2, 2017 · 14 comments

Comments

Projects
None yet
2 participants
@arantius
Collaborator

arantius commented Nov 2, 2017

This issue exists primarily to track my desire to make this change, but inability to:

https://bugzilla.mozilla.org/show_bug.cgi?id=1402110

Opening dialogs is buggy. I've written all the code but it doesn't work thanks to the linked Firefox bug.

@arantius arantius added this to the Tracking Upstream milestone Nov 2, 2017

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Nov 19, 2017

Of interest. In one of the test branches[1] I've been working on I implemented a popup box. And the content painted correctly, without having to click on it[2]. Though the title prefix is still an issue. The prefix is added but the mox-extension://blahblahblah is still before it.

[1] The branch is one that I had setup detecting UserScripts in the background and then caching the content so that the XHR wasn't required to install new scripts. I don't think those changes particularly affect this issue though.
[2] On 59.0a1

@arantius

This comment has been minimized.

Collaborator

arantius commented Nov 19, 2017

I've also got a branch for this ( https://github.com/arantius/greasemonkey/tree/install-dialog ) though I haven't tested in a while.

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Dec 29, 2017

Turns out the problem seems to be with extensions.webextensions.remote.

If that preference is set to false (default for Linux) then the window will be white and buggy. If the preference is set to true (default for Windows) then the window displays without problem.

arantius added a commit to arantius/greasemonkey that referenced this issue Jan 5, 2018

@arantius

This comment has been minimized.

Collaborator

arantius commented Jan 5, 2018

I've updated my branch, linked above, with a workaround. I'd appreciate any comments on how this works. It's a little bit janky, but maybe good enough?

@arantius arantius modified the milestones: Tracking Upstream, 4.2 Jan 5, 2018

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Jan 6, 2018

Android can be accommodated with something like (tested on my phone).

chrome.runtime.getPlatformInfo(platform => {
  let url = chrome.runtime.getURL('src/content/install-dialog.html')
      + '?' + escape(JSON.stringify(message.userScript));

  if ('android' === platform.os) {
    chrome.tabs.create({'active': true, 'url': url});
  } else {
    ...
  }
});

Why history.back()? I'm in favor of just leaving the page open. That way the user can examine the script before installing, if they want.

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Jan 6, 2018

(tested on my phone)

Apparently I didn't test enough. Opening the install thing works on the phone, but install the script just fails. At the moment I'm not sure if related to this change or something else.

@arantius

This comment has been minimized.

Collaborator

arantius commented Jan 6, 2018

Why history.back()?

It's closer to 3.x behavior, which shows only the popup, never any navigation. Inspecting the separately-loaded source also leaves you open to attack by malicious servers, delivering different content for the two different fetches. So it's not a great way to review the source.

More ideally we might switch to a blocking webRequest (?) handler to get even closer.

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Jan 6, 2018

Inspecting the separately-loaded source also leaves you open to attack by malicious servers, delivering different content for the two different fetches. So it's not a great way to review the source.

The PR #2719 and the code that I've been working on in regard to asynchronous download, on this branch[1][2], would prevent that. The installation workflow works on a single request. As well as optimistically downloads / pre-fetches / whatever you want to call it any other remote content before the install button is even pressed. Of course, everything is aborted and cleaned up if the cancel button is pressed, or if the window is closed.

[1] There's a few style things and various minor issues that need to be resolve with the linked code.
[2] Those changes would effectively remove FF 52 support.

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Jan 6, 2018

Apparently I didn't test enough.

I'm just dumb. I have a local server in which I use for navigation .user.js tests. And at some point (which I forgot) I changed the sample script I used. Said sample script had a bad @require.

Anyway, installing scripts do work on Android. There's some other usability problems but those are for a different issue.

arantius added a commit to arantius/greasemonkey that referenced this issue Jan 9, 2018

@arantius arantius closed this in 5295db5 Jan 9, 2018

@arantius

This comment has been minimized.

Collaborator

arantius commented Jan 9, 2018

The above fix is packaged in version 4.2beta2:
https://addons.mozilla.org/firefox/downloads/file/833159/greasemonkey-4.2beta2-an+fx.xpi?src=devhub

Testing is always appreciated!

@arantius

This comment has been minimized.

Collaborator

arantius commented Jan 12, 2018

Dunno if we can help this, but the window pops up in a strange place on my laptop (top of screen, at the right 2/3rds or so).

More importantly: window should close after successful install.

@arantius arantius reopened this Jan 12, 2018

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Jan 12, 2018

Dunno if we can help this, but the window pops up in a strange place on my laptop (top of screen, at the right 2/3rds or so).

Is this a side effect of the resizing? When I was playing with dialog boxes in my testing Async download branch I don't recall it popping up in 'strange' locations. It was generally middle-topish.

@arantius

This comment has been minimized.

Collaborator

arantius commented Jan 12, 2018

On another laptop and on a desktop I'm seeing middle-of-screen now.

@Sxderp

This comment has been minimized.

Contributor

Sxderp commented Jan 12, 2018

On the issue of opening a blank window, it looks like actual patches are being worked on in this ticket.

arantius added a commit to arantius/greasemonkey that referenced this issue Jan 15, 2018

arantius added a commit to arantius/greasemonkey that referenced this issue Jan 15, 2018

@arantius arantius closed this Jan 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment