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

v1.14.1: regression in tabs with non-standard IDs #2344

Closed
tagliala opened this issue Mar 22, 2025 · 6 comments · Fixed by #2345
Closed

v1.14.1: regression in tabs with non-standard IDs #2344

tagliala opened this issue Mar 22, 2025 · 6 comments · Fixed by #2345

Comments

@tagliala
Copy link

Hello,

not sure that this is an issue with jQuery UI, but I think it is worth to report it

In Active Admin, we were trying to upgrade jQuery UI to 1.14.1 from 1.13.2, but one of our specs related to the tabs component was failing.

The failure is related to the way Active Admin creates the fragment name when it contains just an emoji, which is the same way GitHub creates fragments in markdown to HTML conversion

Example: ☺️ => %EF%B8%8F

Reproducible test case:

Code taken from: https://jqueryui.com/tabs/

Expected

Image

Actual

Image

Refs:

@mgol
Copy link
Member

mgol commented Mar 24, 2025

Thanks for the report and easy to run test cases!

This was caused by #2307. I'll have a look at it for 1.14.2.

@mgol mgol added this to the 1.14.2 milestone Mar 24, 2025
@mgol mgol self-assigned this Mar 24, 2025
@mgol
Copy link
Member

mgol commented Mar 25, 2025

I added decoding in #2307 as it's needed for some complex characters like emojis to work - it's interesting that the linked issue claims it's emoji handling that's now broken... 🤔

@mgol
Copy link
Member

mgol commented Mar 25, 2025

I wanted to see if we should or should not be decoding the anchor hash value in the end. I checked native browser behavior, without jQuery UI - one was testing whether <a href="#🤗"> will scroll to <div id="🤗">, the second one was whether <a href="#😅"> will scroll to <div id="%F0%9F%98%85">. Since anchor.hash is always encoded:

anchor1.hash // '#%F0%9F%A4%97' for `<a href="#🤗">`
anchor2.hash // '#%F0%9F%98%85' for `<a href="#😅">`

we have the following:

  1. If and only if we do decode, href from <a href="#🤗"> will point to the id of <div id="🤗">
  2. If and only if we do NOT decode, href from <div id="🤗"> will point to the id of <div id="%F0%9F%98%85">

However, in the browser, clicking any of them will scroll to the proper div. 🤔

That got me curious so I looked into the specs. It starts with the "scrolling to a fragment" section of the HTML spec. That uses a concept of document's indicated part. Slightly below there's an algorithm to compute the indicated part. The interesting parts are steps 4 to 9:

  1. Let potentialIndicatedElement be the result of finding a potential indicated element given document and fragment.
  2. If potentialIndicatedElement is not null, then return potentialIndicatedElement.
  3. Let fragmentBytes be the result of percent-decoding fragment.
  4. Let decodedFragment be the result of running UTF-8 decode without BOM on fragmentBytes.
  5. Set potentialIndicatedElement to the result of finding a potential indicated element given document and decodedFragment.
  6. If potentialIndicatedElement is not null, then return potentialIndicatedElement.

First, in steps 4-5, the algorithm tries the hash as-is, without decoding. Then, if one is not found, the same is attempted with a decoded hash. Yes, it actually checks both!

That complicates things as we cannot compute the id from the hash without running a query and checking if an element with a matching id is actually in the DOM. 🤕 As part of that, we should also perhaps revert 1e5662e (this part should also be tested).

mgol added a commit to mgol/jquery-ui that referenced this issue Mar 26, 2025

Verified

This commit was signed with the committer’s verified signature.
mgol Michał Gołębiowski-Owczarek
Prior to jQuery UI 1.14.1, hashes in anchor hrefs were used directly. In
jquerygh-2307, that was changed - by decoding - to support more complex IDs, e.g.
containing emojis which are automatically encoded in `anchor.hash`.
Unfortunately, that broke cases where the panel ID is decoded as well.

It turns out the spec mandates checking both. In the "scrolling to a fragment"
section of the HTML spec[^1]. That uses a concept of document's indicated
part[^2]. Slightly below there's an algorithm to compute the indicated part[^3].
The interesting parts are steps 4 to 9:
4. Let potentialIndicatedElement be the result of finding a potential
   indicated element given document and fragment.
5. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.
6. Let fragmentBytes be the result of percent-decoding fragment.
7. Let decodedFragment be the result of running UTF-8 decode without BOM on
   fragmentBytes.
8. Set potentialIndicatedElement to the result of finding a potential indicated
   element given document and decodedFragment.
9. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.

First, in steps 4-5, the algorithm tries the hash as-is, without decoding. Then,
if one is not found, the same is attempted with a decoded hash.

This change replicates this logic by first trying the hash as-is and then
decoding it.

Fixes jquerygh-2344
Ref jquerygh-2307

[^1]: https://html.spec.whatwg.org/#scrolling-to-a-fragment
[^2]: https://html.spec.whatwg.org/#the-indicated-part-of-the-document
[^3]: https://html.spec.whatwg.org/#select-the-indicated-part
mgol added a commit to mgol/jquery-ui that referenced this issue Mar 26, 2025

Verified

This commit was signed with the committer’s verified signature.
mgol Michał Gołębiowski-Owczarek
Prior to jQuery UI 1.14.1, hashes in anchor hrefs were used directly. In
jquerygh-2307, that was changed - by decoding - to support more complex IDs, e.g.
containing emojis which are automatically encoded in `anchor.hash`.
Unfortunately, that broke cases where the panel ID is decoded as well.

It turns out the spec mandates checking both. In the "scrolling to a fragment"
section of the HTML spec[^1]. That uses a concept of document's indicated
part[^2]. Slightly below there's an algorithm to compute the indicated part[^3].
The interesting parts are steps 4 to 9:
4. Let potentialIndicatedElement be the result of finding a potential
   indicated element given document and fragment.
5. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.
6. Let fragmentBytes be the result of percent-decoding fragment.
7. Let decodedFragment be the result of running UTF-8 decode without BOM on
   fragmentBytes.
8. Set potentialIndicatedElement to the result of finding a potential indicated
   element given document and decodedFragment.
9. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.

First, in steps 4-5, the algorithm tries the hash as-is, without decoding. Then,
if one is not found, the same is attempted with a decoded hash.

This change replicates this logic by first trying the hash as-is and then
decoding it.

Fixes jquerygh-2344
Ref jquerygh-2307

[^1]: https://html.spec.whatwg.org/#scrolling-to-a-fragment
[^2]: https://html.spec.whatwg.org/#the-indicated-part-of-the-document
[^3]: https://html.spec.whatwg.org/#select-the-indicated-part
mgol added a commit to mgol/jquery-ui that referenced this issue Mar 26, 2025

Verified

This commit was signed with the committer’s verified signature.
mgol Michał Gołębiowski-Owczarek
Prior to jQuery UI 1.14.1, hashes in anchor hrefs were used directly. In
jquerygh-2307, that was changed - by decoding - to support more complex IDs, e.g.
containing emojis which are automatically encoded in `anchor.hash`.
Unfortunately, that broke cases where the panel ID is decoded as well.

It turns out the spec mandates checking both. In the "scrolling to a fragment"
section of the HTML spec[^1]. That uses a concept of document's indicated
part[^2]. Slightly below there's an algorithm to compute the indicated part[^3].
The interesting parts are steps 4 to 9:
4. Let potentialIndicatedElement be the result of finding a potential
   indicated element given document and fragment.
5. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.
6. Let fragmentBytes be the result of percent-decoding fragment.
7. Let decodedFragment be the result of running UTF-8 decode without BOM on
   fragmentBytes.
8. Set potentialIndicatedElement to the result of finding a potential indicated
   element given document and decodedFragment.
9. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.

First, in steps 4-5, the algorithm tries the hash as-is, without decoding. Then,
if one is not found, the same is attempted with a decoded hash.

