Bug 83930 - Fragment navigation does not work properly when the fragment identifier contain hex values
: Fragment navigation does not work properly when the fragment identifier conta...
Status: UNCONFIRMED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: http://hixie.ch/tests/adhoc/uri/fragm...
:
:
:
  Show dependency treegraph
 
Reported: 2012-04-13 12:24 PST by
Modified: 2012-08-22 15:46 PST (History)


Attachments
Patch (6.26 KB, patch)
2012-04-13 13:25 PST, Pravin D
no flags Review Patch | Details | Formatted Diff | Diff
Patch (10.81 KB, patch)
2012-04-16 09:52 PST, Pravin D
abarth: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-13 12:24:20 PST
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 From 2012-04-13 12:28:23 PST -------
FF navigates to the correct anchor element. Chrome and Safari do not navigate properly.
------- Comment #2 From 2012-04-13 13:25:11 PST -------
Created an attachment (id=137135) [details]
Patch
------- Comment #3 From 2012-04-13 15:56:37 PST -------
What does Internet Explorer do? Changing WebKit just for Firefox compatibility is not always the correct move.
------- Comment #4 From 2012-04-14 17:36:59 PST -------
(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 From 2012-04-15 07:19:14 PST -------
Checked the behavior on Opera(windows). It seems behave similar to FF. The test case passes...
------- Comment #6 From 2012-04-16 09:52:24 PST -------
Created an attachment (id=137356) [details]
Patch
------- Comment #7 From 2012-04-16 10:26:47 PST -------
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 From 2012-04-16 10:39:00 PST -------
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 From 2012-04-16 13:38:41 PST -------
(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 From 2012-04-19 16:48:51 PST -------
Adam Barth is your man.  he's on vacation at the moment.
------- Comment #11 From 2012-04-19 23:55:01 PST -------
(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 From 2012-05-04 09:37:47 PST -------
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 From 2012-05-04 10:24:46 PST -------
(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 From 2012-08-22 15:35:11 PST -------
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 From 2012-08-22 15:46:58 PST -------
(From update of attachment 137356 [details])
The path forward here is to get browsers to agree to work the same.