Fix dark-mode

[hgluka]

Description

This fixes dark-mode by removing the javascript: URL prefix from the darken bookmarklet.

Fixes #3128.

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

• Git hygiene:
  • I have pulled from master before submitting this PR
  • There are no merge conflicts.
• I've added the new dependencies as:
  • ASDF dependencies,
  • Git submodules,

    cd /path/to/nyxt/checkout
    git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser

  • and Guix dependencies.
• My code follows the style guidelines for Common Lisp code. See:
  • Norvig & Pitman's Tutorial on Good Lisp Programming Style (PDF)
  • Google Common Lisp Style Guide
• I have performed a self-review of my own code.
• My code has been reviewed by at least one peer. (The peer review to approve a PR counts. The reviewer must download and test the code.)
• Documentation:
  • All my code has docstrings and :documentations written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)
  • I have updated the existing documentation to match my changes.
  • I have commented my code in hard-to-understand areas.
  • I have updated the changelog.lisp with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).
    • Changelog update should be a separate commit.
  • I have added a migration.lisp entry for all compatibility-breaking changes.
  • (If this changes something about the features showcased on Nyxt website) I have these changes described in the new/existing article at Nyxt website or will notify one of maintainters to do so.
• Compilation and tests:
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works. (If possible.)
  • I ran the tests locally ((asdf:test-system :nyxt) and (asdf:test-system :nyxt/gi-gtk)) and they pass.

[aadcg]

In my understanding commits a79a874 and f073f7d should be handling these cases. Did you investigate it @hgluka? See related PR #2971.

We have two options: either properly handle the case when source begins with javascript:; or drop all code that tries to handle it.

[hgluka]

Ah, I missed those. Should've checked for any earlier work on it.

With that in mind, here's what I found:

(source (if (str:starts-with-p "javascript:" source)
                               (quri:url-decode (subseq source 11))
                               source))

If you look at the above code, quri:url-decode is only called if the url starts with javascript:.
If we change the last line to (quri:url-decode source) as well, then one other bookmarklet breaks:
invert-colors, which happens to be the only one other than darken that's not of the form:

(function(){ ...code here... })();

Maybe that's the issue here?

EDIT: It turns out that all the bookmarklets break with quri:url-decode, not just invert-colors and darken.
This could be due to quri:url-decode changing all the plus signs to spaces and such.

[aadcg]

@hgluka it seems that we need to answer the question: do we need quri:decode? CC @aartaka.

[aartaka]

hgluka it seems that we need to answer the question: do we need quri:decode? CC @aartaka.

We do need it in case of javascript: URLs, because these should be percent-encoded. In case there's no javascript: URL, then don't decode.

We can have an additional check, for javascript: URLs: if there's any character that has to be percent-encoded, then this is not a valid javascript: URL and, by extension, bookmarklet. Throw warnings when that happens.

[aadcg]

@hgluka and @aartaka please coordinate efforts to move forward with this PR. I don't know anything about bookmarklets so I'm sure you're in a better position.

Ideally, this fix should be fixed by next Monday, the date of the next release, which will feature bug fixes only. Don't forget to add a changelog entry mentioning it. Thanks.

[hgluka]

I agree with @aartaka. The only question is, do we want to throw an error when a bookmarklet with javascript: isn't properly encoded, or just echo a warning and let things continue?

[jmercouris]

just echo a warning if you can gracefully recover

[aartaka]

Same as @jmercouris:

• Echo a warning.
• Ignore the bookmarklet.
  • Or define it to alert the user (maybe with JS' window.alert().

EDIT: Typo fix

[hgluka]

@aadcg @aartaka I don't think there's a way to prove whether a string is percent-encoded or not.
What I did here is check if decoding the source for the bookmarklet has any effect.

Still, if a bookmarklet that starts with javascript: has a +, by this logic it could possibly be encoded (encoding changes spaces to plus signs, decoding does the opposite), so that would still slip through the cracks.
That's why I left darken without the javascript: prefix.

[aadcg]

@hgluka let me know when it's ready to be merged. One commit for the fix, and another one for the changelog entry please. Thanks!

[hgluka]

@aadcg Should be ready now.

[aadcg]

Thanks @hgluka.