This change replicates this logic by first trying the hash as-is and then
decoding it.

Fixes jquerygh-2344
Ref jquerygh-2307

[^1]: https://html.spec.whatwg.org/#scrolling-to-a-fragment
[^2]: https://html.spec.whatwg.org/#the-indicated-part-of-the-document
[^3]: https://html.spec.whatwg.org/#select-the-indicated-part
mgol added a commit to mgol/jquery-ui that referenced this issue Mar 26, 2025

Verified

This commit was signed with the committer’s verified signature.
mgol Michał Gołębiowski-Owczarek
Prior to jQuery UI 1.14.1, hashes in anchor hrefs were used directly. In
jquerygh-2307, that was changed - by decoding - to support more complex IDs, e.g.
containing emojis which are automatically encoded in `anchor.hash`.
Unfortunately, that broke cases where the panel ID is decoded as well.

It turns out the spec mandates checking both. In the "scrolling to a fragment"
section of the HTML spec[^1]. That uses a concept of document's indicated
part[^2]. Slightly below there's an algorithm to compute the indicated part[^3].
The interesting parts are steps 4 to 9:
4. Let potentialIndicatedElement be the result of finding a potential
   indicated element given document and fragment.
5. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.
6. Let fragmentBytes be the result of percent-decoding fragment.
7. Let decodedFragment be the result of running UTF-8 decode without BOM on
   fragmentBytes.
8. Set potentialIndicatedElement to the result of finding a potential indicated
   element given document and decodedFragment.
9. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.

First, in steps 4-5, the algorithm tries the hash as-is, without decoding. Then,
if one is not found, the same is attempted with a decoded hash.

This change replicates this logic by first trying the hash as-is and then
decoding it.

Fixes jquerygh-2344
Ref jquerygh-2307

[^1]: https://html.spec.whatwg.org/#scrolling-to-a-fragment
[^2]: https://html.spec.whatwg.org/#the-indicated-part-of-the-document
[^3]: https://html.spec.whatwg.org/#select-the-indicated-part
mgol added a commit to mgol/jquery-ui that referenced this issue Mar 26, 2025

Verified

This commit was signed with the committer’s verified signature.
mgol Michał Gołębiowski-Owczarek
Prior to jQuery UI 1.14.1, hashes in anchor hrefs were used directly. In
jquerygh-2307, that was changed - by decoding - to support more complex IDs, e.g.
containing emojis which are automatically encoded in `anchor.hash`.
Unfortunately, that broke cases where the panel ID is decoded as well.

It turns out the spec mandates checking both. In the "scrolling to a fragment"
section of the HTML spec[^1]. That uses a concept of document's indicated
part[^2]. Slightly below there's an algorithm to compute the indicated part[^3].
The interesting parts are steps 4 to 9:
4. Let potentialIndicatedElement be the result of finding a potential
   indicated element given document and fragment.
5. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.
6. Let fragmentBytes be the result of percent-decoding fragment.
7. Let decodedFragment be the result of running UTF-8 decode without BOM on
   fragmentBytes.
8. Set potentialIndicatedElement to the result of finding a potential indicated
   element given document and decodedFragment.
9. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.

First, in steps 4-5, the algorithm tries the hash as-is, without decoding. Then,
if one is not found, the same is attempted with a decoded hash.

This change replicates this logic by first trying the hash as-is and then
decoding it.

Fixes jquerygh-2344
Ref jquerygh-2307

[^1]: https://html.spec.whatwg.org/#scrolling-to-a-fragment
[^2]: https://html.spec.whatwg.org/#the-indicated-part-of-the-document
[^3]: https://html.spec.whatwg.org/#select-the-indicated-part
@mgol
Copy link
Member

mgol commented Mar 26, 2025

@tagliala I have a fix up at #2345, could you verify it fixes the issues you have?

mgol added a commit to mgol/jquery-ui that referenced this issue Mar 26, 2025

Verified

This commit was signed with the committer’s verified signature.
mgol Michał Gołębiowski-Owczarek
Prior to jQuery UI 1.14.1, hashes in anchor hrefs were used directly. In
jquerygh-2307, that was changed - by decoding - to support more complex IDs, e.g.
containing emojis which are automatically encoded in `anchor.hash`.
Unfortunately, that broke cases where the panel ID is decoded as well.

It turns out the spec mandates checking both. In the "scrolling to a fragment"
section of the HTML spec[^1]. That uses a concept of document's indicated
part[^2]. Slightly below there's an algorithm to compute the indicated part[^3].
The interesting parts are steps 4 to 9:
4. Let potentialIndicatedElement be the result of finding a potential
   indicated element given document and fragment.
5. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.
6. Let fragmentBytes be the result of percent-decoding fragment.
7. Let decodedFragment be the result of running UTF-8 decode without BOM on
   fragmentBytes.
8. Set potentialIndicatedElement to the result of finding a potential indicated
   element given document and decodedFragment.
9. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.

First, in steps 4-5, the algorithm tries the hash as-is, without decoding. Then,
if one is not found, the same is attempted with a decoded hash.

This change replicates this logic by first trying the hash as-is and then
decoding it.

Fixes jquerygh-2344
Ref jquerygh-2307

[^1]: https://html.spec.whatwg.org/#scrolling-to-a-fragment
[^2]: https://html.spec.whatwg.org/#the-indicated-part-of-the-document
[^3]: https://html.spec.whatwg.org/#select-the-indicated-part
@tagliala
Copy link
Author

tagliala commented Mar 29, 2025

Hello,

thanks for the fix and apologies for the late reply. I was monitoring this issue but I've somehow missed the notification.

I confirm that #2345 fixes the issue we were having on ActiveAdmin.

You can see the CI pass here: https://github.com/activeadmin/activeadmin/actions/runs/14146757191

Tested by replacing v1.14.1 with the dev file from the PR: activeadmin/activeadmin@7aa7778

Additionally, I've manually tested and the feature appears to correctly work

Before (v1.14.1) ❌

Image

After (#2345) ✅

Image

@mgol
Copy link
Member

mgol commented Mar 29, 2025

Thanks for confirming!

mgol added a commit to mgol/jquery-ui that referenced this issue Mar 31, 2025

Verified

This commit was signed with the committer’s verified signature.
mgol Michał Gołębiowski-Owczarek
Prior to jQuery UI 1.14.1, hashes in anchor hrefs were used directly. In
jquerygh-2307, that was changed - by decoding - to support more complex IDs, e.g.
containing emojis which are automatically encoded in `anchor.hash`.
Unfortunately, that broke cases where the panel ID is decoded as well.

It turns out the spec mandates checking both. In the "scrolling to a fragment"
section of the HTML spec[^1]. That uses a concept of document's indicated
part[^2]. Slightly below there's an algorithm to compute the indicated part[^3].
The interesting parts are steps 4 to 9:
4. Let potentialIndicatedElement be the result of finding a potential
   indicated element given document and fragment.
5. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.
6. Let fragmentBytes be the result of percent-decoding fragment.
7. Let decodedFragment be the result of running UTF-8 decode without BOM on
   fragmentBytes.
8. Set potentialIndicatedElement to the result of finding a potential indicated
   element given document and decodedFragment.
9. If potentialIndicatedElement is not null, then return
   potentialIndicatedElement.

First, in steps 4-5, the algorithm tries the hash as-is, without decoding. Then,
if one is not found, the same is attempted with a decoded hash.

This change replicates this logic by first trying the hash as-is and then
decoding it.

Fixes jquerygh-2344
Closes jquerygh-2345
Ref jquerygh-2307

[^1]: https://html.spec.whatwg.org/#scrolling-to-a-fragment
[^2]: https://html.spec.whatwg.org/#the-indicated-part-of-the-document
[^3]: https://html.spec.whatwg.org/#select-the-indicated-part
@mgol mgol closed this as completed in 89b0eca Mar 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment