Bug 63322 – SVG1.1SE test linking-uri-01-b.svg fails

• Component: SVG
• Status: RESOLVED FIXED

[Rob Buis]

Currently we only handle targets in same document to animation elements, should also work for <view> as direct target.

[Rob Buis]

Created attachment 98496 [details]
Patch

[WebKit Review Bot]

Comment on attachment 98496 [details]
Patch

Attachment 98496 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8939390

New failing tests:
svg/W3C-SVG-1.1-SE/linking-uri-01-b.svg
svg/custom/linking-a-03-b-all.svg
svg/custom/linking-a-03-b-transform.svg
svg/custom/linking-a-03-b-viewBox-transform.svg

[WebKit Review Bot]

Created attachment 98498 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dirk Schulze]

Comment on attachment 98496 [details]
Patch

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

r- because of a missing DRT test.

> Source/WebCore/svg/SVGSVGElement.cpp:630
> +    bool hadUseCurrentView = m_useCurrentView;

no need to set it here. Variable is not used for if and else if case. And now that I look at it, you don't need it at all

> Source/WebCore/svg/SVGSVGElement.cpp:632
> +        // We need to parse the xpointer reference here

Seems to be a FIXME.

> Source/WebCore/svg/SVGSVGElement.cpp:634
> +        if (!currentView()->parseViewSpec(name))

