Bug 17450 – activating a focused link to an in-page fragment ID should transfer focus to the target of the link when possible

• Component: Accessibility
• Status: RESOLVED FIXED

[goetsu]

in page anchor (like skip nav, or go to content link) can by activated by keyboard  and the visual focus is going at the target anchor but the keyboard focus will stay at the same place instead of going at the target anchor.

You can see an example here http://www.handicap.gouv.fr the first three links become visible when you use keyboard navigation with the tab key. if you active it with enter the focus will stay on the link. With firefox, Opera or IE the focus go on the target anchor.

It's really painful for people using keyboard navigation as there day to day navigation mechanism or for people using assistive technology

[Mark Rowe (bdash)]

<rdar://problem/6404984>

[Anthony Ricaud]

Confirmed with r38826.

[goetsu]

the webiste given as an example isn't online anymore but the bug is still on safari 5, here is another website where the bug can be reproducted : http://news.bbc.co.uk/

[Alexey Proskuryakov]

I know that this has been migrated and confirmed, so some people seem to understand this bug report, but I don't.

Could you please explain how to reproduce this step by step? Something like:
1. Open http://news.bbc.co.uk/
2. Press Tab three times to select (a particular element)
3. Press Enter

Expected results:
Actual results:

If this requires non-default settings (like maybe "Tab selects all links" preference from Safari advanced preference pane), please mention those, too.

[goetsu]

1. open  http://www.rnib.org.uk/Pages/Home.aspx
2. Press Tab one time to select "Skip to main content"
3. Press Enter
4. Press Tab

expected result : the focus must go to "Talk and Support appeal" (aka the first link after the h1 with id="skiptomain" in the content)

current result : the focus go to "A" (aka the next link after "Skip to main content" in the content order)

alternative way to see the bug :
1. open  http://www.rnib.org.uk/Pages/Home.aspx#skiptomain
2. Press Tab one time

expected result : the focus must go to "Talk and Support appeal" (aka the first link after the h1 with id="skiptomain" in the content)

current result : the focus go to "Skip to main content" (aka the first link in the content order)

[Mario Sanchez Prada]

This bugs looks very similar to bug 59737, where I already proposed a patch, although not cross platform, but a GTK specific one.

However, if people agrees this is a cross-platform issue (and it looks like it) AND that the approach I followed in the patch is the right one, I wouldn't mind trying to cook a cross-platform version of the patch in there :-)

I can confirm that this is still a valid bug and needs addressing asap please.

[Martin Gartner]

Bug still exists in latest WebKit!
This is such a big fail in acessibillity - please fix it.

[Xandor Schiefer]

Bug still exists. Can reproduce by visiting http://drupal.org/, pressing tab to show skip to content link. Press enter and you're taken to content. Press tab again and you're back at the top.

Seriously guys, this is 4 (yes FOUR) years old now. Lame. We'd like the web to move forward, but before we focus on 3D transforms we need to get this stuff right.

[Mario Sanchez Prada]

Looking at the code, it seems to me this is a problem that must be addressed in every port separately, and that it has been already addressed in the chromium and the windows port (I tried with Google Chrome 16 and seems to work indeed).

However, other a11y-enabled ports such as the Mac and the GTK ports have the infrastructure in place to fix it but haven't fixed it yet :-(

I have no idea about how to write Objective-C so I wouldn't know how to write a patch for the Mac, so I'm CCing Chris Fleizach here, who will know better for sure. As for the GTK port, I already had written a patch some time ago that fixed the problem and even got accepted, but then it broke some tests and got rolled out... and got unfortunately forgotten due to happening in a moment where I was too busy with other matters. Shame on me.

However, I'd like to make up the mistake and will try to find some time soon (hopefully this week) to re-try my old patch, change what it's needed and see if we can get it fixed in the GTK port once and for all.

[Mario Sanchez Prada]

Making this bug depend on bug 59737, which is about fixing the problem for the GTK port

