Bug 8823 – SVG does not support tabbing navigation between web page links

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: SVG

[jay]

keyboard support for SVG withing webkit Safari is currently broken

Reproducible: Always

Steps to Reproduce:
1.open URI 
2.try to navigate through the homepage using the keyboard 'tab' or other key
3.

Actual Results:  
tab key onkeydown not recognised

Expected Results:  
focus should change to next anchor

It should be possible to cycle through the links on any SVG page as one can with HTML using a keyboard

[Eric Seidel (no email)]

Yeah, this just hasn't been wired up yet.  Definitely a bug.

[jay]

although http://www.peepo.co.uk tabbing works in Mozilla, this is itself a bug :-)

please use http://www.peepo.co.uk/index-opera.svg.

simply the second tabs to anchors, whereas mozilla uses groups.
Erik@Opera kindly pointed suggested anchors was correct.

[Eric Seidel (no email)]

Created attachment 10522 [details]
A partial patch

This patch gets us part of the way there.  It doesn't seem to work quite right yet though.  I have not debugged why.  Even once we make SVGAElement's focusable, we'll have to find a way to draw the css outline property, as that's how WebKit does the link focusing.

[jay]

Created attachment 13594 [details]
reduced testcase

[jay]

olliej & harrison,
apologies for copying you in on this bug, it's SVG & Accessibility.

I understand you're all tied up on P1s.

keyboard accessibility is essential.

[Mark Rowe (bdash)]

I don't think this needs to be a P1.

[jay]

#6

bdash,

evidently you're not blind,
and also in control of a mouse.

http://www.w3.org/TR/SVG-access/#events
http://www.w3.org/TR/WAI-WEBCONTENT-TECHS/#def-device-independent

[Mark Rowe (bdash)]

Actually I lack a mouse.  That's besides the point though.

The normal criteria for determining whether something qualifies as a P1 is if it is a crash, security problem, regression, obvious rendering issue, or a bug that affect major sites.

[jay]

The attachment works in Opera, Camino and Mozilla

[Eric Seidel (no email)]

Created attachment 14889 [details]
Updated (complete) patch

[Eric Seidel (no email)]

Created attachment 14891 [details]
automated layout test (hits ASSERT in WebView)

[Eric Seidel (no email)]

This is the backtrace for the ASSERT which is tripped by this layout test:

