Closed Bug 423603 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Disability Access APIs, defect, critical)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9beta5

People

(Reporter: Jamie, Assigned: MarcoZ)

References

Details

(Keywords: access, regression)

Attachments

(3 files, 2 obsolete files)

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
Version: unspecified → Trunk
This example includes A tags with and without HREF attributes, including information about which should be exposed and which should not.
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?
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.
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?
Severity: minor → critical
Yes, this is a regression from bug 421066. Patch coming up, it's a simple fix.
Blocks: 421066
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: aaronleventhal → marco.zehe
Status: NEW → ASSIGNED
Attachment #310438 - Flags: review?(surkov.alexander)
nope, there may be a real link with name attribute, possibly we need to test nsILink interface.
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.
Because of comment 8 I suggest WONTFIX.
I take it back. I thought the issue was an old decision but Marco points out in comment 5 that it's a regression.
Keywords: access, regression
Comment on attachment 310438 [details] [diff] [review]
Patch

Let's get this in ASAP!
Attachment #310438 - Flags: review?(surkov.alexander)
Attachment #310438 - Flags: review+
Attachment #310438 - Flags: approval1.9?
Summary: A elements with no HREF should not be exposed as links → A elements with no HREF should not have state_linked, regression from bug 421066
Comment on attachment 310438 [details] [diff] [review]
Patch

Wait, Surkov is right. A link can have a name and an href.
Attachment #310438 - Flags: review+
Attachment #310438 - Flags: approval1.9?
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #310438 - Attachment is obsolete: true
Attachment #310473 - Flags: review?(aaronleventhal)
Comment on attachment 310473 [details] [diff] [review]
Patch v2

Perfect.
Attachment #310473 - Flags: review?(aaronleventhal) → review+
Comment on attachment 310473 [details] [diff] [review]
Patch v2

Ugh. I am obviously not paying attention because of my move. Sorry.
Attachment #310473 - Flags: review+
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.
Attachment #310486 - Flags: review?(marco.zehe)
Attachment #310486 - Flags: review+
Attachment #310486 - Flags: approval1.9?
Target Milestone: --- → mozilla1.9beta5
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
Attachment #310486 - Flags: approval1.9? → approval1.9+
hmm, I assume this patch has r=aaronlev ?
(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.
It has r=aaronlev
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.
Okay, Surkov can you check in this version then? We definitely need this in the beta.
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

Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(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.
Verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008032005 Minefield/3.0b5pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.