Last Comment Bug 339286 - [ally] tab navigation
: [ally] tab navigation
Status: RESOLVED FIXED
: access, fixed1.8.0.12, fixed1.8.1.4
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: alexander :surkov
: Stephen Pride
Mentors:
http://www.w3.org/TR/xforms/slice8.ht...
Depends on:
Blocks: xformsa11y
  Show dependency treegraph
 
Reported: 2006-05-25 19:49 PDT by alexander :surkov
Modified: 2007-04-23 16:07 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (20.51 KB, patch)
2006-06-02 03:46 PDT, alexander :surkov
no flags Details | Diff | Review
patch2 (26.91 KB, patch)
2007-01-28 08:48 PST, alexander :surkov
no flags Details | Diff | Review
xhtml test (4.63 KB, application/xhtml+xml)
2007-01-29 07:15 PST, alexander :surkov
no flags Details
xul test (3.66 KB, application/vnd.mozilla.xul+xml)
2007-01-29 07:22 PST, alexander :surkov
no flags Details
patch3 (39.23 KB, patch)
2007-01-29 07:23 PST, alexander :surkov
bugs: review+
Details | Diff | Review
xhtml test2 (5.04 KB, application/xhtml+xml)
2007-01-29 10:04 PST, alexander :surkov
no flags Details
xul test2 (3.96 KB, application/vnd.mozilla.xul+xml)
2007-01-29 10:05 PST, alexander :surkov
no flags Details
patch4 (49.13 KB, patch)
2007-01-29 10:06 PST, alexander :surkov
bugs: review+
aaronr: review+
Details | Diff | Review
patch5 (45.52 KB, patch)
2007-02-13 08:21 PST, alexander :surkov
no flags Details | Diff | Review
patch6 (47.90 KB, patch)
2007-02-13 08:28 PST, alexander :surkov
no flags Details | Diff | Review
patch7 (47.90 KB, patch)
2007-02-13 08:30 PST, alexander :surkov
no flags Details | Diff | Review

Description alexander :surkov 2006-05-25 19:49:32 PDT
We should support @navindex attribute for xforms controls (see the specs http://www.w3.org/TR/xforms/slice8.html#id143292)
Comment 1 alexander :surkov 2006-06-02 03:46:36 PDT
Created attachment 224186 [details] [diff] [review]
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 2006-06-05 10:08:06 PDT
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. 2006-06-05 14:50:46 PDT
Do you support xhtml:tabindex?
Comment 4 aaronr 2006-06-05 15:04:51 PDT
(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 2006-06-05 19:30:42 PDT
(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 2006-06-06 09:50:40 PDT
(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 2006-09-17 06:27:33 PDT
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 2006-10-14 08:58:12 PDT
(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 2006-10-16 18:44:05 PDT
(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 2006-10-16 18:53:42 PDT
(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 2006-10-16 22:30:07 PDT
(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 2006-10-17 12:33:25 PDT
(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 2006-10-17 12:51:17 PDT
If we're willing to put a hold on this, I can go get clarification.
Comment 14 alexander :surkov 2007-01-28 08:48:05 PST
Created attachment 253098 [details] [diff] [review]
patch2
Comment 15 Olli Pettay [:smaug] 2007-01-29 02:02:37 PST
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 2007-01-29 06:06:20 PST
XHTML testcase using tabindex="0" and tabindex="-1": http://www.mozilla.org/access/dhtml/tree
Comment 17 alexander :surkov 2007-01-29 07:15:31 PST
Created attachment 253173 [details]
xhtml test

I'll file new patch a bit later ;)
Comment 18 alexander :surkov 2007-01-29 07:22:07 PST
Created attachment 253175 [details]
xul test

xul compact select/select1 are not focused properly due to bug 349461.
Comment 19 alexander :surkov 2007-01-29 07:23:34 PST
Created attachment 253176 [details] [diff] [review]
patch3
Comment 20 alexander :surkov 2007-01-29 10:04:38 PST
Created attachment 253195 [details]
xhtml test2
Comment 21 alexander :surkov 2007-01-29 10:05:12 PST
Created attachment 253196 [details]
xul test2
Comment 22 alexander :surkov 2007-01-29 10:06:11 PST
Created attachment 253198 [details] [diff] [review]
patch4

addresed smaug's comments about tabindex for output and label
Comment 23 Olli Pettay [:smaug] 2007-01-29 10:43:17 PST
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 2007-02-09 16:21:44 PST
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 2007-02-13 08:21:23 PST
Created attachment 254949 [details] [diff] [review]
patch5
Comment 26 alexander :surkov 2007-02-13 08:25:13 PST
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 2007-02-13 08:28:19 PST
Created attachment 254952 [details] [diff] [review]
patch6

forgot to add new file

for checkin
Comment 28 alexander :surkov 2007-02-13 08:30:55 PST
Created attachment 254954 [details] [diff] [review]
patch7

Sorry, previous is wrong patch again.

for check in
Comment 29 alexander :surkov 2007-02-13 10:01:58 PST
checked in by smaug
Comment 30 aaronr 2007-04-23 16:07:13 PDT
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16

Note You need to log in before you can comment on or make changes to this bug.