can we avoid the parsing of svgView( a second time in parseViewSpec?

> Source/WebCore/svg/SVGSVGElement.cpp:639
> +            RefPtr<SVGViewElement> viewElement = anchorNode->hasTagName(SVGNames::viewTag) ? static_cast<SVGViewElement*>(anchorNode) : 0;

Do you need to ref count it? You call .get() multiple times afterwards.

> Source/WebCore/svg/SVGSVGElement.cpp:645
> +                if (element->hasTagName(SVGNames::svgTag)) {
> +                    RefPtr<SVGSVGElement> svg = static_cast<SVGSVGElement*>(element);
> +                    svg->inheritViewAttributes(viewElement.get());
> +                }

We do nothing if the neares viewport element is not a svg-element? What else can happen and shouldn't we do something in this case?

> Source/WebCore/svg/SVGSVGElement.cpp:653
> +    // FIXME: need to decide which <svg> to focus on, and zoom to that one

You take the first SVG parent, can you write this here?

> Source/WebCore/svg/SVGSVGElement.cpp:654
> +    // FIXME: need to actually "highlight" the viewTarget(s)

Wat does it mean? What is 's'?

> LayoutTests/ChangeLog:6
> +        SVG1.1SE test linking-uri-01-b.svg fails
> +        https://bugs.webkit.org/show_bug.cgi?id=63322

I can see it without the changes. So your test is definitely not enough to test the new behavior, can add another DRT test?

[Rob Buis]

Hi Dirk,

(In reply to comment #4)
> (From update of attachment 98496 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98496&action=review
> 
> r- because of a missing DRT test.
> 
> > Source/WebCore/svg/SVGSVGElement.cpp:630
> > +    bool hadUseCurrentView = m_useCurrentView;
> 
> no need to set it here. Variable is not used for if and else if case. And now that I look at it, you don't need it at all

I think it is needed, it used later on.

> > Source/WebCore/svg/SVGSVGElement.cpp:632
> > +        // We need to parse the xpointer reference here
> 
> Seems to be a FIXME.

Yes, should be made clearer.

> > Source/WebCore/svg/SVGSVGElement.cpp:634
> > +        if (!currentView()->parseViewSpec(name))
> 
> can we avoid the parsing of svgView( a second time in parseViewSpec?

I'll have a look.

> > Source/WebCore/svg/SVGSVGElement.cpp:639
> > +            RefPtr<SVGViewElement> viewElement = anchorNode->hasTagName(SVGNames::viewTag) ? static_cast<SVGViewElement*>(anchorNode) : 0;
> 
> Do you need to ref count it? You call .get() multiple times afterwards.

Yeah, don't think it is needed, will have a look.

> > Source/WebCore/svg/SVGSVGElement.cpp:645
> > +                if (element->hasTagName(SVGNames::svgTag)) {
> > +                    RefPtr<SVGSVGElement> svg = static_cast<SVGSVGElement*>(element);
> > +                    svg->inheritViewAttributes(viewElement.get());
> > +                }
> 
> We do nothing if the neares viewport element is not a svg-element? What else can happen and shouldn't we do something in this case?

Not sure atm, but this logic worked before so I want to keep that part like it is.

> > Source/WebCore/svg/SVGSVGElement.cpp:653
> > +    // FIXME: need to decide which <svg> to focus on, and zoom to that one
> 
> You take the first SVG parent, can you write this here?

Also a moved FIXME, want to leave it alone for the moment.

> > Source/WebCore/svg/SVGSVGElement.cpp:654
> > +    // FIXME: need to actually "highlight" the viewTarget(s)
> 
> Wat does it mean? What is 's'?

ditto.

> > LayoutTests/ChangeLog:6
> > +        SVG1.1SE test linking-uri-01-b.svg fails
> > +        https://bugs.webkit.org/show_bug.cgi?id=63322
> 
> I can see it without the changes. So your test is definitely not enough to test the new behavior, can add another DRT test?

Agreed, trying to come up with one.
So in general links have some stuff to do, but OTOH I haven't really seen SVGs with xpointer etc. being used, so I propose to keep the FIXMEs around until we actually see testcases.
Cheers,

Rob.

[Dirk Schulze]

Comment on attachment 98496 [details]
Patch

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

>>> Source/WebCore/svg/SVGSVGElement.cpp:630
>>> +    bool hadUseCurrentView = m_useCurrentView;
>> 
>> no need to set it here. Variable is not used for if and else if case. And now that I look at it, you don't need it at all
> 
> I think it is needed, it used later on.

You use it in one condition but do not set m_useCurrentView up to this usage.

[Rob Buis]

(In reply to comment #6)
> (From update of attachment 98496 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98496&action=review
> 
> >>> Source/WebCore/svg/SVGSVGElement.cpp:630
> >>> +    bool hadUseCurrentView = m_useCurrentView;
> >> 
> >> no need to set it here. Variable is not used for if and else if case. And now that I look at it, you don't need it at all
> > 
> > I think it is needed, it used later on.
> 
> You use it in one condition but do not set m_useCurrentView up to this usage.

Well spotted! I think it was needed at one point because I always resetted to setUseCurrentView(false) at the start, but I changed the code since. Fixed.
Cheers,

Rob.

[Nikolas Zimmermann]

Comment on attachment 98496 [details]
Patch

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

Some more comments from me...

> Source/WebCore/page/FrameView.cpp:1464
> +        RefPtr<SVGSVGElement> svg = static_cast<SVGDocument*>(m_frame->document())->rootElement();

The refing here in RefPtr is needed? If so why? Can handleAnchorScroll potentially kill the SVGSVGElement?

> Source/WebCore/svg/SVGAElement.cpp:217
> +                if (!targetElement->hasTagName(SVGNames::viewTag))
> +                    return;

This deserves a comment :-)

> Source/WebCore/svg/SVGSVGElement.cpp:633
> +    } else if (name.startsWith("svgView(")) {

Also we try to avoid startsWith if possible, but I'm not sure this code path is hot enough to justify that.

> Source/WebCore/svg/SVGSVGElement.cpp:643
> +                    RefPtr<SVGSVGElement> svg = static_cast<SVGSVGElement*>(element);

Same question, why refing here? Doesn't make much sense IMHO.

>>> Source/WebCore/svg/SVGSVGElement.cpp:653
>>> +    // FIXME: need to decide which <svg> to focus on, and zoom to that one
>> 
>> You take the first SVG parent, can you write this here?
> 
> Also a moved FIXME, want to leave it alone for the moment.

Also you might want to write real english phrases using right punctation, as required by the style guide.

> Source/WebCore/svg/SVGSVGElement.cpp:674
> -    
> +

You can omit that change.

> Source/WebCore/svg/SVGSVGElement.h:123
> +    void handleAnchorScroll(String name, Element* anchorNode);

Please pass String by const String& reference.

[Rob Buis]

Created attachment 98545 [details]
Patch

[WebKit Review Bot]

Comment on attachment 98545 [details]
Patch

Attachment 98545 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8937539

New failing tests:
svg/W3C-SVG-1.1-SE/linking-uri-01-b.svg
svg/custom/linking-a-03-b-all.svg
svg/custom/linking-a-03-b-transform.svg
svg/custom/linking-a-03-b-viewBox-transform.svg

[WebKit Review Bot]

Created attachment 98558 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Comment on attachment 98545 [details]
Patch

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

Next rounds of comments ;-)

> Source/WebCore/page/FrameView.cpp:1464
> +        SVGSVGElement* svg = static_cast<SVGDocument*>(m_frame->document())->rootElement();

Out of curiosity: can this ever be null? Suppose we had an empty foo.svg document and load that? Can you try it?

> Source/WebCore/svg/SVGAElement.cpp:217
> +                // only allow navigation to internal <view> anchors

s/only/Only/ add trailing ".".

> Source/WebCore/svg/SVGSVGElement.cpp:631
> +        // FIXME: We need to parse the xpointer reference here

Ideally you directly create a bug report, and link it here.
FIXME: XPointer references are ignored (https://bugs.webkit.org/...

Can you add a return here?

> Source/WebCore/svg/SVGSVGElement.cpp:632
> +    } else if (name.startsWith("svgView(")) {

And remove the } else if, make it a new if ( section.

> Source/WebCore/svg/SVGSVGElement.cpp:635
> +        setUseCurrentView(true);

Hm, there are other places in SVGSVGElement that mutate the UseCurrentView value (the inheritViewAttribues method). How does this interfer? Is this related at all?

> Source/WebCore/svg/SVGSVGElement.cpp:645
> +            }

Also add a return; after this line to avoid ...

> Source/WebCore/svg/SVGSVGElement.cpp:646
> +        } else if (m_useCurrentView) {

.. this else if.

> Source/WebCore/svg/SVGSVGElement.cpp:654
> +    // FIXME: need to decide which <svg> to focus on, and zoom to that one
> +    // FIXME: need to actually "highlight" the viewTarget(s)

Can you make those valid english phrases, start with capital letter and end with punctation.

> Source/WebCore/svg/SVGSVGElement.h:123
> +    void handleAnchorScroll(const String& name, Element* anchorNode);

The method name is very unfortunate. What does handle mean in that context? Maybe "scrollToAnchor" is better?

[Rob Buis]

Hi Niko,

> Next rounds of comments ;-)

Sure :)

> > Source/WebCore/page/FrameView.cpp:1464
> > +        SVGSVGElement* svg = static_cast<SVGDocument*>(m_frame->document())->rootElement();
> 
> Out of curiosity: can this ever be null? Suppose we had an empty foo.svg document and load that? Can you try it?

Yes, looks like it can be null, see SVGDocument.cpp, will add a check.

> > Source/WebCore/svg/SVGAElement.cpp:217
> > +                // only allow navigation to internal <view> anchors
> 
> s/only/Only/ add trailing ".".

Fixed.

> > Source/WebCore/svg/SVGSVGElement.cpp:631
> > +        // FIXME: We need to parse the xpointer reference here
> 
> Ideally you directly create a bug report, and link it here.
> FIXME: XPointer references are ignored (https://bugs.webkit.org/...

Found a bug for this, fixed.

> Can you add a return here?

Fixed.

> > Source/WebCore/svg/SVGSVGElement.cpp:632
> > +    } else if (name.startsWith("svgView(")) {
> 
> And remove the } else if, make it a new if ( section.

Fixed.
 
> > Source/WebCore/svg/SVGSVGElement.cpp:635
> > +        setUseCurrentView(true);
> 
> Hm, there are other places in SVGSVGElement that mutate the UseCurrentView value (the inheritViewAttribues method). How does this interfer? Is this related at all?

Will need to check....

> > Source/WebCore/svg/SVGSVGElement.cpp:645
> > +            }
> 
> Also add a return; after this line to avoid ...

Fixed.

> > Source/WebCore/svg/SVGSVGElement.cpp:646
> > +        } else if (m_useCurrentView) {
> 
> .. this else if.

Fixed.

> > Source/WebCore/svg/SVGSVGElement.cpp:654
> > +    // FIXME: need to decide which <svg> to focus on, and zoom to that one
> > +    // FIXME: need to actually "highlight" the viewTarget(s)
> 
> Can you make those valid english phrases, start with capital letter and end with punctation.

Fixed.

> > Source/WebCore/svg/SVGSVGElement.h:123
> > +    void handleAnchorScroll(const String& name, Element* anchorNode);
> 
> The method name is very unfortunate. What does handle mean in that context? Maybe "scrollToAnchor" is better?

It is better. Still undecided, maybe something with View is needed? setupInitialView? Since the method does setup a view if needed.
Cheers,

Rob.

[Rob Buis]

Created attachment 98590 [details]
Patch

[WebKit Review Bot]

Comment on attachment 98590 [details]
Patch

Attachment 98590 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8932820

New failing tests:
svg/W3C-SVG-1.1-SE/linking-uri-01-b.svg
svg/custom/linking-a-03-b-viewBox-transform.svg
svg/custom/linking-a-03-b-all.svg
svg/custom/linking-a-03-b-transform.svg
svg/custom/linking-uri-01-b.svg

[WebKit Review Bot]

Created attachment 98591 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Comment on attachment 98590 [details]
Patch

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

Looks almost good, r=me with one condition: if it turns out my worries about markForLayoutAndParentResourceInvalidation are wrong, and this is the only place where this is needed, then go ahead and land it otherwhise let's discuss :-)

> Source/WebCore/ChangeLog:11
> +        Test: svg/W3C-SVG-1.1-SE/linking-uri-01-b.svg

The other new test is missing here.

> Source/WebCore/ChangeLog:18
> +        * page/FrameView.cpp: delegate to setupInitialView
> +        (WebCore::FrameView::scrollToAnchor):
> +        * svg/SVGAElement.cpp: allow navigating to internal <view> targets
> +        (WebCore::SVGAElement::defaultEventHandler):
> +        * svg/SVGSVGElement.cpp:
> +        (WebCore::SVGSVGElement::setupInitialView): initialize current view depending on anchor

Please use proper sentences as well here.

> Source/WebCore/page/FrameView.cpp:1460
> +    m_frame->document()->setCSSTarget(anchorNode); // Setting to null will clear the current target.

You can move the comment to its own line before that statement - I think this is preferred style.

> Source/WebCore/page/FrameView.cpp:1465
> +        SVGSVGElement* svg = static_cast<SVGDocument*>(m_frame->document())->rootElement();
> +        if (svg) {

You can write if (SVGSVGElement* svg = ...) { here to save another line :-)

> Source/WebCore/svg/SVGSVGElement.cpp:628
> +void SVGSVGElement::setupInitialView(const String& name, Element* anchorNode)

Sorry for not spotting it ealier but "name" is not very descriptive. "viewIdentifier" ? "viewTarget" ? I'm sure you can come up with a better name.

> Source/WebCore/svg/SVGSVGElement.cpp:651
> +        }

Add return; here.

> Source/WebCore/svg/SVGSVGElement.cpp:653
> +    } else if (hadUseCurrentView) {
> +        currentView()->setTransform(emptyString());

You can save this branch by using if (!hadUseCurrentView) early exit.

> Source/WebCore/svg/SVGSVGElement.cpp:655
> +        if (RenderObject* object = renderer())
> +            RenderSVGResource::markForLayoutAndParentResourceInvalidation(object);

Why do you only need this invalidation here, and not for the other branches?

[Rob Buis]

Hi Niko,

(In reply to comment #17)
> (From update of attachment 98590 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98590&action=review
> 
> Looks almost good, r=me with one condition: if it turns out my worries about markForLayoutAndParentResourceInvalidation are wrong, and this is the only place where this is needed, then go ahead and land it otherwhise let's discuss :-)

Ok.

> > Source/WebCore/ChangeLog:11
> > +        Test: svg/W3C-SVG-1.1-SE/linking-uri-01-b.svg
> 
> The other new test is missing here.

Yes, ran prepare-Changelog too early :( Will fix.

> > Source/WebCore/ChangeLog:18
> > +        * page/FrameView.cpp: delegate to setupInitialView
> > +        (WebCore::FrameView::scrollToAnchor):
> > +        * svg/SVGAElement.cpp: allow navigating to internal <view> targets
> > +        (WebCore::SVGAElement::defaultEventHandler):
> > +        * svg/SVGSVGElement.cpp:
> > +        (WebCore::SVGSVGElement::setupInitialView): initialize current view depending on anchor
> 
> Please use proper sentences as well here.

Ok.

> > Source/WebCore/page/FrameView.cpp:1460
> > +    m_frame->document()->setCSSTarget(anchorNode); // Setting to null will clear the current target.
> 
> You can move the comment to its own line before that statement - I think this is preferred style.

Fixed.

> > Source/WebCore/page/FrameView.cpp:1465
> > +        SVGSVGElement* svg = static_cast<SVGDocument*>(m_frame->document())->rootElement();
> > +        if (svg) {
> 
> You can write if (SVGSVGElement* svg = ...) { here to save another line :-)

True, fixed.

> > Source/WebCore/svg/SVGSVGElement.cpp:628
> > +void SVGSVGElement::setupInitialView(const String& name, Element* anchorNode)
> 
> Sorry for not spotting it ealier but "name" is not very descriptive. "viewIdentifier" ? "viewTarget" ? I'm sure you can come up with a better name.

I picked fragmentIdentifier now.

> > Source/WebCore/svg/SVGSVGElement.cpp:651
> > +        }
> 
> Add return; here.

Fixed.

> > Source/WebCore/svg/SVGSVGElement.cpp:653
> > +    } else if (hadUseCurrentView) {
> > +        currentView()->setTransform(emptyString());
> 
> You can save this branch by using if (!hadUseCurrentView) early exit.

True, fixed.

> > Source/WebCore/svg/SVGSVGElement.cpp:655
> > +        if (RenderObject* object = renderer())
> > +            RenderSVGResource::markForLayoutAndParentResourceInvalidation(object);
> 
> Why do you only need this invalidation here, and not for the other branches?

It is done for the anchorNode exists block as well.
The use case is, when pressing Back from a page where useCurrentView has been activated, i.e. it is zoomed in using some <view>. Then the Root SVG renderer has to be updated so it can process the new initial viewport transformations. I am not sure why/if it is not needed in the parseViewSpec case.
Cheers,

Rob.

[Rob Buis]

Committed r89745: <http://trac.webkit.org/changeset/89745>

[Tim Horton]

Rob, did you mean to put the changes to SVGAElement.cpp::defaultEventHandler within the bounds of the #if ENABLE(SVG_ANIMATION) block? It seems like we'd want to move the #if block inside the if (url[0] == '#') and #endif before the code you added in this patch.

[Rob Buis]

Hi Tim,

(In reply to comment #20)
> Rob, did you mean to put the changes to SVGAElement.cpp::defaultEventHandler within the bounds of the #if ENABLE(SVG_ANIMATION) block? It seems like we'd want to move the #if block inside the if (url[0] == '#') and #endif before the code you added in this patch.

Good catch, this part should be independent on whether animation is enabled:

 if (!targetElement->hasTagName(SVGNames::viewTag))
                    return;

If you want to do a patch for that I can already rubberstamp it.
Cheers,

Rob.

[Tim Horton]

(In reply to comment #21)
> Hi Tim,
>
> Good catch, this part should be independent on whether animation is enabled:
> 
>  if (!targetElement->hasTagName(SVGNames::viewTag))
>                     return;
> 
> If you want to do a patch for that I can already rubberstamp it.
> Cheers,
> 
> Rob.

Once https://bugs.webkit.org/show_bug.cgi?id=66019 lands, I'll do just that!