277516 - Need a way to follow accessibilty.tabfocus for xul elements

Status: RESOLVED FIXED

(Core :: Disability Access APIs, enhancement)

Description

[Asaf Romano (gone)]

On OS X, we should follow accessibilty.tabfocus for xul documents as it is based
on a system preference (see bug 187508).

I understand that it doesn't have any meaning on win32/gnome, so we should
probably have a hidden pref to contorl it (default to true on mac, false
everywhere else)

The relevant code is here:
http://lxr.mozilla.org/seamonkey/source/content/xul/content/src/nsXULElement.cpp#1431

Comment 1

[Asaf Romano (gone)]

Aaron: I may take it, pending your comments.

Comment 2

[Aaron Leventhal]

Makes sense to me. Go ahead and do it. How will you determine which XUL elements
are tabbable in the various scenarios? What about when new elements are
developed with XBL? We might need a new value for the CSS property
-moz-user-focus to help with this.

Comment 3

[Asaf Romano (gone)]

(In reply to comment #2)
> We might need a new value for the CSS property
> -moz-user-focus to help with this.

good. changing summary.

Comment 4

[Asaf Romano (gone)]

Attachment: v1

Comment 5

[Asaf Romano (gone)]

Comment on attachment 170816 [details] [diff] [review]
v1

aaron is going to be away

Comment 6

[Boris Zbarsky [:bzbarsky]]

Why are there underscores in the property name?

What does "follow-forms" mean exactly?

Have you tested the interaction of this with rules like:

:focus {
  -moz-user-focus: none;
}

?

I'd condition the expensive-ish process of getting the style on the focus mask
being right...

Apart from all that, I'm not sure I'm qualified to r= this patch, since I can't
tell what it's trying to accomplish.  I could probably sr it, though.

Comment 7

[Asaf Romano (gone)]

(In reply to comment #6)
> Why are there underscores in the property name?

I suppose we prefer "-" ?

> What does "follow-forms" mean exactly?

It means: focus it only if form controls are focusable (per accessibilty.tabfocus)

> Have you tested the interaction of this with rules like:
> 
> :focus {
>   -moz-user-focus: none;
> }
> 

I'm not sure I understnad what shuld this rule do.

Comment 8

[Asaf Romano (gone)]

(In reply to comment #6)

> :focus {
>   -moz-user-focus: none;
> }
> 

well, as i'm not touching the code that actaully handles -moz-user-suppose (in
ESM), I suppose it should work as usual.

Comment 9

[Asaf Romano (gone)]

Comment on attachment 170816 [details] [diff] [review]
v1

with the keyword name fixed (s/_/-)

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I suppose we prefer "-" ?

Yes.

> It means: focus it only if form controls are focusable (per accessibilty.tabfocus)

Document that, please?  The name is non-obvious (perhaps a better name is in order?)

So why are we only checking for this on XUL elements?  Shouldn't this apply to
all elements?

> I suppose it should work as usual.

As usual is probably broken.  The point is, -moz-user-focus has some by-design
issues, so I'd like to see a different solution if there is a reasonale one
instead of reusing known-broken functionality.

Comment 11

[Asaf Romano (gone)]

(In reply to comment #10)

First of all, i'll /re-explain/ comment 0: On OS X, both UI and webpages are
expected to be handled by a system pref named "Full keyboard access". In bug
187508, i have fixed accessibilty.tabfocus to do this, but this pref applies
html content only.

two notes here:
  1) Having xul ui on win32/gnome follow accessibilty.tabfocus doens't make sense
  2) I can't check in nsXULElement what element is comming, because of new xbl
elements (say the searchbar); having it as a pref cannot be the solution, probably.


> > It means: focus it only if form controls are focusable (per
accessibilty.tabfocus)
> 
> Document that, please?  The name is non-obvious (perhaps a better name is in
order?)

In the code, or is there a docuemnt i should update?

> So why are we only checking for this on XUL elements?  Shouldn't this apply to
> all elements?

Probably not; form elements are form elements, and links cannot be form elements :)
 
> > I suppose it should work as usual.
> 
> As usual is probably broken.  The point is, -moz-user-focus has some by-design
> issues, so I'd like to see a different solution if there is a reasonale one
> instead of reusing known-broken functionality.

as mentioned, pref cannot be the solution, maybe a new css property?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 2) I can't check in nsXULElement what element is comming

Actually, you could have an API that any XBL binding that wishes to be focusable
in this manner could implement...  That may be a little slower than your
proposed solution, however, and a bit more work for binding authors.

> In the code, or is there a docuemnt i should update?

Yes, in the code.

>> So why are we only checking for this on XUL elements?  Shouldn't this apply
> to
>> all elements?

> Probably not; form elements are form elements, and links cannot be form
> elements :)

I'm not sure I follow this statement... In particular, XBL bindings could be
attached to random XML elements, XForms can create form controls, and so forth.
 So it's worth doing a solution that will work for all of these, no?

> as mentioned, pref cannot be the solution, maybe a new css property?

Using any CSS property to do focus is inherently problematic, because focus can
be selected on, is the point.

Comment 13

[Asaf Romano (gone)]

(In reply to comment #12)
> > 2) I can't check in nsXULElement what element is comming
> 
> Actually, you could have an API that any XBL binding that wishes to be focusable
> in this manner could implement...  That may be a little slower than your
> proposed solution, however, and a bit more work for binding authors.

I'm not willing to fix evrery binding in mozilla/... plus, this should be
controled outside of the binding (i'm not expecting extension authors to check
for navigator.platform...)

> > In the code, or is there a docuemnt i should update?
> 
> Yes, in the code.

ok, tbd.


> >> So why are we only checking for this on XUL elements?  Shouldn't this apply
> > to
> >> all elements?
> 
> > Probably not; form elements are form elements, and links cannot be form
> > elements :)
> 
> I'm not sure I follow this statement... In particular, XBL bindings could be
> attached to random XML elements, XForms can create form controls, and so forth.
>  So it's worth doing a solution that will work for all of these, no?

should using the tabfocus pref be prefable for widgets other than UI elements? 

If the answer is yes, i should add this check to
nsGenericHTMLFrameElement::IsFocusable and nsXMLElement::IsFocusable as well.


> > as mentioned, pref cannot be the solution, maybe a new css property?
> 
> Using any CSS property to do focus is inherently problematic, because focus can
> be selected on, is the point.

suggestions?

Comment 14

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #13)
> I'm not willing to fix evrery binding in mozilla/... plus, this should be
> controled outside of the binding (i'm not expecting extension authors to check
> for navigator.platform...)

Your solution also requires some sort of knowledge of bindings, no?  The
-moz-user-focus rule would need to go inside the appropriate bindings'
stylesheets...

> >  So it's worth doing a solution that will work for all of these, no?
> 
> should using the tabfocus pref be prefable for widgets other than UI elements? 

What do you mean?  We follow the pref for HTML, right?  Why not XForms, then?

> If the answer is yes, i should add this check to
> nsGenericHTMLFrameElement::IsFocusable and nsXMLElement::IsFocusable as well.

Why frame element only?

Basically, you're adding a new CSS property.  From what I can see, this property
will only apply to XUL elements. It's not clear to me why you're limiting it
like that....

> suggestions?

Frankly, I think it's best if the element itself can know whether it's a
form-control-like thing.

In fact, that's the only safe way to do it, I suspect...

Comment 15

[Asaf Romano (gone)]

