Closed Bug 334650 Opened 15 years ago Closed 15 years ago

When tabbing between links, link-selected events do not seem to be issued reliably

Categories

(Firefox :: Disability Access, defect)

x86
All
defect
Not set
major

Tracking

()

RESOLVED WONTFIX

People

(Reporter: wwalker, Assigned: ginnchen+exoracle)

References

()

Details

(Keywords: access)

Attachments

(4 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20060321 Firefox/2.0a1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20060321 Firefox/2.0a1

No link-selected events are emitted when tabbing across some links in web pages.  These links seem to be of the ilk that have an image associated with them.


Reproducible: Always

Steps to Reproduce:
1. In an xterm winfow, run the standalone test case from http://bugzilla.mozilla.org/show_bug.cgi?id=320387 (http://bugzilla.mozilla.org/attachment.cgi?id=205958).  

2. Go to http://www.humanware.com.  

3. Tab across the "about us" "products" "solutions" ... links.  

Alternatively,

2. Do a search on Google.

3. Tab across the "1" "2" "3" "4" "5" links at the bottom of the search results

Actual Results:  
You will not see any link-selected events in the xterm window.

Expected Results:  
Link-selected events should be issued when a link is selected.
Keywords: access, sec508
Blocks: fox2access
Assignee: nobody → aaronleventhal
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
This bug is caused because in ATK a new nsHTMLLinkAccessibleWrap will only be created from nsAccessibleHyperText::GetLink(). It can't be created from nsAccessibilityService::GetAccessible() which is how accessibles need to be created on demand for events. Often this is not a problem because those link accessibles are already cached, and GetAccessible() is happy to return the most recently cached accessible for the link.

I don't think more cycles should be spent on this bug for Firefox 2, because this part of the architecture is going away when we go to new-atk. At that point GetAccessible() will know how to create accessibles for every object in the tree  that can generate accessibility events.
Blocks: newatk
No longer blocks: fox2access
Actually in this case GetAccessible() would need to return an nsAccessibleHyperText in order for the event to be processed. Then nsRootAccessible::HandleEvent() uses that to GetSelectedLinkIndex() so that it can fire the event.
Ginn is going to evaluate whether this is reasonable to implement for Gecko 1.8 and Firefox 2, which is supposed to be a stable branch. Changes should not be risky.

If we do take a fix for Firefox 2 it also means we'll be fixing this bug twice, since the newatk changes will be fixing this anyway.
My preliminary investigation:
Firefox doesn't fire link-selected events for image links.

We had a not perfect patch for Sun Mozilla 1.7.
With this patch, it fires focus events for image links, the role of AtkObject is image.
Then Gnopernicus reads the alt text of the image, so it somehow works.

Will, what do you think?
I can polish the patch, if you think it makes sense.
Hi Ginn:

Ideally, things would work and be consistent (which is what we all want for FF3), but I understand the challenges for FF2.  If you have this patch in a build I can test, I can always try it out and see how far we can get.  I suspect it will probably do just fine, but I really wish I were able to tell if the object was a link or not since that information is important to the user.

Will
ATK doesn't have a ROLE for image link.

From at-poke, hyperlinked image has an action named "Jump"
Assignee: aaronleventhal → ginn.chen
Attachment #219993 - Flags: review?(aaronleventhal)
Comment on attachment 219993 [details] [diff] [review]
patch (fire focus event for image link with accessible name)

I'm hesitating whether we should fire event for image without accessible name. If we don't, when shifting focus back, focus event isn't fired either. Because atk thinks it's still the last focused element.

Also, the banner in humanware.com is HTMLAreaElement. We didn't implement it well for ATK yet.
Interesting, it I use at-poke to expose HTMLArea once, it will work fine.

The reason we didn't want to fire event for image without access name was we were trying to adopt gnopernicus/gok.
Comment on attachment 219993 [details] [diff] [review]
patch (fire focus event for image link with accessible name)

Ginn, I don't like this at all. What if the image has sibling text that is also within the same link? The image might not be the first child, but could be a descendent of the link. What happens in those cases?

Will this be going away when we implement newatk?
(In reply to comment #8)
> (From update of attachment 219993 [details] [diff] [review] [edit])
> I'm hesitating whether we should fire event for image without accessible name.
> If we don't, when shifting focus back, focus event isn't fired either. Because
> atk thinks it's still the last focused element.

This is a good reason to fix
https://bugzilla.mozilla.org/show_bug.cgi?id=277469

Then all images will have some accessible name.

Attachment #219993 - Flags: review?(aaronleventhal) → review-
Attachment #219993 - Attachment is obsolete: true
Attachment #220118 - Flags: review?(aaronleventhal)
Image map/area is more complicated than image link.

Getting accessible object for HTMLArea failed, if it is not traversed before.

From nsAccessibilityService.cpp
1830       if (frame->GetContent() != content) {
1831         // Not the main content for this frame!
1832         // For example, this happens because <area> elements return the
1833         // image frame as their primary frame. The main content for the 
1834         // image frame is the image content.
1835         return NS_ERROR_FAILURE;

I'm not familiar with this.
I suggest we treat it as another bug, since image link is more common.

Aaron, do you think the <area> element problem will be solved by the new-atk?
(In reply to comment #13)

> Getting accessible object for HTMLArea failed, if it is not traversed before.

In this case, we can create HTMLImageAccessible first, call GetChildCount to get its children HTMLAreaAccessible cached.

I'd proved it can work.

Comment on attachment 220118 [details] [diff] [review]
patch v2 (don't check accessible name)

Yes, HTML area will be fixed in new-atk
Attachment #220118 - Flags: review?(aaronleventhal) → review+
Comment on attachment 220118 [details] [diff] [review]
patch v2 (don't check accessible name)

Johnny, this is a temporary fix until we reachitect the way that ATK deals with links, which is scheduled to be done for Firefox 3.
Attachment #220118 - Flags: superreview?(jst)
Attachment #220118 - Flags: approval-branch-1.8.1+
Attached patch fix for HTMLArea (obsolete) — Splinter Review
Attachment #220230 - Flags: review?(aaronleventhal)
Attached patch fix for HTMLAreaSplinter Review
re-post to get rid of damn tab
Attachment #220230 - Attachment is obsolete: true
Attachment #220237 - Flags: review?(aaronleventhal)
Attachment #220230 - Flags: review?(aaronleventhal)
Bug in MSAA as well unless this part of doc has been walked already.
OS: Linux → All
Attachment #220237 - Flags: superreview?(bzbarsky)
Attachment #220237 - Flags: review?(aaronleventhal)
Attachment #220237 - Flags: review+
Comment on attachment 220237 [details] [diff] [review]
fix for HTMLArea

>Index: nsAccessibilityService.cpp
>+        frame->QueryInterface(NS_GET_IID(nsIImageFrame), (void**)&imageFrame);

Please use CallQueryInterface.

Ok otherwise, I guess...
Attachment #220237 - Flags: superreview?(bzbarsky) → superreview+
Attachment #220237 - Flags: approval-branch-1.8.1+
Checked in "fix for HTMLArea" in both branch and trunk -- still waiting for sr= on the other part so we can check it into both places.
Comment on attachment 220118 [details] [diff] [review]
patch v2 (don't check accessible name)

Darin, this is a temporary fix until we restructure how ATK handles embedded objects like links and images. So, we're not looking for an in-depth sr= here :)
Attachment #220118 - Flags: superreview?(jst) → superreview?(darin)
Test Link 1 and Test Link 2 will always fire the event correctly when tabbed to.
Test Link 3 and Test Link 4 will always fail to fire the event when tabbed to.
Ok, I just submitted a very simple test case that demonstrates the failure.

I think the problem has to do with the BR element being used. In the test.html example that I submitted, the first two links always fire the event correctly while the last two links always fail. The only difference between the links is that the first two do not have a BR element as the first thing in the link wherease the failing links do start with a BR. I get the feeling that perhaps this is not firing because of the links starts with a child node which is not text. 
Interesting test case, I appreciate your work.
The two patches just fixed image link and image map.
I'll check what's the problem of BR.
Thanks again.
Evan found the problem of BR is
The first child frame is not a text frame.
We didn't deal with this case in current implementation.

<BR> between 2 text frames in a link also has a problem, it is exposed as 2 links.

I think we should fix BR issues with new-atk implementation in Firefox 3.
Based upon an internal build from Ginn as of Fri, May 12, 2006, this test case shows that apparently no useful information is obtained from the google "1" "2" "3" ... links at the bottom of a Google search results page.  There are two main problems here:

1) focus: events are delivered for the links when you tab or arrow between them, but the only information we seem to be able to get is an image with no name.

2) When arrowing between the "1" "2" "3" links, you'll see that no caret-moved events are reported (see bug https://bugzilla.mozilla.org/show_bug.cgi?id=312093)
*** Bug 277895 has been marked as a duplicate of this bug. ***
Will, thanks for your testing.

I understand why IMG has no name, because google.com doesn't give alt text for it.
(perhaps Bug 277469 can help on this)

I don't know why 'hypertext' of IMG is also empty. :(
In examining the "1" "2" "3" links, however, one can cut/paste the visual text for the link (e.g., "1" "2").  So, something is awry here.  That is, the problem is not necessarily with the image, the problem is that we cannot get to the other text associated with the link.
Comment on attachment 220118 [details] [diff] [review]
patch v2 (don't check accessible name)

>+  if (anchorElement) {
>+    nsCOMPtr<nsIDOMNode> childNode;
>+    if (NS_SUCCEEDED(targetNode->GetFirstChild(getter_AddRefs(childNode)))) {
>+      while (childNode) {
>+        nsCOMPtr<nsIDOMHTMLImageElement> imgNode(do_QueryInterface(childNode));
>+        if (imgNode) {
>+          anchorElement = nsnull; // ignore the link
>+          targetNode = childNode; // only fire event for image
>+          break;
>+        }
>+        nsCOMPtr<nsIDOMNode> tmpNode(childNode);
>+        tmpNode->GetNextSibling(getter_AddRefs(childNode));

Leverage nsCOMPtr's ".swap" method to avoid the extra AddRef call here:

          nsCOMPtr<nsIDOMNode> tmpNode;
          tmpNode.swap(childNode);
          tmpNode->GetNextSibling(getter_AddRefs(childNode));

sr=darin
Attachment #220118 - Flags: superreview?(darin) → superreview+
(In reply to comment #30)
> In examining the "1" "2" "3" links, however, one can cut/paste the visual text
> for the link (e.g., "1" "2").  So, something is awry here.  That is, the
> problem is not necessarily with the image, the problem is that we cannot get to
> the other text associated with the link.
> 
The link is for both the image and the text, when the text is focused, GetFirstChild returns the image, so the ROLE is IMAGE and doesn't implement AccessibleText.
Current Mozilla accessible structure for atk doesn't provide any relationship between the image and the text for this case.
We'll address this issue in the new-atk plan.
http://www.mozilla.org/access/unix/new-atk.html
fix for image link landed on trunk and MOZILLA_1_8_BRANCH
No longer blocks: newatk
Depends on: newatk
We're using focus events for links now instead of link-selected, because links are their own object. See http://www.mozilla.org/access/unix/new-atk
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.