Bug 83930 – Fragment navigation involving fragment identifiers does not match specification

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Pravin D]

Description:
Suppose a page has 2 anchor/paragraph elements one of them having the id as "h" and another having id as "%68" , i.e hex equivalent of 'h'. When the browser tries to do a fragment navigation whose value is hex encoded 'h'(%68), then the page should scroll to the the element having id='h' and not to the element having id='%68'.

[Pravin D]

FF navigates to the correct anchor element. Chrome and Safari do not navigate properly.

[Pravin D]

Created attachment 137135 [details]
Patch

[Alexey Proskuryakov]

What does Internet Explorer do? Changing WebKit just for Firefox compatibility is not always the correct move.

[Pravin D]

(In reply to comment #3)
> What does Internet Explorer do? Changing WebKit just for Firefox compatibility is not always the correct move.

Internet explorer behaves fails the above test case. 
It expected for the fragment component to contain hex encoded text so, using it directly without decoding is a good idea ?

[Pravin D]

Checked the behavior on Opera(windows). It seems behave similar to FF. The test case passes...

[Pravin D]

Created attachment 137356 [details]
Patch

[Pravin D]

Here is the link to the spec that talks about fragment navigation:
http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#scroll-to-fragid

The algorithm for fragment navigation is given in 7 steps. Webkit adheres to them expect for step 4 and 5.

These steps can be broken into 2 parts each:
Step 4 
a. Use hex decoded fragId.
b. Find an element whose id attr matches this decoded value.
If not found do step 5

Step 5
a. Use original fragId(non-decoded).
b. Find an anchor element(<a>) whose name attr matches this value.


Currently Webkit does the following:

A .Use original fragId 
B. find an element whose id attr matches this value or find an anchor tag whose name attr matches this value.

If not found Use the decode the fragId and do step (B).

So on webkit the steps 4.a and 5.a are swapped and 4.b and 5.b are not followed properly.


As I was not sure how closely we have to adhere to the above spec , I have uploaded a patch iwhich does the following:

X .Use decoded fragId 
Y. find an element whose id attr matches this value or find and anchor tag whose name matches this value.

If not found Use the original fragId and do step (Y).

Also contains 2 testcases to verify the same.

Please let know how to proceed about....

[Alexey Proskuryakov]

How did you make the decision that we should fix WebKit, and not the spec?

I don't oppose the proposed change per se, but we need stronger rationale to know whether this is something we'll stand behind. For example, if we take this change and it breaks an important site, will we be willing to roll it out? And what if it's an unimportant site?

[Pravin D]

(In reply to comment #8)
> How did you make the decision that we should fix WebKit, and not the spec?
> 
> I don't oppose the proposed change per se, but we need stronger rationale to know whether this is something we'll stand behind. For example, if we take this change and it breaks an important site, will we be willing to roll it out? And what if it's an unimportant site?

Just for information sake:

Tracked the logic used in the function to this 
http://trac.webkit.org/changeset/6479/trunk/WebCore/khtml/khtml_part.cpp

This is the beginning of the the gotoAnchor() function which is now changed to scrollToAnchor()... 

As for the logic used for scrolling to anchor seems to part of kde code base...
http://trac.webkit.org/browser/trunk/WebCore/khtml/khtml_part.cpp?rev=1035

Anyways the only rationale that i have to offer is that this part of the code pretty old and won't do much harm if someone poked it and bring it in terms with the current Web world...

[Eric Seidel (no email)]

Adam Barth is your man.  he's on vacation at the moment.

[Pravin D]

(In reply to comment #10)
> Adam Barth is your man.  he's on vacation at the moment.

Oh ok... thanks for the update. I'll follow it up with Adam

[Adam Barth]

To summarize:

1) Firefox, Opera, and the spec agree.
2) Safari, Chrome, and IE agree on the reverse.
3) We don't know of any web sites that are affected one way or the other.

Is that understanding correct?