(In reply to comment #14)

> Your solution also requires some sort of knowledge of bindings, no?  The
> -moz-user-focus rule would need to go inside the appropriate bindings'
> stylesheets...

hmm, yeah.
 
> > >  So it's worth doing a solution that will work for all of these, no?
> > 
> > should using the tabfocus pref be prefable for widgets other than UI elements? 
> 
> What do you mean?  We follow the pref for HTML, right?  Why not XForms, then?

I agree we should follow the tabfocus pref for xforms (and maybe other
elements), but does it need to be prefable (i mean, would you want not to follow
the tabfocus pref for a particular xforms element for example)?

In other words, xul/xbl is the only point you woudn't like to follow the
tabfocus pref.

> > If the answer is yes, i should add this check to
> > nsGenericHTMLFrameElement::IsFocusable and nsXMLElement::IsFocusable as well.
> 
> Why frame element only?

hmm, are there more places that implement IsFocusable (not nsIFrame) that don't
check for nsGenericHTMLFrameElement,nsXMLElement or nsXULElement IsFocusable impl'?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> In other words, xul/xbl is the only point you woudn't like to follow the
> tabfocus pref.

But XBL can be applied to everything, not just XUL (so I could write a binding
for an HTML element, eg).

> hmm, are there more places that implement IsFocusable (not nsIFrame) that don't
> check for nsGenericHTMLFrameElement,nsXMLElement or nsXULElement IsFocusable
> impl

fieldsets, anchors, etc, etc.  None of those are FrameElement....

Comment 17

[Asaf Romano (gone)]

(In reply to comment #16)
> > In other words, xul/xbl is the only point you woudn't like to follow the
> > tabfocus pref.
> 
> But XBL can be applied to everything, not just XUL (so I could write a binding
> for an HTML element, eg).

Good to know, but i guess i meant xul only.

 
> > hmm, are there more places that implement IsFocusable (not nsIFrame) that don't
> > check for nsGenericHTMLFrameElement,nsXMLElement or nsXULElement IsFocusable
> > impl
> 
> fieldsets, anchors, etc, etc.  None of those are FrameElement....

at least for anchors, they check nsGenericHTMLElement::IsFocusable (which i
meantioned earlier). I wasn't able to find impl' of IsFocusable outside of
content/ ...

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Good to know, but i guess i meant xul only.

Right.  But why?

> at least for anchors, they check nsGenericHTMLElement::IsFocusable (which i
> meantioned earlier).

Actually, you had not.  Hence me being confused.

There better not be any impls of IsFocusable outside of content/....

Comment 19

[Asaf Romano (gone)]

Attachment: v2

Comment 20

[[e-mail]]

FYI, XUL textboxes aren't inherently focusable.
Instead, XBL is used to give them an inner <html:input> which is focusable.

Also, I don't understand why we need an extra preference -
we went to great lengths to make sure tab focus followed Mac OS X settings.

Comment 21

[Asaf Romano (gone)]

(In reply to comment #20)

> Also, I don't understand why we need an extra preference -
> we went to great lengths to make sure tab focus followed Mac OS X settings.

We did make sure tabfocus is follows OS X system setting; but the tab focus pref
doesn't apply XUL elements (and we probably don't want to change this behavior
on other systems).

Comment 22

[Asaf Romano (gone)]

Attachment: v2.1

remove check for textbox.

Comment 23

[sairuh (rarely reading bugmail)]

note to self: see also bug 203734.

Comment 24

[Aaron Leventhal]

(In reply to comment #21)
> We did make sure tabfocus is follows OS X system setting; but the tab focus pref
> doesn't apply XUL elements (and we probably don't want to change this behavior
> on other systems).

If we don't have the pref, and someone on Windows or Linux changes their tab
navigation setting to make browsing HTML more convenient, it will also affect
navigation of dialogs. Most of the time, however, people are probably just
turning off the ability to tab to links. If they really only want to tab to text
fields, they are serious mouse users anyway.

I could honestly go either way. I'm fine with having the hidden pref which
defaults to true for Mac, or with just always paying attention to the pref in XUL.

Neil, comments?

Comment 25

[Aaron Leventhal]

Comment on attachment 171267 [details] [diff] [review]
v2.1

+      nsCOMPtr<nsIDOMXULTreeElement> xulTree = 
+	 do_QueryInterface(NS_STATIC_CAST(nsIContent*, this));
Is the cast really necessary? Is there an ambiguious inheritance chain?

Why I don't understand is how this affects whether something like a XUL
checkbox is focusable. What point is there in doing this just for trees.

Comment 26

[Asaf Romano (gone)]

(In reply to comment #25)
 
> Why I don't understand is how this affects whether something like a XUL
> checkbox is focusable. What point is there in doing this just for trees.

Short Answer: that's how native apps on OS X bahave.
Long answer: Tress are also used as our "list" element as well (say, mail&news
messages lists), where arrows are expected to work in order to select an item.

Comment 27

[Aaron Leventhal]

Comment on attachment 171267 [details] [diff] [review]
v2.1

Okay, I get it now. I take it that textbox works because they use an HTML
textbox as anonymous content?

You shouldn't need the static cast inside the QueryInterface. But QI is slow
anyway, compared to checking the tag.

For example, this would be faster:
nsCOMPtr<nsIAtom> tag = Tag();

if (tag != nsXULAtoms::tree && tag!= nsXULAtoms::listbox) {
tabIndex = -1;
}

Comment 28

[[e-mail]]

NodeInfo()->Equals(nsXULAtoms::tree) seems to be popular.

Comment 29

[Asaf Romano (gone)]

Attachment: v2.2

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 172581 [details] [diff] [review]
v2.2

>Index: content/base/public/nsIContent.h

>+  // accessibility.tabfocus_applies_xul pref.

This pref name is still not obvious to me.... see below.

> If it is set to true,
>+  // the tabfocus bit field applies xul documents.

Did you mean "applies to xul documents"?  Note the preposition.  Perhaps there
should be one in the pref name as well?  And the global name here.  Similar in
other comments throughout.

And it really applies to XUL _elements_, not documents, no?

>Index: content/xul/content/src/nsXULElement.cpp

>+    if (tabIndex != -1 &&
>+          (sTabFocusModelAppliesXUL && !(sTabFocusModel & eTabFocus_formElementsMask))) {

So this is "if widget is generally tabbable, and we're applying the focus model
to XUL, and the focus model says to not focus form elements"... (though there
are extra parens and a weird indent here, please fix those).

>+      if (!mNodeInfo->Equals(nsXULAtoms::tree) && !mNodeInfo->Equals(nsXULAtoms::listbox))
>+        tabIndex = -1; 

... and the widget is not a tree or a listbox, don't allow tabbing to it.

>Index: modules/libpref/src/init/all.js
> #ifndef XP_MACOSX
>+pref("accessibility.tabfocus_applies_xul", false);
>+#else
>+// Only on mac tabfocus is expected to handle UI widgets as well as web content
>+pref("accessibility.tabfocus_applies_xul", true);
> #endif

So let me see if I understand correctly.  On non-mac, the behavior is
unchanged.  On mac, if tabbing to form elements is not supposed to happen,
disable tabbing to anything except tree and listbox.  If tabbing to form
elements _is_ supported, allow tabbing everywhere.

In other words, this just adds more cases when we disallow tabbing on Mac.  Is
that correct?  If so, wasn't the aim to allow tabbing to _more_ things on mac?

Marking sr- for the grammar fixes, at least, but I'd really like to see the
answer to this last question.

Comment 31

[Asaf Romano (gone)]

Attachment: v2.3

(as mentioned on irc: after landing this patch, we need to remove
-moz-user-focus: ignore from many style rules in the mac themes; so 'yes' - the
aim is to allow tabbing to more elements)

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 172761 [details] [diff] [review]
v2.3

sr=bzbarsky

Comment 33

[Asaf Romano (gone)]

checked in 2005-01-29 15:47