Bug 83930 - Fragment navigation involving fragment identifiers does not match specification
Summary: Fragment navigation involving fragment identifiers does not match specification
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Rob Buis
URL: http://hixie.ch/tests/adhoc/uri/fragm...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-04-13 12:24 PDT by Pravin D
Modified: 2020-07-02 05:39 PDT (History)
13 users (show)

See Also:


Attachments
Patch (6.26 KB, patch)
2012-04-13 13:25 PDT, Pravin D
no flags Details | Formatted Diff | Diff
Patch (10.81 KB, patch)
2012-04-16 09:52 PDT, Pravin D
no flags Details | Formatted Diff | Diff
Patch (6.18 KB, patch)
2020-05-26 13:22 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (7.08 KB, patch)
2020-05-27 00:55 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.44 KB, patch)
2020-06-07 09:00 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (6.45 KB, patch)
2020-06-29 03:07 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (9.11 KB, patch)
2020-06-29 10:56 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (16.27 KB, patch)
2020-07-01 08:35 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (15.91 KB, patch)
2020-07-02 02:28 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (15.43 KB, patch)
2020-07-02 02:34 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pravin D 2012-04-13 12:24:20 PDT
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'.
Comment 1 Pravin D 2012-04-13 12:28:23 PDT
FF navigates to the correct anchor element. Chrome and Safari do not navigate properly.
Comment 2 Pravin D 2012-04-13 13:25:11 PDT
Created attachment 137135 [details]
Patch
Comment 3 Alexey Proskuryakov 2012-04-13 15:56:37 PDT
What does Internet Explorer do? Changing WebKit just for Firefox compatibility is not always the correct move.
Comment 4 Pravin D 2012-04-14 17:36:59 PDT
(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 ?
Comment 5 Pravin D 2012-04-15 07:19:14 PDT
Checked the behavior on Opera(windows). It seems behave similar to FF. The test case passes...
Comment 6 Pravin D 2012-04-16 09:52:24 PDT
Created attachment 137356 [details]
Patch
Comment 7 Pravin D 2012-04-16 10:26:47 PDT
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....
Comment 8 Alexey Proskuryakov 2012-04-16 10:39:00 PDT
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?
Comment 9 Pravin D 2012-04-16 13:38:41 PDT
(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...
Comment 10 Eric Seidel (no email) 2012-04-19 16:48:51 PDT
Adam Barth is your man.  he's on vacation at the moment.
Comment 11 Pravin D 2012-04-19 23:55:01 PDT
(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
Comment 12 Adam Barth 2012-05-04 09:37:47 PDT
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.
Comment 13 Pravin D 2012-05-04 10:24:46 PDT
(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.
Comment 14 Eric Seidel (no email) 2012-08-22 15:35:11 PDT
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.
Comment 15 Adam Barth 2012-08-22 15:46:58 PDT
Comment on attachment 137356 [details]
Patch

The path forward here is to get browsers to agree to work the same.
Comment 16 Simon Pieters (:zcorpan) 2014-10-07 01:52:43 PDT
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26988
Comment 17 Rob Buis 2020-05-26 13:22:58 PDT
Created attachment 400269 [details]
Patch
Comment 18 Darin Adler 2020-05-26 14:22:39 PDT
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.
Comment 19 Rob Buis 2020-05-27 00:55:23 PDT
Created attachment 400313 [details]
Patch
Comment 20 Rob Buis 2020-06-07 09:00:11 PDT
Created attachment 401296 [details]
Patch
Comment 21 Rob Buis 2020-06-29 03:07:31 PDT
Created attachment 403046 [details]
Patch
Comment 22 Rob Buis 2020-06-29 10:56:42 PDT
Created attachment 403084 [details]
Patch
Comment 23 Darin Adler 2020-06-30 12:40:02 PDT
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.
Comment 24 Darin Adler 2020-06-30 12:40:30 PDT
Title of this bug is unclear that the problem is specific to non-UTF-8-encoded webpages.
Comment 25 Rob Buis 2020-07-01 08:35:20 PDT
Created attachment 403295 [details]
Patch
Comment 26 EWS Watchlist 2020-07-01 08:36:11 PDT
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
Comment 27 Rob Buis 2020-07-02 02:28:55 PDT
Created attachment 403357 [details]
Patch
Comment 28 Rob Buis 2020-07-02 02:34:19 PDT
Created attachment 403358 [details]
Patch
Comment 29 EWS 2020-07-02 05:38:12 PDT
Committed r263841: <https://trac.webkit.org/changeset/263841>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403358 [details].
Comment 30 Radar WebKit Bug Importer 2020-07-02 05:39:22 PDT
<rdar://problem/65030734>