Bug 176577 – SVG scrolling anchor should be reset if the fragmentIdentifier does not exist or is not provided

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Said Abou-Hallawa]

If an SVG image is referenced multiple times and the URL of one of the referencing elements does not provide a fragmentIdentifier or provides a fragmentIdentifier but this fragmentIdentifier doesn't exist in the SVG, the scrolling anchor of the SVGImage should be reset. This should behave as if the image is displayed only once in the page and the URL of the SVGImage does not have a fragmentIdentifier.

[Said Abou-Hallawa]

Created attachment 320288 [details]
Patch

[Build Bot]

Comment on attachment 320288 [details]
Patch

Attachment 320288 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4489937

New failing tests:
fast/html/empty-fragment-id-goto-top.html

[Build Bot]

Created attachment 320293 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320288 [details]
Patch

Attachment 320288 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4489941

New failing tests:
fast/html/empty-fragment-id-goto-top.html

[Build Bot]

Created attachment 320296 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320288 [details]
Patch

Attachment 320288 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4489947

New failing tests:
fast/html/empty-fragment-id-goto-top.html

[Build Bot]

Created attachment 320298 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320288 [details]
Patch

Attachment 320288 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4489963

New failing tests:
fast/html/empty-fragment-id-goto-top.html

[Build Bot]

Created attachment 320300 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Said Abou-Hallawa]

Created attachment 320336 [details]
Patch

[Darin Adler]

Comment on attachment 320288 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2401
> -    if (!url.hasFragmentIdentifier()) {
> -        frame().document()->setCSSTarget(nullptr);
> +    if (fragmentIdentifier.isEmpty()) {
> +        resetScrollAnchor();
>          return false;
>      }

These aren’t the same thing.

- URL::hasFragmentIdentifier returns true for URLs that end in "#", and the fragment identifier is the empty string.
- The new code treats that the same as URLs that have no "#" in them at all.

I would expect this would change behavior.

To express this distinction we need a value that means "lack of a fragment identifier". One approach would be to use the null string for lack of a fragment identifier, and the empty string for URLs that end in "#", an empty fragment identifier. Another programmer might choose to use std::optional<String> in a case like this.

[Said Abou-Hallawa]

Created attachment 320439 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/34371110>

[Said Abou-Hallawa]

Created attachment 321113 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 321113 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2517
> +    document.updateStyleIfNeeded();

This is a new call. Is it necessary?

> Source/WebCore/svg/SVGSVGElement.cpp:495
> +        if (svgRootParent.m_scrollRootElement == this)
> +            svgRootParent.m_scrollRootElement = nullptr;

Seems a bit weird to access m_scrollRootElement directly. Can this be hidden inside a member function?

> Source/WebCore/svg/SVGSVGElement.cpp:663
> +            m_scrollRootElement = &downcast<SVGSVGElement>(*viewportElement);

Pretty sure that you can downcast pointers too: m_scrollRootElement = downcast<SVGSVGElement>(viewportElement);

> Source/WebCore/svg/SVGSVGElement.h:164
> +    SVGSVGElement* m_scrollRootElement { nullptr };

This goes against our desire to not have raw pointers. Please talk to rniwa about the best way to do something like this.

[Said Abou-Hallawa]

Created attachment 327122 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 321113 [details]
Patch

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

>> Source/WebCore/page/FrameView.cpp:2517
>> +    document.updateStyleIfNeeded();
> 
> This is a new call. Is it necessary?

Yes it is necessary to force layout after reseting the SVGViewElement anchor attributes.

>> Source/WebCore/svg/SVGSVGElement.cpp:495
>> +            svgRootParent.m_scrollRootElement = nullptr;
> 
> Seems a bit weird to access m_scrollRootElement directly. Can this be hidden inside a member function?

I changed that code. Instead of storing the nearest root to the SVGViewElement anchor, I am storing the fragmentIdentifier.

>> Source/WebCore/svg/SVGSVGElement.cpp:663
>> +            m_scrollRootElement = &downcast<SVGSVGElement>(*viewportElement);
> 
> Pretty sure that you can downcast pointers too: m_scrollRootElement = downcast<SVGSVGElement>(viewportElement);

Done but in the new functions SVGSVGElement::findViewAnchor() and SVGSVGElement::findRootAnchor().

>> Source/WebCore/svg/SVGSVGElement.h:164
>> +    SVGSVGElement* m_scrollRootElement { nullptr };
> 
> This goes against our desire to not have raw pointers. Please talk to rniwa about the best way to do something like this.

Instead of storing the nearest root to the SVGViewElement anchor, I am storing the fragmentIdentifier. To get the nearest SVGSVGElement to the SVGViewElement anchor, I added the new functions SVGSVGElement::findViewAnchor() and SVGSVGElement::findRootAnchor().

[Simon Fraser (smfr)]

(In reply to Said Abou-Hallawa from comment #17)
> Comment on attachment 321113 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321113&action=review
> 
> >> Source/WebCore/page/FrameView.cpp:2517
> >> +    document.updateStyleIfNeeded();
> > 
> > This is a new call. Is it necessary?
> 
> Yes it is necessary to force layout after reseting the SVGViewElement anchor
> attributes.

But won't document.setCSSTarget() trigger a later style update? Why does it have to synchronous here?

[Said Abou-Hallawa]

Comment on attachment 321113 [details]
Patch

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

>>>> Source/WebCore/page/FrameView.cpp:2517
>>>> +    document.updateStyleIfNeeded();
>>> 
>>> This is a new call. Is it necessary?
>> 
>> Yes it is necessary to force layout after reseting the SVGViewElement anchor attributes.
> 
> But won't document.setCSSTarget() trigger a later style update? Why does it have to synchronous here?

Because FrameView::resetScrollAnchor() is called FrameView::scrollToFragment() which can be called from SVGImage::drawForContainer() so it has to be synchronous in this case. But since this is only needed for SVG, I moved this call in the if (is<SVGDocument>(document)) statement.

[Said Abou-Hallawa]

Created attachment 327134 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 327134 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2211
> +            document.updateStyleIfNeeded();

Please add a comment saying why this eager style update is necessary. Ideally, you'd put it before the thing that needs it, not after something that may change style.

[Said Abou-Hallawa]

Created attachment 327148 [details]
Patch

[EWS Watchlist]

Comment on attachment 327148 [details]
Patch

Attachment 327148 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5269133

New failing tests:
http/tests/appcache/resource-redirect-2.html

[EWS Watchlist]

Created attachment 327149 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 327172 [details]
Patch

[EWS Watchlist]

Comment on attachment 327172 [details]
Patch

Attachment 327172 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5275264

New failing tests:
http/tests/appcache/resource-redirect-2.html

[EWS Watchlist]

Created attachment 327182 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[WebKit Commit Bot]

Comment on attachment 327172 [details]
Patch

Clearing flags on attachment: 327172

Committed r224973: <https://trac.webkit.org/changeset/224973>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.