339286 - [ally] tab navigation

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

We should support @navindex attribute for xforms controls (see the specs http://www.w3.org/TR/xforms/slice8.html#id143292)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

The patch haven't full support of @navindex.
I can see two stuffs:
1) patch doesn't add navindex support for several controls, they are implementations of xf:output[appearance='full'].
2) patch doesn't support local navigation sequence (see http://www.w3.org/TR/xforms/slice4.html#evt-next)

Comment 2

[aaronr]

I'd argue that we shouldn't support navindex at all.  We should get it for free from the host language like xhtml or svg.  Ideally, our XBL should just inherit it and push it down to the control to handle like we do for accesskey.  At the most, we should only support it if we detect/know that the host language supports it.  Since xhtml 1.x doesn't contain navindex as an attribute and neither does xhtml2 as of now, I'd suggest that we take a wait and see attitude with supporting this attribute.

Comment 3

[Leigh L. Klotz, Jr.]

Do you support xhtml:tabindex?

Comment 4

[aaronr]

(In reply to comment #3)
> Do you support xhtml:tabindex?
> 

No, we don't support tabindex currently.  When we use native XHTML controls to represent a xforms control (for example, xf:input or xf:textarea), that wouldn't be a big deal just to forward the attribute value on down.  I don't know what would happen with our custom controls, though, like datepicker or xf:range.  Or if XUL-hosted XForms would be able to handle it, either.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> I'd argue that we shouldn't support navindex at all.  We should get it for free
> from the host language like xhtml or svg.  Ideally, our XBL should just inherit
> it and push it down to the control to handle like we do for accesskey.  At the
> most, we should only support it if we detect/know that the host language
> supports it.  Since xhtml 1.x doesn't contain navindex as an attribute and
> neither does xhtml2 as of now, I'd suggest that we take a wait and see attitude
> with supporting this attribute.
> 

If we skip @navindex attribute supporting and we'll relay on host language realization of navigation then I'm not sure it's right because:
1) We can miss supporting of local navigation sequence (http://www.w3.org/TR/xforms/slice4.html#evt-next).
2) If we'll use @tabindex instead of @navindex then we can get xforms control and underlying control are focusable and therefore they will be included in navigation sequence both (bug 313088).

Comment 6

[aaronr]