My guess is that it will be harder to change IE than the spec, so perhaps we should ask the Firefox and Opera folks if they'd be willing to match IE if we changed the spec to match IE.

[Pravin D]

(In reply to comment #12)
> To summarize:
> 
> 1) Firefox, Opera, and the spec agree.
> 2) Safari, Chrome, and IE agree on the reverse.
> 3) We don't know of any web sites that are affected one way or the other.
> 
> Is that understanding correct?
> 
> My guess is that it will be harder to change IE than the spec, so perhaps we should ask the Firefox and Opera folks if they'd be willing to match IE if we changed the spec to match IE.

Hi Adam,
Thanks for your time. FF and Opera also do not agree fully with spec!!!
The spec says(as I have already pointed out, sorry for the redundancy) that first check for  following:

These steps can be broken into 2 parts each:
Step 4 
a. Use hex decoded fragId.
b. Find an element whose id attr matches this decoded value.
If not found do step 5

Step 5
a. Use original fragId(non-decoded).
b. Find an anchor element(<a>) whose name attr matches this value.

But FF and Opera only check for decoded fragId and IE checks for non decoded fragId(I use non decoded in the sense that fragId may not contain hex encoded string) and WebKit uses the fragId in the reverse order, i.e 1st uses non-decoded fragId to check a matching anchor element and then uses a decoded fragId to do the same if the anchor Element is not found. 


PS: The logic used in Webkit seems to be very old, about 10yrs(comment #9), that does not mean its wrong, just that its not par with the current spec(maybe the current spec might be wrong as Alexey commented).

So conclusion:
IE is on 1 extreme and FF is on another. WebKit seems to follow the middle ground, not guided by spec nor following other browsers.

[Eric Seidel (no email)]

I don't understand the path forward here.  Adam asked if it woudl be possible to get the other browsers to change to match IE.  It sounds like no browser matches the spec as-is.

[Adam Barth]

Comment on attachment 137356 [details]
Patch

The path forward here is to get browsers to agree to work the same.

[Simon Pieters (:zcorpan)]

https://www.w3.org/Bugs/Public/show_bug.cgi?id=26988

[Rob Buis]

Created attachment 400269 [details]
Patch

[Darin Adler]

Comment on attachment 400269 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400269&action=review

> Source/WebCore/page/FrameView.cpp:2208
>      Element* anchorElement = document.findAnchor(fragmentIdentifier);
> +    if (!anchorElement)
> +        anchorElement = document.findAnchor(decodeURLEscapeSequences(fragmentIdentifier));

I would think we’d want the findAnchor function to implement this rule, rather than putting it here. Unless for some reason the 3 or 4 other call sites don’t also need this logic.

[Rob Buis]

Created attachment 400313 [details]
Patch

[Rob Buis]

Created attachment 401296 [details]
Patch

[Rob Buis]

Created attachment 403046 [details]
Patch

[Rob Buis]

Created attachment 403084 [details]
Patch

[Darin Adler]

Comment on attachment 403084 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403084&action=review

> LayoutTests/ChangeLog:12
> +        * http/tests/navigation/anchor-frames-gbk-expected.txt:
> +        * http/tests/navigation/resources/frame-with-anchor-gbk.html:

The revised test is now unclear about what it tests. It describes itself: "Tests that loading a frame with a URL that contains a fragment pointed at a named anchor actually scrolls to that anchor." That doesn't seem accurate; doesn’t even mention the character set. And test doesn’t say *why* it should not scroll. I’d almost prefer deleting this test, if it doesn’t add value over the fragment identifier tests in WPT.

[Darin Adler]

Title of this bug is unclear that the problem is specific to non-UTF-8-encoded webpages.

[Rob Buis]

Created attachment 403295 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Rob Buis]

Created attachment 403357 [details]
Patch

[Rob Buis]

Created attachment 403358 [details]
Patch

[EWS]

Committed r263841: <https://trac.webkit.org/changeset/263841>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403358 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65030734>