280065 - XUL text-link widget is not accessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

Right now we use <label class="text-link"> for our link widget.
First, it does not make sense that we use a class to add a widget with
functionality. A link should have it's own XUL tag, like <link> or <textlink>,
with a src attribute or something.

http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/text.xml#211

Whether we decide to create a new link tag or not, we do need to implement
nsIAccessibleProvider for this element and create a xul link accessible that
exposes ROLE_LINK for it.

Comment 1

[parente]

Attachment: Modifications to text-link binding

Comment 2

[parente]

Attachment: Use of new binding in xul.css

Comment 3

[parente]

Attachment: Test case

Comment 4

[parente]

I started modifying the existing text-link binding. Two problems. First, I keep 
getting a security error when trying to open the link href from XBL. Second, XUL 
ignores the link tag event after I've modified the xul.css file.

The test case demonstrates that creating new classes in the xul.css file works 
as expected, but creating new, top level styles representing new tags does not. 
I must be missing something.

Comment 5

[[e-mail]]

Comment on attachment 185023 [details]
Modifications to text-link binding

>openDialog('chrome://browser/content/browser.xul', '_blank', 'chrome,all,dialog=no', href, null, null);
Why not just open(href)?

Comment 6

[[e-mail]]

(In reply to comment #4)
>The test case demonstrates that creating new classes in the xul.css file works 
>as expected, but creating new, top level styles representing new tags does not. 
>I must be missing something.
Perhaps you forgot to tell your <link> tag that it should be displaying text?

Comment 7

[Aaron Leventhal]

(In reply to comment #6)
> (In reply to comment #4)
> >The test case demonstrates that creating new classes in the xul.css file works 
> >as expected, but creating new, top level styles representing new tags does not. 
> >I must be missing something.
> Perhaps you forgot to tell your <link> tag that it should be displaying text?

Neil, where/how does one do that?

Comment 8

[[e-mail]]

Something like <binding ... display="xul:text" ...> should work.

Comment 9

[parente]

That's works, sort of. The formatting is a bit off when compared with the other 
text.

So if the binding is being attached to a "top level" tag (e.g. <link />) it 
needs to be explicitly displayed whereas when it's attached to a class (e.g. 
<label class="text-link" />) it doesn't? The same binding is being used in both 
cases and is visible in the second case but not the first without using "xul:
display".

Comment 10

[[e-mail]]

(In reply to comment #9)
>The formatting is a bit off when compared with the other text.
Possibly because your theme is styling the label tag but not the link tag?

>So if the binding is being attached to a "top level" tag (e.g. <link />) it 
>needs to be explicitly displayed whereas when it's attached to a class (e.g. 
><label class="text-link" />) it doesn't? The same binding is being used in both 
>cases and is visible in the second case but not the first without using "xul:
>display".
<label> tags already know (hardwired in nsCSSFrameConstructor) to display text.

Comment 11

[parente]

It appears that there is still on-going discussion (http://wiki.mozilla.org/XUL:
New_Widgets) about whether or not a xul:link tag is desired or whether html:a 
should be used instead. Plus, the text-link style is referenced repeatedly 
throughout a number of stylesheets and themes. I'm guessing it'll have to be 
slowly deprecated to allow the themes to catch up and use link instead.

In the mean time, it might make more sense just to 1) make the text-link binding 
accessible as ROLE_LINK, 2) give it an href attribute, and 3) have it provide a 
convience click handler that opens the URL given by the href. If it turns out 
people want xul:link later, they can simply attach the text-link binding to the 
link tag.

Comment 12

[parente]

Attachment: XULTextAccessible first try - binding independent

Comment 13

[parente]

Attachment: XULTextAccessible first try - binding independent

Relies on patch for bug 298572 to report accessible name properly.
Cannot provide standalone test case because of security problems (bug 297069).

Best way to test is to modify the label in
http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/content/extensions.xul#286
so it is a link:

<label class="text-link" href="foobar" />

Note this patch is binding independent so if text-link disappears later, it
will continue to work. The binding only needs to invoke createXULLinkAccessible
on the IAccessibilityService to expose itself as a link.

Comment 14

[Aaron Leventhal]

Comment on attachment 187126 [details] [diff] [review]
XULTextAccessible first try - binding independent

I don't see anyone using CreateXULLinkAccessible(). Did the XBL part of the
patch accidentally get left out?

Nits:

In general check the coding style guidelines on mozilla.org. You're doing some
stuff I did too, before working in Mozilla and people want all the whitespace
just so, yada yada yada.

