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

Status: RESOLVED WONTFIX

(Firefox :: Disability Access, defect)

Description

[Willie Walker]

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.

Comment 1

[Aaron Leventhal]

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.

Comment 2

[Aaron Leventhal]

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.

Comment 3

[Aaron Leventhal]

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.

Comment 4

[Ginn Chen]

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.

Comment 5

[Willie Walker]

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

Comment 6

[Ginn Chen]

ATK doesn't have a ROLE for image link.

From at-poke, hyperlinked image has an action named "Jump"

Comment 7

[Ginn Chen]

Attachment: patch (fire focus event for image link with accessible name)

Comment 8

[Ginn Chen]

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.

Comment 9

[Ginn Chen]

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 10

[Aaron Leventhal]

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?

Comment 11

[Aaron Leventhal]

(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.

Comment 12

[Ginn Chen]

Attachment: patch v2 (don't check accessible name)

Comment 13

[Ginn Chen]

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?

Comment 14

[Ginn Chen]

(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 15

[Aaron Leventhal]

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

Yes, HTML area will be fixed in new-atk

Comment 16

[Aaron Leventhal]

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.

Comment 17

[Ginn Chen]

Attachment: fix for HTMLArea

Comment 18

[Ginn Chen]

Attachment: fix for HTMLArea

re-post to get rid of damn tab

Comment 19

[Aaron Leventhal]

Bug in MSAA as well unless this part of doc has been walked already.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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...

Comment 21

[Aaron Leventhal]

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 22

[Aaron Leventhal]

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 :)

Comment 23

[clc4tts]

Attachment: Demonstrates the problem

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.

Comment 24

[clc4tts]

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. 

Comment 25

[Ginn Chen]

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.

Comment 26

[Ginn Chen]

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.

Comment 27

[Willie Walker]

Attachment: Attempts to gather any information from image object with focus

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)

Comment 28

[Ginn Chen]

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

Comment 29

[Ginn Chen]

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. :(

Comment 30

[Willie Walker]

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 31

[Darin Fisher]

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

Comment 32

[Ginn Chen]

(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

Comment 33

[Ginn Chen]

fix for image link landed on trunk and MOZILLA_1_8_BRANCH

Comment 34

[Aaron Leventhal]

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

Comment 35

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381