423603 - A elements with no HREF should not have state_linked, regression from bug 421066

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008030607 Minefield/3.0b5pre
Build Identifier: 

In HTML, there are two uses for the <a> tag:
1. The HREF attribute refers to a destination so that when the link is activated, the browser will jump to that destination.
2. The NAME attribute creates a link destination which can be referenced elsewhere.
When there is no HREF attribute, the element is not a link (i.e. it does not jump to another destination). Therefore, it should not be exposed as such to accessibility APIs.

It seems that A elements with no HREF have a blank accessible action. Thus, this can be worked around on the assistive technology side if there is a good reason for this behaviour, but I can't think of any good reason at this point.

Reproducible: Always

Comment 1

[James Teh [:Jamie]]

Attachment: Link destination example.

This example includes A tags with and without HREF attributes, including information about which should be exposed and which should not.

Comment 2

[Marco Zehe (:MarcoZ)]

I just checked with JAWS, and it correctly speaks the second item as not being a link. I also checked with accProbe. The first of the two links has states "focusable" and "linked", the second has only "selectable". Would that be enough for you to distinguish them?

Comment 3

[Marco Zehe (:MarcoZ)]

Additional info: Ie also exposes the named anchor as a link, it also gives it a state of "linked". However, both IE and FF 3 have in common that the accValue of the link that is actually pointing somewhere, contains the URL of the destination, whereas there is no accValue for the named anchor.

I do not know what we'd break if we stopped exposing the named anchors as links.

Comment 4

[Marco Zehe (:MarcoZ)]

I just realized that this broke recently. The 2008-03-16 build is fine in regards to not be showing as a link for the second item, the 2008-03-18 build is broken. This throws off all kinds of stuff like JAWS no longer being able to focus the "Additional comments" edit in Bugzilla. Alexander, could this be a result of the fix for bug 421066? Did you do anything to HTML:a tags in that patch, too, not just xul:labels?

Comment 5

[Marco Zehe (:MarcoZ)]

Yes, this is a regression from bug 421066. Patch coming up, it's a simple fix.

Comment 6

[Marco Zehe (:MarcoZ)]

Attachment: Patch

Return once we have set the state for a named anchor. Otherwise, it'll get other states than just "selectable", which causes JAWS and possibly Window-Eyes to erroneously treat named anchors as links.

Comment 7

[alexander :surkov (:asurkov)]

nope, there may be a real link with name attribute, possibly we need to test nsILink interface.

Comment 8

[Aaron Leventhal]

Use STATE_LINKED.

We're like IE in that we expose ROLE_LINK for any anchor. Sometimes the user may jump there. The <a name="foo"> are end point links.

Comment 9

[Aaron Leventhal]

Because of comment 8 I suggest WONTFIX.

Comment 10

[Aaron Leventhal]

I take it back. I thought the issue was an old decision but Marco points out in comment 5 that it's a regression.

Comment 11

[Aaron Leventhal]

Comment on attachment 310438 [details] [diff] [review]
Patch

Let's get this in ASAP!

Comment 12

[Aaron Leventhal]

Comment on attachment 310438 [details] [diff] [review]
Patch

Wait, Surkov is right. A link can have a name and an href.

Comment 13

[Marco Zehe (:MarcoZ)]

Attachment: Patch v2

This fixes the empty links I got for the named anchors right above the Additional Comments textarea, but it does not fix the testcase attachment from the top of this bug. Aaron, do you know why? The only thing I see is that the named anchor now has an action of "jump", where it used to have no action at all.

Comment 14

[Aaron Leventhal]

Comment on attachment 310473 [details] [diff] [review]
Patch v2

Perfect.

Comment 15

[Aaron Leventhal]

Comment on attachment 310473 [details] [diff] [review]
Patch v2

Ugh. I am obviously not paying attention because of my move. Sorry.

Comment 16

[Aaron Leventhal]

Attachment: Marco's patch plus fix for nsLinkableAccessible::CacheActionContent() so that only true links get mIsLink = PR_TRUE

Comment 17

[Marco Zehe (:MarcoZ)]

Comment on attachment 310486 [details] [diff] [review]
Marco's patch plus fix for nsLinkableAccessible::CacheActionContent() so that only true links get mIsLink = PR_TRUE

This should definitely go into beta 5 since it breaks at least JAWS on a wide scale.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 310486 [details] [diff] [review]
Marco's patch plus fix for nsLinkableAccessible::CacheActionContent() so that only true links get mIsLink = PR_TRUE

a1.9=beltzner

Comment 19

[Reed Loden [:reed]]

hmm, I assume this patch has r=aaronlev ?

Comment 20

[Marco Zehe (:MarcoZ)]

(In reply to comment #19)
> hmm, I assume this patch has r=aaronlev ?

The first part of the patch, attachment 310473 [details] [diff] [review], did have r+=aaronlev, but he took it away for some reason when he created the combined patch.

Comment 21

[Aaron Leventhal]

It has r=aaronlev

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 310486 [details] [diff] [review]
Marco's patch plus fix for nsLinkableAccessible::CacheActionContent() so that only true links get mIsLink = PR_TRUE


>     if (walkUpAcc && Role(walkUpAcc) == nsIAccessibleRole::ROLE_LINK) {
>-      mIsLink = PR_TRUE;
>-      mActionContent = walkUpContent;
>-      return;
>+      nsCOMPtr<nsILink> link = do_QueryInterface(walkUpContent);
>+      if (link) {
>+        nsLinkState linkState;
>+        link->GetLinkState(linkState);
>+        if (linkState != eLinkState_NotLink) {
>+          mIsLink = PR_TRUE;
>+          mActionContent = walkUpContent;
>+          return;
>+        }
>+      }

It's not correct from point of view architecture, not all links implement nsILink interface (it is specific for HTML links and some SVG links). XUL link doesn't implement it and iirc xlink doesn't do as well (though we don't support it yet). I think it's worth to check STATE_LINKED instead.

Comment 23

[Aaron Leventhal]

Attachment: Final patch for checkin -- remove nsILink optimization and instead check State() as Surkov recommends

Okay, Surkov can you check in this version then? We definitely need this in the beta.

Comment 24

[alexander :surkov (:asurkov)]

done.

Checking in accessible/src/base/nsBaseWidgetAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsBaseWidgetAccessible.cpp,v  <--  nsBaseWidgetAccessible.cpp
new revision: 1.70; previous revision: 1.69
done
Checking in accessible/src/html/nsHTMLLinkAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHTMLLinkAccessible.cpp,v  <--  nsHTMLLinkAccessible.cpp
new revision: 1.27; previous revision: 1.26
done

Comment 25

[James Teh [:Jamie]]

(In reply to comment #8)
> Use STATE_LINKED.
> We're like IE in that we expose ROLE_LINK for any anchor. Sometimes the user
> may jump there. The <a name="foo"> are end point links.
Just for the record, I retract my original intent for this bug based on Aaron's reasoning. While a link destination (an A tag without an HREF attribute) should not be treated as a link which can be activated by the user, it may still be useful to expose this via accessibility APIs to allow the user to jump to spots of interest on a page. I still believe that "link" is a confusing role for this, but I guess it does make some sense and IE does this as well.

Comment 26

[Marco Zehe (:MarcoZ)]

Verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032005 Minefield/3.0b5pre