Always put a space before and after operators, including the ? operator in ? :

For methods, always put the { on a new line, as in:
+PRBool nsXULLinkAccessible::IsALink() {

All the _retval's in method params came from a time when I didn't know better.
Don't follow that custom. Use aFoo for paramater names.

Keep new #include's alphabetized (although ignoring the "nsI" bit and keeping
the matching header for the .cpp as the first one).

Collapse these into 1 line:
nsresult rv;
rv = nsLinkableAccessible::GetState(_retval);

Always put a space before (

Always use { and }, even for 1 line, at least in this module. Don't ask. A lot
of sr='s started requiring that when a bad smoketest blocking bug was created
because of a { } mistake.

Don't compact things like this into one line, because it makes it harder to set
breakpoints when debugging.
+  if(NS_FAILED(rv)) return rv;

In fact the way I would write that part is:

+  rv = nsLinkableAccessible::GetState(_retval);
+  if (NS_SUCCEEDED(rv) && IsALink()) {
+    *aState |= STATE_FOCUSABLE;
+  }
+  return rv;
+}

I think IsAlink() could do away with the temporary nsCOMPtr<nsIContent> and
just use mLinkContent from the start

Comment 15

[parente]

Attachment: Includes missing XBL, fixes nits, simplifies IsALink

Comment 16

[Aaron Leventhal]

Comment on attachment 187208 [details] [diff] [review]
Includes missing XBL, fixes nits, simplifies IsALink

+nsAccessibilityService::CreateXULLinkAccessible(nsIDOMNode *aNode,
nsIAccessible **_retval)
Change _retval to aAccessible or something

+  // should there be a third case where it becomes just text?
I'd say no, so jsut remove that comment. BTW, the way to write temporary
comments in the code is something like:
// XXX Should we be doing blah instead? It might be better because blah blah.
People can search on the XXX's so it's better for guys who just like to clean
things up. Anyway, that's the custom :)

Looks like I may have made a mistake when I said to use mLinkContent, sorry :/
Either go back to the old method or set mLinkContent = nsnull when there is no
href attribute. Otherwise the second call to IsALink() will just see that
mLinkContent is not null and return true. Oops.

Fix those things and r=me

Comment 17

[parente]

Attachment: Removes temp comment, fixes mLinkContent always non-null

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 187219 [details] [diff] [review]
Removes temp comment, fixes mLinkContent always non-null

>Index: toolkit/content/widgets/text.xml
>===================================================================

>+      <handler event="click" action="if (this.disabled) return;
>+                                     var href = this.getAttribute('href');
>+                                     if (href) {
>+                                       open(href);
>+                                     }
>+                                    " />

Why not use the block style, like at
http://lxr.mozilla.org/mozilla/source/browser/base/content/search.xml#360 ?
Makes it clearer, and most other code uses it for more than one or two lines.

Comment 19

[parente]

Attachment: Pretties up click handler, otherwise same as last

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Hyatt isn't currently reviewing any Mozilla stuff. Neil or Mike Connor are
probably better choices for Toolkit reviews.

Comment 21

[Aaron Leventhal]

(In reply to comment #20)
> Hyatt isn't currently reviewing any Mozilla stuff. Neil or Mike Connor are
> probably better choices for Toolkit reviews.

We should find someone to update the super reviewers page on mozilla.org so it
is a bit more accurate.

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 187856 [details] [diff] [review]
Pretties up click handler, otherwise same as last

r=me on the toolkit bits, but a real SR needs to happen for the rest of this.

Comment 23

[Aaron Leventhal]

Comment on attachment 187856 [details] [diff] [review]
Pretties up click handler, otherwise same as last

This one has r=aaronlev for the accessibility module changes, and r=mconnor for
the toolkit changes.

We just need sr= for the accessibility module changes now.

Comment 24

[Mike Connor [:mconnor]]

Comment on attachment 187856 [details] [diff] [review]
Pretties up click handler, otherwise same as last

additional note on this:

>+      <handler event="click">
>+      <![CDATA[
>+        if (this.disabled) return;
>+        var href = this.getAttribute('href');
>+        if (href) {
>+          open(href);
>+        }

prevailing style in toolkit doesn't use return on the same line.  Unnecessary
brackets are generally wrong too.

if (this.disabled)
  return;

var href = this.getAttribute('href');
if (href)
  open(href);

Comment 25

[[e-mail]]

Comment on attachment 187856 [details] [diff] [review]
Pretties up click handler, otherwise same as last

>+PRBool nsXULLinkAccessible::IsALink()
>+{
>+  // use the cached answer if it exists
>+  if (mIsALinkCached) {
>+    return mLinkContent ? PR_TRUE: PR_FALSE;
>+  }
>+  // indicate the test result is cached
>+  mIsALinkCached = PR_TRUE;
>+  mLinkContent = do_QueryInterface(mDOMNode);
>+  if (!mLinkContent) {
>+    return PR_FALSE;
>+  }
>+  // not a link if there is no href attribute
>+  if (!mLinkContent->HasAttr(kNameSpaceID_None, nsAccessibilityAtoms::href)) {
>+    return PR_FALSE;
>+  }
>+  // can't detect traversed yet; need ILink interface
>+  return PR_TRUE;
>+}
The base nsLinkableAccessible only sets mLinkContent if the element is a link.

Comment 26

[parente]

Attachment: Changes requested by neil, mconnor

Uses temp local to store mLinkContent

Comment 27

[parente]

Attachment: Changes requested by neil, mconnor

Comment 28

[[e-mail]]

Comment on attachment 189192 [details] [diff] [review]
Changes requested by neil, mconnor

>+    return mLinkContent ? PR_TRUE: PR_FALSE;
Nit: space needed between PR_TRUE and :

Comment 29

[Aaron Leventhal]

Thanks Pete!

Checking in accessible/src/base/nsAccessibilityAtomList.h;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityAtomList.h,v  <-- 
nsAccessibilityAtomList.h
new revision: 1.26; previous revision: 1.25
done
Checking in accessible/src/base/nsAccessibilityService.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityService.cpp,v  <-- 
nsAccessibilityService.cpp
new revision: 1.146; previous revision: 1.145
done
Checking in accessible/src/base/nsBaseWidgetAccessible.h;
/cvsroot/mozilla/accessible/src/base/nsBaseWidgetAccessible.h,v  <-- 
nsBaseWidgetAccessible.h
new revision: 1.14; previous revision: 1.13
done
Checking in accessible/src/xul/nsXULTextAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULTextAccessible.cpp,v  <-- 
nsXULTextAccessible.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in accessible/src/xul/nsXULTextAccessible.h;
/cvsroot/mozilla/accessible/src/xul/nsXULTextAccessible.h,v  <-- 
nsXULTextAccessible.h
new revision: 1.13; previous revision: 1.12
done
Checking in toolkit/content/widgets/text.xml;
/cvsroot/mozilla/toolkit/content/widgets/text.xml,v  <--  text.xml
new revision: 1.14; previous revision: 1.13
done
Checking in accessible/public/nsIAccessibilityService.idl;
/cvsroot/mozilla/accessible/public/nsIAccessibilityService.idl,v  <-- 
nsIAccessibilityService.idl
new revision: 1.49; previous revision: 1.48
done

Comment 30

[Aaron Leventhal]

Attachment: Patch that applies to the trunk, which was checked in and contains Neil's last whitespace nit

Comment 31

[[e-mail]]

Comment on attachment 189192 [details] [diff] [review]
Changes requested by neil, mconnor

>+          open(href);
I was racking my brains trying to work out why I was suspicious of this. I've
just remembered :-/
1. This might not work in modal dialogs. As I recall, modal dialogs can only
open modal dialogs. We had issues trying to open Help from a dialog's Help
button.
2. This might not work in Thunderbird, or it might open the wrong sort of
window altogether.

Comment 32

[Aaron Leventhal]

(In reply to comment #31)
> (From update of attachment 189192 [details] [diff] [review] [edit])
> >+          open(href);
> I was racking my brains trying to work out why I was suspicious of this. I've
> just remembered :-/

So override onclick in the capturing phase and do event.preventDefault() in it
in those cases. We need some kind of decent default behavior for links.

Comment 33

[Aaron Leventhal]

This was checked in already.

Comment 34

[Asaf Romano (gone)]

(In reply to comment #32)
> (In reply to comment #31)
> > (From update of attachment 189192 [details] [diff] [review] [edit] [edit])
> > >+          open(href);
> > I was racking my brains trying to work out why I was suspicious of this. I've
> > just remembered :-/
> 
> So override onclick in the capturing phase and do event.preventDefault() in it
> in those cases. We need some kind of decent default behavior for links.
> 

This doesn't work (probably becuase it's too late to preventDefault, i'm not
sure), and, FYI, it's indeed very broken in modal dialogs.
> 

Comment 35

[Firefox Bug Husbandry Bot]

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