#0	0x0026426c in -[WebView becomeFirstResponder] at WebView.mm:2251
#1	0x932cc483 in -[NSWindow makeFirstResponder:]
#2	0x93380fef in -[NSWindow selectKeyViewFollowingView:]
#3	0x002951d8 in WebChromeClient::takeFocus at WebChromeClient.mm:119
#4	0x013efedb in WebCore::Chrome::takeFocus at Chrome.cpp:85
#5	0x0143227f in WebCore::FocusController::advanceFocus at FocusController.cpp:155
#6	0x0143253c in WebCore::FocusController::advanceFocus at FocusController.cpp:112
#7	0x002477d2 in -[WebHTMLView becomeFirstResponder] at WebHTMLView.mm:3142
#8	0x932cc483 in -[NSWindow makeFirstResponder:]
#9	0x9344fcd3 in -[NSClipView becomeFirstResponder]
#10	0x932cc483 in -[NSWindow makeFirstResponder:]
#11	0x9344fc34 in -[NSScrollView becomeFirstResponder]
#12	0x932cc483 in -[NSWindow makeFirstResponder:]
#13	0x0025a8f3 in -[WebFrameView becomeFirstResponder] at WebFrameView.mm:391
#14	0x932cc483 in -[NSWindow makeFirstResponder:]
#15	0x002643a8 in -[WebView becomeFirstResponder] at WebView.mm:2275
#16	0x932cc483 in -[NSWindow makeFirstResponder:]
#17	0x93380fef in -[NSWindow selectKeyViewFollowingView:]
#18	0x002951d8 in WebChromeClient::takeFocus at WebChromeClient.mm:119
#19	0x013efedb in WebCore::Chrome::takeFocus at Chrome.cpp:85
#20	0x0143227f in WebCore::FocusController::advanceFocus at FocusController.cpp:155
#21	0x0143253c in WebCore::FocusController::advanceFocus at FocusController.cpp:112
#22	0x014083e1 in WebCore::EventHandler::defaultTabEventHandler at EventHandler.cpp:1681
#23	0x01409463 in WebCore::EventHandler::defaultKeyboardEventHandler at EventHandler.cpp:1419
#24	0x01239c99 in WebCore::EventTargetNode::defaultEventHandler at EventTargetNode.cpp:583
#25	0x01237fc4 in WebCore::EventTargetNode::dispatchGenericEvent at EventTargetNode.cpp:267
#26	0x01239909 in WebCore::EventTargetNode::dispatchEvent at EventTargetNode.cpp:308
#27	0x0106041d in WebCore::SVGElement::dispatchEvent at SVGElement.cpp:237
#28	0x01409350 in WebCore::EventHandler::defaultKeyboardEventHandler at EventHandler.cpp:1409
#29	0x01239c99 in WebCore::EventTargetNode::defaultEventHandler at EventTargetNode.cpp:583
#30	0x01237fc4 in WebCore::EventTargetNode::dispatchGenericEvent at EventTargetNode.cpp:267
#31	0x01239909 in WebCore::EventTargetNode::dispatchEvent at EventTargetNode.cpp:308
#32	0x0106041d in WebCore::SVGElement::dispatchEvent at SVGElement.cpp:237
#33	0x01238f3a in WebCore::EventTargetNode::dispatchKeyEvent at EventTargetNode.cpp:370
#34	0x014081a4 in WebCore::EventHandler::keyEvent at EventHandler.cpp:1375
#35	0x01405265 in WebCore::EventHandler::keyEvent at EventHandlerMac.mm:138
#36	0x0023f128 in -[WebHTMLView keyDown:] at WebHTMLView.mm:3419
#37	0x00004dad in -[EventSendingController keyDown:withModifiers:] at EventSendingController.m:367
#38	0x90a5ac56 in objc_msgSendv
#39	0x927f53b2 in -[NSInvocation invoke]
#40	0x003e1486 in KJS::Bindings::ObjcInstance::invokeMethod at objc_instance.mm:187
#41	0x003dcfd5 in KJS::RuntimeMethod::callAsFunction at runtime_method.cpp:89
#42	0x0041e24e in KJS::JSObject::call at object.cpp:98
#43	0x0044773d in KJS::FunctionCallDotNode::evaluate at nodes.cpp:790
#44	0x00444e93 in KJS::ExprStatementNode::execute at nodes.cpp:1723
#45	0x00444d9d in KJS::IfNode::execute at nodes.cpp:1742
#46	0x00442176 in KJS::SourceElementsNode::execute at nodes.cpp:2528
#47	0x0041adb4 in KJS::BlockNode::execute at nodes.cpp:1699
#48	0x0043f301 in KJS::Interpreter::evaluate at interpreter.cpp:365
#49	0x0127901f in WebCore::KJSProxy::evaluate at kjs_proxy.cpp:78
#50	0x013df4bb in WebCore::FrameLoader::executeScript at FrameLoader.cpp:712
#51	0x0102cd6f in WebCore::XMLTokenizer::endElementNs at XMLTokenizer.cpp:753
#52	0x0102ce07 in endElementNsHandler at XMLTokenizer.cpp:985
#53	0x91bff515 in xmlParseNotationDecl
#54	0x91be4d86 in xmlParseChunk
#55	0x0102be90 in WebCore::XMLTokenizer::write at XMLTokenizer.cpp:566
#56	0x013d4075 in WebCore::FrameLoader::write at FrameLoader.cpp:927
#57	0x013d41a7 in WebCore::FrameLoader::addData at FrameLoader.cpp:1583
#58	0x01102a01 in -[WebCoreFrameBridge addData:] at WebCoreFrameBridge.mm:288
#59	0x01105b9c in -[WebCoreFrameBridge receivedData:textEncodingName:] at WebCoreFrameBridge.mm:1427
#60	0x00233245 in -[WebHTMLRepresentation receivedData:withDataSource:] at WebHTMLRepresentation.mm:173
#61	0x0022e73b in -[WebDataSource(WebInternal) _receivedData:] at WebDataSource.mm:176
#62	0x002917c5 in WebFrameLoaderClient::committedLoad at WebFrameLoaderClient.mm:658
#63	0x013d0f6b in WebCore::FrameLoader::committedLoad at FrameLoader.cpp:3027
#64	0x013e1acd in WebCore::DocumentLoader::commitLoad at DocumentLoader.cpp:347
#65	0x013e1b26 in WebCore::DocumentLoader::receivedData at DocumentLoader.cpp:359
#66	0x013d0465 in WebCore::FrameLoader::receivedData at FrameLoader.cpp:2039
#67	0x013e33f0 in WebCore::MainResourceLoader::addData at MainResourceLoader.cpp:133
#68	0x013e5599 in WebCore::ResourceLoader::didReceiveData at ResourceLoader.cpp:208
#69	0x013e3725 in WebCore::MainResourceLoader::didReceiveData at MainResourceLoader.cpp:289
#70	0x013e51a0 in WebCore::ResourceLoader::didReceiveData at ResourceLoader.cpp:330
#71	0x013c0100 in -[WebCoreResourceHandleAsDelegate connection:didReceiveData:lengthReceived:] at ResourceHandleMac.mm:351
#72	0x92854afa in -[NSURLConnection(NSURLConnectionInternal) _sendDidReceiveDataCallback]
#73	0x92852ddb in -[NSURLConnection(NSURLConnectionInternal) _sendCallbacks]
#74	0x92852ab5 in _sendCallbacks
#75	0x9082bf92 in CFRunLoopRunSpecific
#76	0x9082bace in CFRunLoopRunInMode
#77	0x92823d3a in -[NSRunLoop runMode:beforeDate:]
#78	0x0000b09d in runTest at DumpRenderTree.m:1458
#79	0x00006f7e in dumpRenderTree at DumpRenderTree.m:526
#80	0x000071e6 in main at DumpRenderTree.m:572