(In reply to comment #5)
> (In reply to comment #2)
> > I'd argue that we shouldn't support navindex at all.  We should get it for free
> > from the host language like xhtml or svg.  Ideally, our XBL should just inherit
> > it and push it down to the control to handle like we do for accesskey.  At the
> > most, we should only support it if we detect/know that the host language
> > supports it.  Since xhtml 1.x doesn't contain navindex as an attribute and
> > neither does xhtml2 as of now, I'd suggest that we take a wait and see attitude
> > with supporting this attribute.
> > 
> 
> If we skip @navindex attribute supporting and we'll relay on host language
> realization of navigation then I'm not sure it's right because:
> 1) We can miss supporting of local navigation sequence
> (http://www.w3.org/TR/xforms/slice4.html#evt-next).
> 2) If we'll use @tabindex instead of @navindex then we can get xforms control
> and underlying control are focusable and therefore they will be included in
> navigation sequence both (bug 313088).
> 


I don't have any problems supporting some kind of navigation attribute, but it needs to come from the host language.  Otherwise a document that has more than just XForms on it won't navigate properly if we keep stealing focus from things.  I mean, just think if we supported navindex and there is also xhtml on the page with tabindex on the various elements.  Who wins?  The behavior would be too unpredictable.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 224186 [details] [diff] [review]
patch

Canceling review request sicne it's not clear for me yet whether we will support @navindex attribute or not.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> I don't have any problems supporting some kind of navigation attribute, but it
> needs to come from the host language.  Otherwise a document that has more than
> just XForms on it won't navigate properly if we keep stealing focus from
> things.  I mean, just think if we supported navindex and there is also xhtml on
> the page with tabindex on the various elements.  Who wins?  The behavior would
> be too unpredictable.
> 

I think we shouldn't steal focus from nobody. @navidex should behave itself like analogue attribute from host document. The only problem I can see is what will be if xforms element will have both navindex and xhtml:tabindex (for example).

Comment 9

[aaronr]

(In reply to comment #8)
> (In reply to comment #6)
> 
> > I don't have any problems supporting some kind of navigation attribute, but it
> > needs to come from the host language.  Otherwise a document that has more than
> > just XForms on it won't navigate properly if we keep stealing focus from
> > things.  I mean, just think if we supported navindex and there is also xhtml on
> > the page with tabindex on the various elements.  Who wins?  The behavior would
> > be too unpredictable.
> > 
> 
> I think we shouldn't steal focus from nobody. @navidex should behave itself
> like analogue attribute from host document. The only problem I can see is what
> will be if xforms element will have both navindex and xhtml:tabindex (for
> example).
> 


I'd say that we ignore @xhtml:tabindex (or the corresponding attr from any other host language) on a xforms element if we are going to support @navindex.  Just like we do for almost every other host language attribute when it is placed on a xforms element.

Comment 10

[aaronr]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #6)
> > 
> > > I don't have any problems supporting some kind of navigation attribute, but it
> > > needs to come from the host language.  Otherwise a document that has more than
> > > just XForms on it won't navigate properly if we keep stealing focus from
> > > things.  I mean, just think if we supported navindex and there is also xhtml on
> > > the page with tabindex on the various elements.  Who wins?  The behavior would
> > > be too unpredictable.
> > > 
> > 
> > I think we shouldn't steal focus from nobody. @navidex should behave itself
> > like analogue attribute from host document. The only problem I can see is what
> > will be if xforms element will have both navindex and xhtml:tabindex (for
> > example).
> > 
> 
> 
> I'd say that we ignore @xhtml:tabindex (or the corresponding attr from any
> other host language) on a xforms element if we are going to support @navindex. 
> Just like we do for almost every other host language attribute when it is
> placed on a xforms element.
> 

sorry, now I am reversing my opinion somewhat.  I'd say we'd ignore any keyboard navigation attribute that has no place in the current host language.  If we are hosted in a xhtml 1.x document, then we should listen to tabindex.  If we are in a xhtml 2.x document, then we should listen to navindex.  Which really means that in our *-xhtml.xml bindings we should listen for tabindex and ignore tabindex since we'd probably need brand new widget bindings for xhtml2 anyhow.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)

> sorry, now I am reversing my opinion somewhat.  I'd say we'd ignore any
> keyboard navigation attribute that has no place in the current host language. 
> If we are hosted in a xhtml 1.x document, then we should listen to tabindex. 
> If we are in a xhtml 2.x document, then we should listen to navindex.  Which
> really means that in our *-xhtml.xml bindings we should listen for tabindex and
> ignore tabindex since we'd probably need brand new widget bindings for xhtml2
> anyhow.
> 

So, that means you'd like to skip special xforms support for keyboard navigation. Right? And we should allow gecko to handle xforms as any other custom namespace language (bug 313088). But what is with @navindex defined by xforms. At least I can see one advantage, it is common syntax independent from kind of host document. What does WG say?

Comment 12

[aaronr]

(In reply to comment #11)
> (In reply to comment #10)
> 
> > sorry, now I am reversing my opinion somewhat.  I'd say we'd ignore any
> > keyboard navigation attribute that has no place in the current host language. 
> > If we are hosted in a xhtml 1.x document, then we should listen to tabindex. 
> > If we are in a xhtml 2.x document, then we should listen to navindex.  Which
> > really means that in our *-xhtml.xml bindings we should listen for tabindex and
> > ignore tabindex since we'd probably need brand new widget bindings for xhtml2
> > anyhow.
> > 
> 
> So, that means you'd like to skip special xforms support for keyboard
> navigation. Right? And we should allow gecko to handle xforms as any other
> custom namespace language (bug 313088). But what is with @navindex defined by
> xforms. At least I can see one advantage, it is common syntax independent from
> kind of host document. What does WG say?
> 


There is no @navindex defined by xforms.  If you look in the schema, it doesn't exist.  Just like accesskey, navindex is mentioned in the spec in anticipation of xhtml2 having a navindex attribute.  But even THAT has gone by the wayside.  xhtml2 is using @nextfocus and @prevfocus in its latest draft.  I've asked the working group (http://lists.w3.org/Archives/Member/w3c-forms/2006AprJun/0259) to provide guidance, but I haven't had a response in 4 months.

So I'd say that we obey the 'spirit' of the spec and allow keyboard navigation using the attributes that are available in the host language.  But we still need to obey the keyboard navigation rules put down by the xforms spec like how to navigate through a repeat, etc.

Comment 13

[Aaron Leventhal]

If we're willing to put a hold on this, I can go get clarification.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I'd like to see some XHTML and XUL testcases which use tabindex
(with values < 0, == 0, > 0), just to make sure that everything works.

Comment 16

[Aaron Leventhal]

XHTML testcase using tabindex="0" and tabindex="-1": http://www.mozilla.org/access/dhtml/tree

Comment 17

[alexander :surkov (:asurkov)]

Attachment: xhtml test

I'll file new patch a bit later ;)

Comment 18

[alexander :surkov (:asurkov)]

Attachment: xul test

xul compact select/select1 are not focused properly due to bug 349461.

Comment 19

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 20

[alexander :surkov (:asurkov)]

Attachment: xhtml test2

Comment 21

[alexander :surkov (:asurkov)]

Attachment: xul test2

Comment 22

[alexander :surkov (:asurkov)]

Attachment: patch4

addresed smaug's comments about tabindex for output and label

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 253198 [details] [diff] [review]
patch4

r=me, but not sure which css rules we want to have under skin/ and which under content/

Comment 24

[aaronr]

Comment on attachment 253198 [details] [diff] [review]
patch4

>Index: extensions/xforms/resources/content/input-xul.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/input-xul.xml,v
>retrieving revision 1.11
>diff -u -8 -p -r1.11 input-xul.xml
>--- extensions/xforms/resources/content/input-xul.xml	24 Jan 2007 22:36:26 -0000	1.11
>+++ extensions/xforms/resources/content/input-xul.xml	29 Jan 2007 18:02:26 -0000

>@@ -190,19 +191,20 @@
> 
>     <resources>
>       <stylesheet src="chrome://xforms/skin/input-xul.css"/>
>     </resources>
> 
>     <content>
>       <children includes="label"/>
>       <xul:hbox class="-moz-menulist-container" flex="1">
>-        <html:input anonid="control" xbl:inherits="accesskey" flex="1"
>+        <html:input anonid="control" flex="1"
>                     class="-moz-menulist-textfield"
>-                    allowevents="true"/>
>+                    allowevents="true"
>+                    xbl:inherits="tabindex"/>

any particular reason that you took away accesskey inheritance here?

>Index: extensions/xforms/resources/content/select-xhtml.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select-xhtml.xml,v
>retrieving revision 1.9
>diff -u -8 -p -r1.9 select-xhtml.xml
>--- extensions/xforms/resources/content/select-xhtml.xml	6 Dec 2006 18:02:39 -0000	1.9
>+++ extensions/xforms/resources/content/select-xhtml.xml	29 Jan 2007 18:02:26 -0000

remember that this file is slated to go away with bug 339827.  Somehow you'll have to remember to move these changes over.

>Index: extensions/xforms/resources/content/select-xul.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/select-xul.xml,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 select-xul.xml
>--- extensions/xforms/resources/content/select-xul.xml	4 Dec 2006 14:55:25 -0000	1.7
>+++ extensions/xforms/resources/content/select-xul.xml	29 Jan 2007 18:02:26 -0000


remember that this file is slated to go away with bug 339827.  Somehow you'll have to remember to move these changes over.

>Index: extensions/xforms/resources/content/xforms-xhtml.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms-xhtml.xml,v
>retrieving revision 1.16
>diff -u -8 -p -r1.16 xforms-xhtml.xml
>--- extensions/xforms/resources/content/xforms-xhtml.xml	31 Oct 2006 22:23:36 -0000	1.16
>+++ extensions/xforms/resources/content/xforms-xhtml.xml	29 Jan 2007 18:02:27 -0000
>@@ -58,37 +58,32 @@
>           xmlns:wairole="http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy#">
> 
> 
>   <!-- OUTPUT: <DEFAULT> -->
>   <binding id="xformswidget-output"
>            extends="chrome://xforms/content/xforms.xml#xformswidget-output-base">
>     <content>
>       <children includes="label"/>
>-      <!-- XXX initialize span with a space until repeat is xbl-ized.  Part
>-           of workaround for bug 322975
>-      -->
>-      <html:span class="xf-value" anonid="control"> </html:span>
>-      <children/>
>+      <html:span anonid="control"
>+                 class="xf-value"
>+                 xbl:inherits="tabindex">
>+        <children/>
>+      </html:span>

I guess if a form author is goofy enough to put a tabindex attribute on an output, we should honor it.  But it'll seem odd to users who are tabbing around the form and all the sudden they can't see who has focus.  Same with label below.  Could you put a comment in those two places so that if someone comes along later, they know that we allowed tabindex on purpose so they don't try to remove it thinking it is unnecessary?

>   <!-- LABEL: <DEFAULT> -->
>   <binding id="xformswidget-label"
>            extends="chrome://xforms/content/xforms.xml#xformswidget-label-base">
>     <content>
>-      <html:span anonid="implicitcontent"/>
>-      <html:span anonid="explicitcontent"><children/></html:span>
>+      <html:span anonid="implicitcontent"
>+                 xbl:inherits="tabindex"/>
>+      <html:span anonid="explicitcontent"
>+                 xbl:inherits="tabindex"><children/></html:span>
>     </content>
> 
>     <implementation>
>       <method name="getControlElement">
>         <body>
>           return {
>             _implicitContent: this.ownerDocument.
>               getAnonymousElementByAttribute(this, "anonid", 'implicitcontent'),
>             _explicitContent: this.ownerDocument.
>               getAnonymousElementByAttribute(this, 'anonid', 'explicitcontent'),
>             __proto__: this,
> 
>             set value(val) {
>               if (val != null) {
>-                this._implicitContent.textContent = val;
>                 this._explicitContent.style.display = 'none';
>+                this._implicitContent.style.display = 'inline';
>+                this._implicitContent.textContent = val;
>               } else {
>-                this._implicitContent.textContent = '';
>                 this._explicitContent.style.display = 'inline';
>+                this._implicitContent.style.display = 'none';
>+                this._implicitContent.textContent = '';
>               }

You are going to have a conflict here with the patch to bug 339827.  Please make sure that after these two patches go in that you have what you want and that something isn't left out/removed by the patch that goes in second.

I also don't know how to best separate CSS from the extension root to the skins.  I guess at some point we need to follow up with someone and make a policy.

With those questions and concerns answered and addressed, plus that accesskey inheritance added back in, r=me

Comment 25

[alexander :surkov (:asurkov)]

Attachment: patch5

Comment 26

[alexander :surkov (:asurkov)]

I putted comments for lablel/output about tab navigation and I will be care about merging patches :)

> I also don't know how to best separate CSS from the extension root to the
> skins.  I guess at some point we need to follow up with someone and make a
> policy.

We have bug for this, let's save this question for it. Now I'd like to have the new css file since it's compatible with css'ing XUL in XForms.

Comment 27

[alexander :surkov (:asurkov)]

Attachment: patch6

forgot to add new file

for checkin

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch7

Sorry, previous is wrong patch again.

for check in

Comment 29

[alexander :surkov (:asurkov)]

checked in by smaug

Comment 30

[aaronr]

checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16