Allow fragment navigation from <svg:a>

Allow fragment navigation from <svg:a>

This essentially reverts the fix for WK bug #19995 while adding a test for
the (new) expected behavior.

BUG=296071
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172722

[pdr., 2014-04-23 17:22:45]

On 2014/04/23 16:50:35, fs wrote:

I think this makes sense, but I'd like to make sure we are on the right side of
the spec here.

My reading of
https://svgwg.org/svg2-draft/single-page.html#linking-SVGFragmentIdentifiers is
that we shouldn't do this. Is there some spec text that says we should?

[fs, 2014-04-24 07:35:22]

On 2014/04/23 17:22:45, pdr wrote:
> On 2014/04/23 16:50:35, fs wrote:
> 
> I think this makes sense, but I'd like to make sure we are on the right side
of
> the spec here.
> 
> My reading of
> https://svgwg.org/svg2-draft/single-page.html#linking-SVGFragmentIdentifiers
is
> that we shouldn't do this. Is there some spec text that says we should?

I think the relevant part of that section is:

"If the SVG fragment identifier addresses any element other than a ‘view’
element, then the document defined by the closest ancestor ‘svg’ element is
displayed in the viewport using the view specification attributes on that ‘svg’
element."

I however think that the section about the 'a' element (18.2) is more
interesting in this case, and it says:

"The remote resource may be any Web resource (e.g., an image, a video clip, a
sound bite, a program, another SVG document, an HTML document,
_an_element_within_the_current_document_, an element within a different
document, etc.)."

which I would interpret as allowing the navigation. (Rather, it doesn't really
appear to disallow any form of navigation.)

There's also at least one upstream/spec tests for this (linking-a-04-t.svg -
second/middle arrow), and it still passes (and arguably better than before
because now it actually navigates to the fragment.)

[pdr., 2014-04-24 19:40:05]

I understand now, and you're right. Sorry about delaying this patch for a day :/

I think this patch is heading in the right direction.

https://codereview.chromium.org/249313005/diff/1/LayoutTests/svg/custom/fragm...
File LayoutTests/svg/custom/fragment-navigation-01.html (right):

https://codereview.chromium.org/249313005/diff/1/LayoutTests/svg/custom/fragm...
LayoutTests/svg/custom/fragment-navigation-01.html:5: <a xlink:href="#test">
I think this test would pass even without the patch because there's no element
width id test so targetElement would be null. Could you add a second test that
the page correctly navigates to the target fragment? In my tests we just go to
the bottom of the page.

[fs, 2014-04-25 08:00:09]

https://codereview.chromium.org/249313005/diff/1/LayoutTests/svg/custom/fragm...
File LayoutTests/svg/custom/fragment-navigation-01.html (right):

https://codereview.chromium.org/249313005/diff/1/LayoutTests/svg/custom/fragm...
LayoutTests/svg/custom/fragment-navigation-01.html:5: <a xlink:href="#test">
On 2014/04/24 19:40:05, pdr wrote:
> I think this test would pass even without the patch because there's no element
> width id test so targetElement would be null. Could you add a second test that
> the page correctly navigates to the target fragment? In my tests we just go to
> the bottom of the page.

Added an additional test similar to the TC in the bug.

[pdr., 2014-04-28 03:09:12]

On 2014/04/25 08:00:09, fs wrote:
>
https://codereview.chromium.org/249313005/diff/1/LayoutTests/svg/custom/fragm...
> File LayoutTests/svg/custom/fragment-navigation-01.html (right):
> 
>
https://codereview.chromium.org/249313005/diff/1/LayoutTests/svg/custom/fragm...
> LayoutTests/svg/custom/fragment-navigation-01.html:5: <a xlink:href="#test">
> On 2014/04/24 19:40:05, pdr wrote:
> > I think this test would pass even without the patch because there's no
element
> > width id test so targetElement would be null. Could you add a second test
that
> > the page correctly navigates to the target fragment? In my tests we just go
to
> > the bottom of the page.
> 
> Added an additional test similar to the TC in the bug.

LGTM

[pdr., 2014-04-28 03:09:16]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-04-28 03:09:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/249313005/60001

[commit-bot: I haz the power, 2014-04-28 03:19:03]

Change committed as 172722