[Eric Seidel (no email)]

I'm betting that the implementation of
static Node* deepFocusableNode(FocusDirection direction, Node* node, KeyboardEvent* event)

in FocusController is our problem.  It seems to be looking for a HTMLFrameOwnerElement, which I doubt an SVG document has.

[Eric Seidel (no email)]

If I make it send option-\t instead, it doesn't hit the ASSERT (but it also fails the test).  option-\t is required in my Safari browser, I guess because I don't have some preference turned on to allow link-tabbing.  I'm not sure which preference is on in DRT.

I believe it's hitting the ASSERT because it's not able to find any node which responds yes to isKeyboardFocusable (which makes sense, because they all respond "NO" when keyboard tabbing it turned off).  It doesn't seem that the focus code is prepared for this eventuality however.

[Eric Seidel (no email)]

*** Bug 12631 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Ha!  Turns out that this is not an SVG bug after all.  This happens with xhtml documents too!  I've filed bug 14027 to track the ASSERT/crash.

[Eric Seidel (no email)]

To clarify:  The patch still stands for review.  It can be landed.  We just can't land the layout test until bug 14027 is fixed.

[Oliver Hunt]

Comment on attachment 14889 [details]
Updated (complete) patch

r=me

[Eric Seidel (no email)]

Landed on feature branch as r22066.

[jay]

I don't seem to have received an email regarding this having been "fixed"
however, using Safari 3.0.3 webkit r25571 seems totally broken.

[Oliver Hunt]

This has been fixed in the feature branch (r22066)

[jay]

#21  how about a link? 
when will the 'feature' branch enter the trunk?

the comments so far fall short of being helpful to the bug reporter

cheers

[Oliver Hunt]

The bug is fixed, if you checkout the feature branch you can verify that it is fixed.  There is no date we can provide for when the feature branch will be merged into trunk, nor when a subsequent release will occur.

All that matters is that the bug is fixed, and will eventually end up in a release.

[jay]

#23 

Oliver wrote "checkout the feature branch"

however this isn't a link, let alone a method.
a search with google provided no further information.

is there a build, or is this cvs only?
where is it?

as the bug reporter, it's hard for me to agree this is 'fixed' without further information.

also iirc it is quite usual for developers to consider a bug fixed, when in fact the reporter is not of the same opinion....

[Mark Rowe (bdash)]

