AprilSylph / XKit-Rewritten

🧰 The enhancement suite for Tumblr's new web interface
GNU General Public License v3.0
274 stars 44 forks source link

upgrade to Manifest V3 #1466

Closed AprilSylph closed 1 week ago

AprilSylph commented 1 month ago

Description

This PR makes the following changes:

Testing steps

Feature testing: Installation

To make sure the new permissions prompt is working:

  1. Install the branch version of the addon to a browser which treats host_permissions as optional, e.g. Firefox
  2. Open the XKit Rewritten control panel
    • Expected result: The warning banner appears at the top, with the Always allow on www.tumblr.com button
  3. Click Always allow on www.tumblr.com
    • Expected result: You are prompted to grant the host permission
  4. Grant the host permission
    • Expected result: The warning banner disappears

Smoke testing

Test each feature to make sure it's still working as it was before. Panorama is excluded from this list, since it is already broken.

Usage testing

[!IMPORTANT] If you are using regular Firefox (i.e. not Nightly or Developer Edition), you must install the branch version of the addon via about:debugging, which means the addon and its config will be lost as soon as you close the browser!

  1. On your main browser where you use XKit Rewritten, download a config backup
  2. Disable the release version of XKit Rewritten
  3. Load the branch version of XKit Rewritten:
    • On Chromium: unzip the addon, then load as unpacked via chrome://extensions
    • On Firefox Developer Edition or Firefox Nightly:
      • Disable the addon signature requirement (about:configxpinstall.signatures.requiredfalse)
      • Install the addon ZIP via about:addons → :gear: → Install Add-On From File
    • On Firefox: Install the addon ZIP as a temporary extension via about:debugging
  4. Restore your config on the branch version of XKit Rewritten
  5. Use Tumblr as normal for the rest of the day
    • Expected result: Other than some modifications now being applied a few frames slower, your Tumblr/XKit experience should be the same as ever
marcustyphoon commented 3 weeks ago

What do you think about bumping to Chrome 121 for https://caniuse.com/mdn-css_properties_scrollbar-width (so we don't have a Firefox-only feature) or perhaps Chrome 117 for https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/groupBy (which I found a use for in #1482)?

Edit: Chrome 117 drops support for MacOS 10.13 and 10.14, though, and there are a few years of hardware where those are the final versions. I believe none of the relevant versions run on Win7/8, so no drawbacks on that side.

AprilSylph commented 2 weeks ago

All features smoke-tested and working in Firefox 128.0b3.

AccessKit, PostBlock, and Tweaks tested and working in Chromium 126.0.6478.55.

Will merge with one approving review. If you'd like to test extensively on your browser of choice, please feel free to edit the PR description to reset the checklist.

marcustyphoon commented 2 weeks ago
marcustyphoon commented 2 weeks ago

Stupid question: why do we need host permissions, again...?

Edit: Ah, I see, https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/host_permissions doesn't mention content scripts, but https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/permissions#host_permissions does. That looks like an MDN oversight. Maybe if I remember I'll PR a clarification.

Edit 2: I guess for clarification I should mention that this seems to be Firefox-specific. In a sense, I guess, "Firefox ≤126 weirdly includes static content scripts in the list of things that require host permissions" seems more accurate to me than "Firefox weirdly treats host permissions as optional."

marcustyphoon commented 2 weeks ago

https://bugzilla.mozilla.org/show_bug.cgi?id=1889402 https://github.com/mdn/content/pull/34094

Uh. Hm. Are we supposed to request host permissions in manifest.json?

Obviously in Chromium land, no we aren't, since we only have static content scripts. But if in Firefox land we do need host permissions, and if in the future Firefox will ask the user during install if they want to grant the host permissions we request in the manifest, and if in the present Firefox will apparently unconditionally the host permissions we request in the manifest...

Edit: Looking for clarification on this, but I believe the answer is no and the current setup is correct.

marcustyphoon commented 1 week ago

Also tested in Firefox for Android 121.1.0.

Also tested: my patience.

AprilSylph commented 1 week ago

(Aside: if it were me, I might consider rebasing, squashing to a "rename everything without changes" commit and an "implement MV3" commit, then merge committing that, so they're separate in the repo history. Kind of a pain, but might make it easier to merge into PR branches.)

I could cherrypick out the renames into a separate PR, merge that, then rebase this PR with those changes already in the main branch?

marcustyphoon commented 1 week ago

That would work! Definitely not a big deal if I had to guess.

marcustyphoon commented 1 week ago

We could also consider setting the minimum Firefox version to at least 127, in which users will (it seems) continue to have the extension work in the background without manually granting origin permissions. I did test this using web-ext and it does seem to work in that case, while 126 requires the click.

There may be other MV3 improvements in 128, as well, but that releases 4 weeks from now and I'm not keen on delaying bugfix/feature updates for Firefox users until then-plus-however-long-it-takes-people-to-update.

AprilSylph commented 1 week ago

hm. c9d5924 should've been included in #1507, technically. oh well.

AprilSylph commented 1 week ago

also, to be clear, my plan for this weekend is:

  1. today, get Manifest V3 merged, but not released
  2. tomorrow, fix the TimelineV2 shenanigans, and then do a release
AprilSylph commented 1 week ago

Next up: time to rebase #1501.