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.
Created attachment 320288 [details] Patch
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
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
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
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
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
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
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
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
Created attachment 320336 [details] Patch
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.
Created attachment 320439 [details] Patch
<rdar://problem/34371110>
Created attachment 321113 [details] Patch
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.
Created attachment 327122 [details] Patch
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().
(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?
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.
Created attachment 327134 [details] Patch
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.
Created attachment 327148 [details] Patch
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
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
Created attachment 327172 [details] Patch
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
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
Comment on attachment 327172 [details] Patch Clearing flags on attachment: 327172 Committed r224973: <https://trac.webkit.org/changeset/224973>
All reviewed patches have been landed. Closing bug.