Skip to content
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

Support Roll20 as a Discord Activity #1109

Closed

Conversation

rileydutton
Copy link
Contributor

Hey there!

Roll20 is coming to Discord as an Activity. As part of that we are taking a look at popular extensions that Roll20 users install and seeing what needs to be done to support the Activity version. In particular, it doesn't seem possible to use the messaging feature to send messages between tabs since the Activity runs as a nested iFrame in a different domain from the parent tab (this is a Discord security feature and applies to all Activities).

Therefore, I put together this quick PR that adds support for using a content port to communicate with the Discord Activity version. It only uses this method if the extension detects that it's running in an Activity, otherwise it uses the current message passing method.

The one drawback to this is the "filter to a specific tab" feature really isn't supported for the Activity (if you pick a specific tab it just won't work), but I think that's an acceptable tradeoff to get this working.

Let me know if this makes sense or if I can help work on a different approach that would work well for the extension. And thanks for providing this resource to the tabletop gaming community!

Copy link

@jtbg jtbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a staging build of the new activity to use for testing?

I guess a bit of new documentation to tackle as well, so I can kill two kobolds with one stone this weekend

},
{
"matches": [
"*://1199271093882589195.discordsays.com/*"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming discord activity IDs static/reserved (or at least reasonably so):

Are there other proxies that the activity might be served through? eg geolocal/app-specific/mweb

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the Discord Activity IDs are static and I'm not aware of any other URLs where the activity will be served from.

@rileydutton
Copy link
Contributor Author

Is there a staging build of the new activity to use for testing?

I guess a bit of new documentation to tackle as well, so I can kill two kobolds with one stone this weekend

@jtbg There is an internal Beta going on right now that I can get you access to, if you let me know your Roll20 account email. You can just email me at riley@roll20.net.

Copy link

@jtbg jtbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • (done) static code review

  • (done) test build and extension load

    • Chromium
    • firefox
  • (done) regression test existing the Roll20 Web support

    • Chromium
    • firefox
  • (pending) test the new integration Roll20 Discord support

product surface testing status:

DDB in Chromium DDB in Firefox
Discord Win app out of scope out of scope
Discord Mac app out of scope out of scope
Discord web ⚠️not tested ⚠️not tested
Discord Linux app out of scope out of scope

(@rileydutton is my understanding correct that the content port would still be out of band from the perspective of the standalone electron apps?)

@HaarisQureshi
Copy link

HaarisQureshi commented Apr 26, 2024

Have given @jtbg's build a test, does not seem to be sending Rolls to the Discord activity, either to the app or via browser

@kakaroto
Copy link
Owner

Hi @rileydutton!
Welcome to the Beyond20 contributors list! 😁

Thanks for sending this PR, I saw the announcement a few days ago on your blog and was excited by the idea. I think the discord activity is a great feature and I can't wait to give it a try. I'll send you an email to get beta access so I can test this out.

I've had a quick look at the code, it looks good, I have a couple of suggestions that I want to try (and see if they're viable) before I do a PR review.
The main issue I have with this though is the addition of the new host to the manifest. Unfortunately, as we've discovered when we added astral tabletop support a couple of years back, changing those permissions causes the extension to get automatically disabled for all users, until they manually go to "manage extensions" and approve the new permissions. Because of that, I believe we lost half our userbase and it caused a lot of user confusion. Because of that, I decided not to add any new domains to the manifest for the time being. I might be wrong with how exactly chrome does it, but I don't want to tempt fate.

What we've implemented instead is the ability to prompt the user at runtime for optional permissions as needed, so I think for this instance, it's going to be better to do a user prompt with an optional permission so it doesn't disable the extension, so I want to add that alternative way instead for the discord activity.
Once I have beta access to it, I'll give it a try and see how to make it work as smoothly as possible for the users.
Thanks again for the contribution, and congrats on the new Roll20<->Discord feature!

@jtbg
Copy link

jtbg commented Apr 27, 2024

I might be wrong with how exactly chrome does it, but I don't want to tempt fate.

Not wrong at all, this would require the user to explicitly re-enable it.


1. Technical demo of chrome flagging the update
b20_update.mp4
2. UX demo of public-facing view post-update
b20_enable.mp4

UX-demo-gif

@rileydutton
Copy link
Contributor Author

Hey all,

Thanks for taking the time to look at this! Appreciated.

Really however you all want to handle it re: the permissions is fine, that makes sense. I did notice there is a part of the options dialog that shows up if it detects that permissions don't exist for the current domain, but that was detecting "discord.com" instead of the "1199271093882589195.discordsays.com" (again due to the whole nested iframes issue). But if there's a way to prompt for permissions on the correct domain the first time you launch the Activity (or however you want to do it) that should work fine!

Really I just wanted to get this process started and at least attempt to provide some working code, but as you all get in and check it out I'm sure you know better than me what changes need to be made :-) But let me know if I can help.

@kakaroto
Copy link
Owner

kakaroto commented Jun 6, 2024

Update:
I did some testing/investigating today, and yeah, removing the permission from the manifest is going to make it a bit challenging because of the all_frames: true part. We can't inject into 1199271093882589195.discordsays.com unless the tab itself has that URL, so I'd need to add a content-script on discord.com and have that look for the iframe and inject the roll20 script into it. I think that's the best/only way to do it.

Note that would also allow us to have the popup settings work if we can have the discord.com content script act as roll20 basically, when the iframe is there. Will see how we can solve that.

I'll take a stab at getting that to work soon (hopefully 🤞)

@kakaroto kakaroto mentioned this pull request Jul 31, 2024
@dmportella
Copy link
Contributor

if there is something i can help with this let me know

@kakaroto
Copy link
Owner

kakaroto commented Aug 7, 2024

if there is something i can help with this let me know

Thank you very much for volunteering to help.

Where I got stuck last time was with the giving of permissions. The idea would be to add the permissions dynamically instead of having it in the manifest file. We can do it by adding:

    "*://1199271093882589195.discordsays.com/*" // Roll20 Discord activity

to the SUPPORTED_VTT_URLS array in common/constants.js and then when that page is detected, likely from the service worker (background.js), if the user didn't authorize beyond20 to give it access to that url, then prompt the user (using the same code as the one for custom domains, in settings.js line ~1770):

            chrome.permissions.contains({origins: [url]}, (hasPermission) => {
                if (hasPermission) return;
                chrome.permissions.request({origins: [url]}, (response) => {
                    if (response) {
                        console.log("Permission was granted");
                        alertify.success(`Beyond20 will now load automatically on ${url}`);
                    } else {
                        console.log("Permission was refused");
                        alertify.error(`Error requesting permission for ${url}`);
                    }
                });
            });

Ideally, also storing in a config if the user rejects it so we don't prompt them every time they open the page, and also allow them to re-permit it from a button in the advanced settings, in case they mistakenly reject it but then later want to authorize it. Humm... I'd probably put an entry in advanced settings for "Support Roll20 Discord Activity" with a checkbox, if the value is undefined, then replace the checkbox with a "Authorize" button, if the user got prompted once before and they approved or rejected, then the setting will store true/false and the setting becomes that...
arggg, forget it, I ended up quickly doing the code, not very well tested and doesn't do anything other than set the permissions, and actually ignores the checkbox idea I wrote above, since I don't see the point of a checkbox unless it does permissions.remove... I think that's all we need, other than requesting the permissions when the user opens discord itself, then this needs merging with the other changes done here by @rileydutton and then testing that it works.

I've pushed my changes here: https://github.com/kakaroto/Beyond20/tree/roll20-discord-activity
Note that I also realized that there's no reason why firefox wouldn't support the permission requests, but in their docs, they only show it returning a promise, not taking a callback function so maybe that's why it wasn't working on firefox when I implemented that 2 years ago, so I'd love to test if we can request permissions on firefox as well at this point and remove the firefox specific code that just asks people to click the browser action.
That's about it for what I can do at the moment, I leave it in your hands until my availability frees up again!
I gotta go, good luck!

@dmportella
Copy link
Contributor

thank you for the update, i will try to merge all the code and get my head around it, i got it to work when you are on web discord but i have not been able to get it to work on the discord application yet.

I will have a try and let you know

@kakaroto
Copy link
Owner

i got it to work when you are on web discord but i have not been able to get it to work on the discord application yet.

Awesome! And yes, it will only work with web discord, I don't think it's doable with the discord app, since that counts as a separate program and beyond20 can only run within the browser (unless there's a way to install extensions into the discord app and have the D&D Beyond pages open inside the discord app too.... so yeah, that won't work!)

Let me know when you've pushed a working branch. Thanks!

@dmportella
Copy link
Contributor

@kakaroto got things all merged and working but I have not done any changes yet there is a few bugs that need to be fixed, perhaps you can provide me some idea of what and where it might be. I wasnt sure which was the best way to get this to you so i pushed another PR. -> #1166

kakaroto added a commit that referenced this pull request Sep 23, 2024
By adding the discord activity url to the manifest, it forces
new permissions which would disable the extension for everyone,
instead, we use dynamic request for optional permissions, and we use
webNavigation to be able to inject the roll20 scripts
into the iframe inside of discord.com
This should fix and close #1166 and #1109
And fixes #1157
@kakaroto
Copy link
Owner

Thank you @rileydutton for the contribution. I had to rewrite the permissions system so we could request permissions dynamically and inject beyond20 into the iframe of the discord activity and it's working quite well now.
I've merged it via the PR #1166, it will be available in the next release which I plan on releasing tonight.

@kakaroto kakaroto closed this Sep 23, 2024
@rileydutton
Copy link
Contributor Author

rileydutton commented Sep 23, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants