1380323 - Urlencoded anchors in URL don't scroll to content

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Michał Muskała]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36

Steps to reproduce:

Paste the following link in the location bar and open the page:

https://hexdocs.pm/elixir/Kernel.SpecialForms.html#%3C%3C%3E%3E/1

The page contains an <a> element with a href="#%3C%3C%3E%3E/1" attribute.


Actual results:

The page is opened at the top as if there was no anchor.


Expected results:

The page should scroll to the place where the anchor is.

Comment 1

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Hi Alphan, would you like to give it a try?

Comment 2

[Alphan Chen [:alchen]]

sure!!(In reply to Hsin-Yi Tsai (55 Regression Engineering support) [:hsinyi] from comment #1)
> Hi Alphan, would you like to give it a try?

Sure !!

Comment 3

[Alphan Chen [:alchen]]

Update the finding:

If we replace the id from "%3C%3C%3E%3/1" to "<<>>/1", the page would scroll to the right place.

From:
<h3 id="%3C%3C%3E%3E/1-performance-optimizations" class="section-heading">
To:
<h3 id="<<>>/1-performance-optimizations" class="section-heading">

Comment 4

[Alphan Chen [:alchen]]

I have a patch to fix this symptom.

However, we cannot pass the following wpt-test if applying the patch.
http://w3c-test.org/html/browsers/browsing-the-web/scroll-to-fragid/scroll-frag-percent-encoded.html

One more thing, edge has the same symptom as well.

Comment 5

[Alphan Chen [:alchen]]

According the algorithm, the behavior of firefox should be correct.
https://www.w3.org/TR/2011/WD-html5-20110113/history.html#the-indicated-part-of-the-document

3. Let decoded fragid be the result of expanding any sequences of percent-encoded octets in fragid that are valid UTF-8 sequences into Unicode characters as defined by UTF-8. If any percent-encoded octets in that string are not valid UTF-8 sequences, then skip this step and the next one.

Comment 6

[Alphan Chen [:alchen]]

(In reply to Alphan Chen [:alchen] from comment #5)
> According the algorithm, the behavior of firefox should be correct.
> https://www.w3.org/TR/2011/WD-html5-20110113/history.html#the-indicated-part-
> of-the-document
> 
> 3. Let decoded fragid be the result of expanding any sequences of
> percent-encoded octets in fragid that are valid UTF-8 sequences into Unicode
> characters as defined by UTF-8. If any percent-encoded octets in that string
> are not valid UTF-8 sequences, then skip this step and the next one.

Hi Anne,
need your help for this bug.

Currently, we cannot scroll to the anchor that the website wants.
(https://hexdocs.pm/elixir/Kernel.SpecialForms.html#%3C%3C%3E%3E/1)
The behavior of Firefox is correct by the spec.
However, Chrome and Safari both can scroll to the right anchor.


There are some options that we can do here.
1. Do nothing. Since we are doing right according to the spec, website developer should follow it.
2. Refine the algorithm in the spec. We could try to find the anchor by original string(without unescape one. in this case, "%3C%3C%3E%3/1") first. Then do unescape one. In this option, we also need to refine the wpt test.(http://w3c-test.org/html/browsers/browsing-the-web/scroll-to-fragid/scroll-frag-percent-encoded.html)
3. Just add one more trial(find the anchor by original string) if we cannot find the anchor.

(In reply to Alphan Chen [:alchen] from comment #4)
> I have a patch to fix this symptom.
> However, we cannot pass the following wpt-test if applying the patch.
> http://w3c-test.org/html/browsers/browsing-the-web/scroll-to-fragid/scroll-
> frag-percent-encoded.html
> 
Edge can pass the test. Chrome/Safari cannot.
We check the Chrome's code. They will try to find the anchor by original string first(in this case, "%3C%3C%3E%3/1"). Then try the document's charset(in this case, "<<>>/1").

Comment 7

[Anne (:annevk)]

https://html.spec.whatwg.org/multipage/browsing-the-web.html#the-indicated-part-of-the-document is a better reference, though in this case they happen to be similar.

I suspect Edge first tries the decoded variant and then the original string. (Though it seems to do the opposite for :target matching. Or maybe :target matching doesn't decode at all, unclear. I didn't test it deeply.)

I think we should match Chrome/Safari and update the standard and that test. (I'll work on a PR for the standard as a start.)

Comment 8

[Anne (:annevk)]

Created https://github.com/whatwg/html/pull/2901 with a revised algorithm. Would you be willing to update the tests?

Might be good to expand that test to cover some other scenarios as well. E.g., that <a name=literal> is found before <a id=decoded> and such.

Comment 9

[Alphan Chen [:alchen]]

(In reply to Anne (:annevk) from comment #8)
> Created https://github.com/whatwg/html/pull/2901 with a revised algorithm.
> Would you be willing to update the tests?
> 
> Might be good to expand that test to cover some other scenarios as well.
> E.g., that <a name=literal> is found before <a id=decoded> and such.

OK, I will update the tests and create a pull request.

Comment 10

[Alphan Chen [:alchen]]

(In reply to Anne (:annevk) from comment #8)
> Created https://github.com/whatwg/html/pull/2901 with a revised algorithm.
> Would you be willing to update the tests?
> 
> Might be good to expand that test to cover some other scenarios as well.
> E.g., that <a name=literal> is found before <a id=decoded> and such.

Here is the pull request.
https://github.com/w3c/web-platform-tests/pull/6887

Comment 11

[Alphan Chen [:alchen]]

Attachment: bug1380323.patch

Here is the patch according to the revised algorithm.

We can pass the tests with this patch.
https://github.com/w3c/web-platform-tests/pull/6887

Comment 12

[Anne (:annevk)]

Thanks, sorry for the delay. I think your tests look good. Just waiting for someone to sign off on the HTML Standard change. You should feel free to go ahead with the Firefox change though since this is the way to go.

Comment 13

[Alphan Chen [:alchen]]

Attachment: Revise the behavior of scrolling to fragments

In this patch, we follow the latest algorithm[1].
The wpt test also needs to be updated[2].
Before pulling the latest wpt test, we expect the test would be failed.

[1] https://github.com/whatwg/html/pull/2901
[2] https://github.com/w3c/web-platform-tests/pull/6887

Comment 14

[(no longer active)]

Comment on attachment 8899747 [details] [diff] [review]
Revise the behavior of scrolling to fragments

Review of attachment 8899747 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +10011,5 @@
>    if (shell) {
> +    nsresult rv = NS_ERROR_FAILURE;
> +    // We assume that the bytes are in UTF-8, as it says in the spec:
> +    // http://www.w3.org/TR/html4/appendix/notes.html#h-B.2.1
> +    NS_ConvertUTF8toUTF16 ref(mScrollToRef);

Nit: You only need |ref| inside the if condition, so you could modify the condition itself to check mScrollToRef.IsEmpty() instead and then move the NS_ConvertUTF8toUTF16 to inside the if body to avoid doing that work in the else case.

Comment 15

[Alphan Chen [:alchen]]

Comment on attachment 8899747 [details] [diff] [review]
Revise the behavior of scrolling to fragments

Review of attachment 8899747 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +10011,5 @@
>    if (shell) {
> +    nsresult rv = NS_ERROR_FAILURE;
> +    // We assume that the bytes are in UTF-8, as it says in the spec:
> +    // http://www.w3.org/TR/html4/appendix/notes.html#h-B.2.1
> +    NS_ConvertUTF8toUTF16 ref(mScrollToRef);

Actually, we check mScrollToRef.IsEmpty() in line 10006.
The if checking is for "empty string which might be caused by the UTF-8 conversion".

Comment 16

[Alphan Chen [:alchen]]

Attachment: Revise the behavior of scrolling to fragments. r=ehsan

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29ffcc3cd2833d590372a567c409320fc1278a7f

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44cc06afd53d
Revise the behavior of scrolling to fragments. r=ehsan

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/44cc06afd53d