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

JS module loading for older Androids (fixes #9311) #13812

Closed
wants to merge 1 commit into from
Closed

JS module loading for older Androids (fixes #9311) #13812

wants to merge 1 commit into from

Conversation

nihil-admirari
Copy link
Contributor

Purpose / Description

JS modules must have text/javascript MIME, otherwise they won't load: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules.

Fixes

Fixes #9311.

Approach

guessMimeType() works on Android 10+. MIME type must be set explicitly on older Androids.

How Has This Been Tested?

Android 8.0 and 13.0 in emulator.

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@welcome
Copy link

welcome bot commented May 7, 2023

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

@david-allison
Copy link
Member

This needs to be checked against #10175 before it goes in.

See: f758778

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label May 7, 2023
@nihil-admirari
Copy link
Contributor Author

I've restored ba41619 implementation with the exception of JS MIME type, which should be text/javascript.

I wasn't able to reproduce #10175. I have certain questions about that issue, however, which I've listed in #10175 (comment).

@nihil-admirari
Copy link
Contributor Author

@david-allison, it's been a week since I've pushed the last changes, can you please take a look?

Considering that you've been unable to reproduce #10175 yourself, and that @Anthropos888 still got the issue even in a release with module loading changes removed, these changes are probably safe to merge.

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

this seems reasonable and is backed by testing, it does seem safe to me

@mikehardy mikehardy requested a review from david-allison May 15, 2023 11:59
@mikehardy mikehardy added Needs Second Approval Has one approval, one more approval to merge and removed Needs Author Reply Waiting for a reply from the original author labels May 15, 2023
@nihil-admirari nihil-admirari deleted the branch ankidroid:main May 20, 2023 16:46
@nihil-admirari nihil-admirari deleted the main branch May 20, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Second Approval Has one approval, one more approval to merge New contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Closet addon nonfunctional in Android versions 9 and below
3 participants