Jay, you can check out and build the feature branch in a similar manner to the normal (eg, trunk) build process.  Follow the instructions starting from <http://webkit.org/building/checkout.html>, but replace the step that says:

svn checkout http://svn.webkit.org/repository/webkit/trunk WebKit

with:

svn checkout http://svn.webkit.org/repository/webkit/branches/feature-branch WebKit

Then if you follow the build steps through to completion, you will have a built copy of WebKit from the feature branch which contains the fix for this issue.

[jay]

#25

cheers Mark, well all went okay till build, ran for a short while and then:

....
CompileC /Users/Jay/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/ppc/WebCoreAXObject.o /Users/Jay/WebKit/WebCore/bridge/mac/WebCoreAXObject.mm normal ppc objective-c++ com.apple.compilers.gcc.4_0
    cd /Users/Jay/WebKit/WebCore
    /usr/bin/gcc-4.0 -x objective-c++ -arch ppc -pipe -Wno-trigraphs -fno-exceptions -fno-rtti -fobjc-exceptions -fpascal-strings -fasm-blocks -O2 -Werror -Wnon-virtual-dtor -Wnewline-eof -DNDEBUG -DENABLE_SVG -DENABLE_SVG_EXPERIMENTAL_FEATURES -DENABLE_XPATH -DENABLE_XSLT -DENABLE_VIDEO -fmessage-length=0 -fobjc-direct-dispatch -fobjc-gc -mtune=G5 -fvisibility-inlines-hidden -fno-threadsafe-statics -mmacosx-version-min=10.4 -gdwarf-2 -I/Users/Jay/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/WebCore.hmap -Wall -W -Wcast-align -Wchar-subscripts -Wformat-security -Wmissing-format-attribute -Wpointer-arith -Wwrite-strings -Wno-format-y2k -Wno-unused-parameter -Wno-long-double -Wundef -Wshorten-64-to-32 -F/Users/Jay/WebKit/WebKitBuild/Release -I/Users/Jay/WebKit/WebKitBuild/Release/include -IForwardingHeaders -Iicu -I/usr/include/libxslt -I/usr/include/libxml2 -I/Users/Jay/WebKit/WebKitBuild/Release/DerivedSources/WebCore -I/Users/Jay/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/DerivedSources -include /Library/Caches/com.apple.Xcode.501/SharedPrecompiledHeaders/WebCorePrefix-emjzgmqqjsmxpbcgzxwibfbkfkom/WebCorePrefix.h -c /Users/Jay/WebKit/WebCore/bridge/mac/WebCoreAXObject.mm -o /Users/Jay/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/ppc/WebCoreAXObject.o
/Users/Jay/WebKit/WebCore/bridge/mac/WebCoreAXObject.mm: In function 'BOOL -[WebCoreAXObject accessibilityIsIgnored](WebCoreAXObject*, objc_selector*)':
/Users/Jay/WebKit/WebCore/bridge/mac/WebCoreAXObject.mm:820: internal compiler error: Bus error
Please submit a full bug report,
with preprocessed source if appropriate.
See <URL:http://developer.apple.com/bugreporter> for instructions.
{standard input}:11105:FATAL:.abort  detected.  Assembly stopping.
** BUILD FAILED **
jay:~ Jay$ 

[jay]

I had thought the 'feature' branch was discontinued.

what is the delay? any clues please?

regards

[Eric Seidel (no email)]

It would appear this regressed.

[Eric Seidel (no email)]

Comment on attachment 14889 [details]
Updated (complete) patch

Clearing review bit so it's out of the review queue.

[jay]

#29 eric could you clarify, please.

does this comment suggest that your patch doesn't need reviewing?
or perhaps that this bug doesn't need reviewing for the next 'landing'?

or something else?

cheers

[Eric Seidel (no email)]

(In reply to comment #30)
> #29 eric could you clarify, please.

The code is already in the tree.  We never landed (aka committed) the layout test because of bug 14027.  Our behavior regressed (which could have been avoided had we landed the layout test as well).

Eventually someone will get around to fixing this again.

[Eric Seidel (no email)]

turns out our navigation is just fine.  It's just that our repaint rects do not include the outline (thus it's never painted).

[jay]

eric,

what's the status on this bug?