[chris fleizach]

(In reply to comment #10)
> Looking at the code, it seems to me this is a problem that must be addressed in every port separately, and that it has been already addressed in the chromium and the windows port (I tried with Google Chrome 16 and seems to work indeed).
> 
> However, other a11y-enabled ports such as the Mac and the GTK ports have the infrastructure in place to fix it but haven't fixed it yet :-(
> 
> I have no idea about how to write Objective-C so I wouldn't know how to write a patch for the Mac, so I'm CCing Chris Fleizach here, who will know better for sure. As for the GTK port, I already had written a patch some time ago that fixed the problem and even got accepted, but then it broke some tests and got rolled out... and got unfortunately forgotten due to happening in a moment where I was too busy with other matters. Shame on me.
> 
> However, I'd like to make up the mistake and will try to find some time soon (hopefully this week) to re-try my old patch, change what it's needed and see if we can get it fixed in the GTK port once and for all.

Why does this need to be a platform decision? It seems like all we'd want to do is, if a in page link is activated and focus was on that link, move setFocusedNode() to the anchor if it accepts keyboard focus...

[Mario Sanchez Prada]

(In reply to comment #12)
> [...]
> Why does this need to be a platform decision? It seems like all we'd want to do is, if a in page link is activated 
> and focus was on that link, move setFocusedNode() to the anchor if it accepts keyboard focus...

Makes sense. I just wrote that, "looking at the code, it seemed to me this is a problem that must be addressed in every port separately", but perhaps I'm wrong.

The root of my guessing is that I've quickly skimmed through the code and saw there were platform specific implementations for chromium and windows, and that made me think there could be some reason for that.

But maybe (hopefully!) you're right and this can be addressed in one take in cross-platform code.

[chris fleizach]

(In reply to comment #13)
> (In reply to comment #12)
> > [...]
> > Why does this need to be a platform decision? It seems like all we'd want to do is, if a in page link is activated 
> > and focus was on that link, move setFocusedNode() to the anchor if it accepts keyboard focus...
> 
> Makes sense. I just wrote that, "looking at the code, it seemed to me this is a problem that must be addressed in every port separately", but perhaps I'm wrong.
> 
> The root of my guessing is that I've quickly skimmed through the code and saw there were platform specific implementations for chromium and windows, and that made me think there could be some reason for that.
> 
> But maybe (hopefully!) you're right and this can be addressed in one take in cross-platform code.

Mario, are you actively working on this one. I too agree that it's a really bad experience and we should fix it soon

[Mario Sanchez Prada]

(In reply to comment #14)
> [...]
> Mario, are you actively working on this one. I too agree that it's a really bad experience and we should fix it soon

I will give it a shot during today, trying first to find a cross platform solution, as you suggested. No promises, though.

[Mario Sanchez Prada]

Created attachment 129465 [details]
Patch proposal + Layout Test

Attaching a patch that addresses the issue in a cross-platform way: I moved AXObjectCache::handleScrolledToAnchor() from AXObjectMac.mm and AXObjectGTK.cpp out to AXObjectCache.cpp and implemented there the fix.

However, as win and chromium ports already provide their own implementation of that function (which are different among them) I dare not to remove that code and so I put the new function in AXObjectCache.cpp inside a #if PLATFORM(GTK) || PLATFORM(MAC) -- #endif section. In the future if the people from those ports want to use the cross-platform solution, it would be an easy change.

Last, I wrote a layout test that I think should work fine both in GTK and Mac but, as usual, no promises yet :-)

[chris fleizach]

Comment on attachment 129465 [details]
Patch proposal + Layout Test

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

> LayoutTests/accessibility/anchor-link-selection-and-focus.html:35
> +        shouldBe("getSelection().anchorNode.nodeValue", "'The Anchor'");

you should probably also check that the anchor is focused now

> Source/WebCore/accessibility/AXObjectCache.cpp:163
> +{

is this only going to happen if accessibility is enabled?
I'd think we'd want this to happen all the time for people using the keyboard to navigate as well (without a screen reader per se)

[WebKit Review Bot]

Comment on attachment 129465 [details]
Patch proposal + Layout Test

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

New failing tests:
accessibility/anchor-link-selection-and-focus.html

[Mario Sanchez Prada]

Comment on attachment 129465 [details]
Patch proposal + Layout Test

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

Thanks for the review. See some comments below...

>> LayoutTests/accessibility/anchor-link-selection-and-focus.html:35
>> +        shouldBe("getSelection().anchorNode.nodeValue", "'The Anchor'");
> 
> you should probably also check that the anchor is focused now

You're right. I'll add that in a follow-up patch.

>> Source/WebCore/accessibility/AXObjectCache.cpp:163
>> +{
> 
> is this only going to happen if accessibility is enabled?
> I'd think we'd want this to happen all the time for people using the keyboard to navigate as well (without a screen reader per se)

In its current form, "yes". This is because AXObjectCache::handleScrolledToAnchor() is called from FrameView::scrollToAnchor(), in the following fashion:

void FrameView::scrollToAnchor()
{
    [...]

    // Scroll nested layers and frames to reveal the anchor.
    // Align to the top and to the closest side (this matches other browsers).
    anchorNode->renderer()->enclosingLayer()->scrollRectToVisible(rect, ScrollAlignment::alignToEdgeIfNeeded, ScrollAlignment::alignTopAlways);

    if (AXObjectCache::accessibilityEnabled())
        m_frame->document()->axObjectCache()->handleScrolledToAnchor(anchorNode.get());

    [...]
}

I agree with you this issue is something that we want to get fixed across different platforms, but I'm not sure if changing the selection and focus is something that an user with a11y disabled would expect. After all keyboard navigation it's usually related (well, at least in GTK) to caret navigation, which is an accessibility feature, so maybe the expected behaviour when a11y is disabled is just to scroll the window (as it does now) and not do anything else.

Checking the W3C recommendation at http://www.w3.org/TR/html4/struct/links.html, I can't find any clue about whether updating the focus and selection is the expected normal behaviour, so I guess it's safe to fix this when a11y is enabled.

[Mario Sanchez Prada]

Created attachment 129671 [details]
Patch proposal + Layout Test

[James Craig]

Nothing is really clear about focusing anchors in HTML 4, but HTML 5 is promising, if slightly obscure:

Following a hyperlink:
http://www.w3.org/TR/html5/links.html#following-hyperlinks
"When a user follows a hyperlink created by an element, the user agent must resolve the URL given by the href attribute of that element, relative to that element, and if that is successful, must navigate a browsing context to the resulting absolute URL."

Navigate:
http://www.w3.org/TR/html5/history.html#navigate
"Fragment identifiers: If the absolute URL of the new resource is the same as the address of the active document of the browsing contextbeing navigated, ignoring any <fragment> components of those URLs, and the new resource is to be fetched using HTTP GET or equivalent, and the absolute URL of the new resource has a <fragment> component (even if it is empty), then navigate to that fragment identifier and abort these steps."

Navigating to a fragment identifier:
http://www.w3.org/TR/html5/history.html#scroll-to-fragid
"When the user agent is required to scroll to the fragment identifier, it must change the scrolling position of the document using the scroll an element into view algorithm defined in the CSSOM View specification, or perform some other action, such that the indicated part of the document is brought to the user's attention."

The phrase "perform some other action such that…[the anchor]…is brought to the user's attention" is vague, but focusing the anchor is clearly the action that should be performed if full keyboard access (FKA) is enabled.

I'll also file a defect against HTML 5 Accessibility to get the spec to explicitly state the UA must move focus to that location if focusable, and perhaps something about moving the browsing context focus there even if it's not "focusable" and therefore can't provide a DOM focus event. For example, if the linked anchor is a non-focusable heading, pressing Tab should move to the next focusable item past the heading, even if the heading itself was not the focused document.activeElement.

This should also cover the case were a user just loads a URL with a fragID (like the ones above) without actually clicking the link.

[James Craig]

HTML 5 Defect:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=16186

[WebKit Review Bot]

Comment on attachment 129671 [details]
Patch proposal + Layout Test

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

New failing tests:
accessibility/anchor-link-selection-and-focus.html

[chris fleizach]

(In reply to comment #23)
> (From update of attachment 129671 [details])
> Attachment 129671 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11779593
> 
> New failing tests:
> accessibility/anchor-link-selection-and-focus.html

Once we resolve the layout test failures i think things look OK. We can make this apply all the time if HTML5 provides clarification to that effect

[chris fleizach]

Comment on attachment 129671 [details]
Patch proposal + Layout Test

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

> Source/WebCore/accessibility/AXObjectCache.cpp:179
> +    Position targetPosition = firstPositionInOrBeforeNode(const_cast<Node*>(node));

you already const_cast'ed this node before into refNode

[Mario Sanchez Prada]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 129671 [details] [details])
> > Attachment 129671 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/11779593
> > 
> > New failing tests:
> > accessibility/anchor-link-selection-and-focus.html
> 
> Once we resolve the layout test failures i think things look OK. We can make this apply all the time if HTML5 > provides clarification to that effect

Problem is that without seeing the actual output in chromium-ews for that test, I can hardly spot what the problem is about. I can try, though, but if there were a way to check that out, it would be awesome. Do you know if there's any?

(In reply to comment #25)
> (From update of attachment 129671 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129671&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:179
> > +    Position targetPosition = firstPositionInOrBeforeNode(const_cast<Node*>(node));
> 
> you already const_cast'ed this node before into refNode

I'm such an idiot... Will change that in a follow up patch

[Mario Sanchez Prada]

Created attachment 129904 [details]
Patch Proposal + Layout Test

I asked in #webkit but there seems not to be a way to check the actual result in chromium EWS after all, so I took a guess and decided to move the mac-specific expectation file down from platform/mac/a11y to to a11y/, so the chromium port will find it. 

Let's see what happens now, although I don't feel very confident, since the error is saying "Unexpected text diff mismatch", instead of "not expected results found".

If this keeps failing I'm not sure what would be the way to go, maybe pinging someone at Google (Dominic?) so they can try it there and report the actual result, or just take the wild way and add it to test_expectations.txt in chromium, although in theory all the required stuff from the AccessibilityController and AccessibilityUIElement (see Chromium bug 10322 [1]) seem to be already implemented in the chromium port, so it should be working.

[1] http://code.google.com/p/chromium/issues/detail?id=10322

[WebKit Review Bot]

Comment on attachment 129904 [details]
Patch Proposal + Layout Test

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

New failing tests:
accessibility/anchor-link-selection-and-focus.html

[Mario Sanchez Prada]

(In reply to comment #28)
> (From update of attachment 129904 [details])
> Attachment 129904 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11796048
> 
> New failing tests:
> accessibility/anchor-link-selection-and-focus.html

Not surprised :-/

Anyway, I just sent a mail to Dominic Mazzoni some minutes ago, who kindly agreed on applying the patch and checking the result during the following days (he's not available for that at the moment), so I believe we will eventually get somewhere with this patch.

Have a nice weekend!

[chris fleizach]

(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 129904 [details] [details])
> > Attachment 129904 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/11796048
> > 
> > New failing tests:
> > accessibility/anchor-link-selection-and-focus.html
> 
> Not surprised :-/
> 
> Anyway, I just sent a mail to Dominic Mazzoni some minutes ago, who kindly agreed on applying the patch and checking the result during the following days (he's not available for that at the moment), so I believe we will eventually get somewhere with this patch.
> 
> Have a nice weekend!

I think you should add this to the skip list for Chromium. 

We're only changing code for GTK and mac, so we can't know what the behavior will be anywhere else. There may in fact be subtle differences on the chromium implementation of this.

[Mario Sanchez Prada]

(In reply to comment #30)
> [...]
> I think you should add this to the skip list for Chromium. 
> 
> We're only changing code for GTK and mac, so we can't know what the behavior will be anywhere else. 
> There may in fact be subtle differences on the chromium implementation of this.

I agree with this, but I think we can anyway wait for a couple of days before going ahead, in case Dominic has time to try it out. It could be that it would add some valuable extra information here.

So let's wait until, say, Thursday or so, I'd say.

[Mario Sanchez Prada]

Created attachment 131012 [details]
Patch

Attaching new patch skipping the test for chromium

[Mario Sanchez Prada]

Ping reviewers?

[chris fleizach]

Comment on attachment 131012 [details]
Patch

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

r=me with fixes suggested

> Source/WebCore/accessibility/AXObjectCache.cpp:164
> +    Document* document = node->document();

you probably want to ASSERT(node)

> Source/WebCore/accessibility/AXObjectCache.cpp:173
> +    if (!selection)

it seems that even if selection == 0, we can still set the focusedNode on the document

[Mario Sanchez Prada]

Committed r112851: <http://trac.webkit.org/changeset/112851>

[Mario Sanchez Prada]

Looks like the patch broke some things, so it got rolled out (see bug 82915).

Hence reopening...

[Mario Sanchez Prada]

Comment on attachment 131012 [details]
Patch

Clearing flags

[Maciej Stachowiak]

(In reply to comment #21)

> 
> The phrase "perform some other action such that…[the anchor]…is brought to the user's attention" is vague, but focusing the anchor is clearly the action that should be performed if full keyboard access (FKA) is enabled.
> 

If the HTML5 spec meant to focus something, it would clearly say so. What do other browsers do in this case?

[Ian 'Hixie' Hickson]

Focusing a focusable target when you follow a fragment link would be a reasonable thing to do. The spec doesn't require it, though, as most browsers don't do it, at least not by default.

[Leif Halvard Silli]

(In reply to comment #39)
> Focusing a focusable target when you follow a fragment link would be a reasonable thing to do. The spec doesn't require it, though, as most browsers don't do it, at least not by default.

When comment #0 said that "With firefox, Opera or IE the focus go on the target anchor", then it does not sound correct to say that "most browsers don't do it".  I agree that, of the common, graphical user agents, it might only be Firefox that, in a systematic way, moves the foucs to the targeted element. But there seems to be subtle and important differences that makes it work better in e.g. IE than in Webkit browsers.

The page http://html5accessibility.com tracks HTML5 accessibiltiy for OSX and Windows browsers. It oftens mentions keyboardaccess as a shortcoming - and it gives the highest score to Firefox on Windows - 14% better score than the second best - Safari on OSX.  (And tha page does not test skip to-links.) A better correlation between what visually gets focused and what gets keyboard focus, should probably make it simpler for Webkit browsers to score higher on that tracker page.

Speaking about HTML5, it ought to be possible for it to be more concrete about what users should expect to happen. For example, regarding the phrase "action such that…[the anchor]…is brought to the user's attention", then the current problem e.g. in Webkit is that, yes, FIRST the anchor is brought to attention - visually - BUT THEN, if the user hits the Tabulator key to e.g. move to next focusable element, the keyboard focus just jumps away and off to the very first focusable element of the entire page. Such behavior - "one foot forwards and then many (unexpected) feet backwards" - is not what the phrase "to follow a link" should signify. Ideally, all the focuses - they keyboard focus, the visual focus and the A11Y focus should more or less follow. And it should be possible for HTML5 to indicate the need for this correlation.

[James Craig]

Not seeing any objection to Leif's comments, but the point again that this is fixing a terrible experience for keyboard users, . 

I wrote:
> focusing the anchor is clearly the action that should be performed if full keyboard access (FKA) is enabled.

To clarify: Whether or not the HTML spec is entirely explicit about the focus event, "focusing the anchor is clearly the action that should be performed (for the sake of a usable experience) if full keyboard access is enabled."

If anyone disagrees, please unplug your mouse for a few minutes. I suspect you'll come around.

[Maciej Stachowiak]

(In reply to comment #41)
> Not seeing any objection to Leif's comments, but the point again that this is fixing a terrible experience for keyboard users, . 
> 
> I wrote:
> > focusing the anchor is clearly the action that should be performed if full keyboard access (FKA) is enabled.
> 
> To clarify: Whether or not the HTML spec is entirely explicit about the focus event, "focusing the anchor is clearly the action that should be performed (for the sake of a usable experience) if full keyboard access is enabled."
> 
> If anyone disagrees, please unplug your mouse for a few minutes. I suspect you'll come around.

I don't think anyone is objecting to this change.

I would say the behavior proposed here is correct when links are eligible for tab-focus (which will be true in Safari on Mac when full keyboard access is enabled, or Safari's "Press Tab to highlight each item on a webpage" pref is enabled, and on other ports is true always. This assumes the fragment target is an <a> element. If it's something else, then whether it gets focus should likewise depend on whether it is eligible for tab-focus.

[Ben Sciascia]

Does anyone know if/when this issue might be fixed? 

Is there a cross-platform issue making this difficult to overcome or is it fixed - sorry, just wasn't sure based on all the comments but various best practice articles still say this is an issue and recommend javascript to get around it.

[goetsu]

(In reply to comment #43)
As initial creator of this bug I hope too but I'm afraid it will not be before the reign of unicorn with only one left to rule them all.

[chris fleizach]

Created attachment 186658 [details]
patch

[WebKit Review Bot]

Comment on attachment 186658 [details]
patch

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

New failing tests:
svg/custom/scrolling-embedded-svg-file-image-repaint-problem.html

[Mario Sanchez Prada]

(In reply to comment #45)
> Created an attachment (id=186658) [details]
> patch

I like this new approach better than my initial attempt to fix it through the accessibility layer. About the failing test, I don't really see the relationship with this change. Perhaps it's just flaky?

(In reply to comment #44)
> (In reply to comment #43)
> As initial creator of this bug I hope too but I'm afraid it will not be
> before the reign of unicorn with only one left to rule them all.

I think I owe you an apology here, since I worked initially on this at the beginning of the year and then plainly forgot about finishing it after the rollback due to other external matters that got me busy most of the year.

So sorry about that, and happy that Chris picked it up too.

[Mario Sanchez Prada]

(In reply to comment #45)
> Created an attachment (id=186658) [details]
> patch

It would be great if we could this patch getting an official review since it's a pain for many users and the solution seems to be very close right now.

About the patch itself, as I said before, it looks good to me but I'm not an reviewer yet, so we need someone else checking it.

The only suggestion I think I could make is whether it would make sense to add some bits to the test to check that pressing "Tab" after moving the focus by following a link works properly.

[Maciej Stachowiak]

Comment on attachment 186658 [details]
patch

r=me

[chris fleizach]

http://trac.webkit.org/changeset/148481

[James Craig]

Related to bug 116046.

[Mario Sanchez Prada]

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

[Josh Habdas]

I noticed there may be a regression with this "fixed" bug, as the issue persists. There is already a related bug open but it appears to be a different issue. Should this bug be reopened or should a new issue be filed?

To reproduce please do the following:

1. Navigate to https://chicagoganghistory.com and tab twice to the skip nav link.
2. Then press [Enter] followed by [Tab] to activate the skip nav anchor.

Expected result:
First focusable element in the anchored fragment receives focus.

Actual result:
Drop down menu anchors intending to be skipped receive focus.

Environment:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.125 YaBrowser/17.7.0.1575 Yowser/2.5 Safari/537.36

[James Craig]

Since the most recent commit did not resolve all the issues, refiling as bug 185264