340088 - provide accessible label for xul

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

We should have xf:label for xul that shows accessekey like there is the one for xhtml.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

offhand patch

Comment 2

[alexander :surkov (:asurkov)]

You can find examples at bug 339287.

Comment 3

[aaronr]

(In reply to comment #2)
> You can find examples at bug 339287.
> 

Those examples are xhtml.  Do you have any xul testcases?  Is this patch only for trunk or is it for 1.8, too?

Comment 4

[alexander :surkov (:asurkov)]

Attachment: testcase

sorry, I thought that bug has testcase for XUL.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #3)

> Is this patch only
> for trunk or is it for 1.8, too?
> 

for 1.8 too.

Comment 6

[aaronr]

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


>Index: extensions/xforms/resources/content/xforms.xml
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms.xml,v
>retrieving revision 1.46
>diff -u -8 -p -r1.46 xforms.xml
>--- extensions/xforms/resources/content/xforms.xml	2 Nov 2006 14:50:32 -0000	1.46
>+++ extensions/xforms/resources/content/xforms.xml	13 Feb 2007 18:58:19 -0000

>@@ -377,16 +380,61 @@

>+  <!-- LABEL: <ACCESKEY SUPPORT>
>+    The label widget assumes successors bindings implement getElementControl()
>+    method what returns the object:
>+    {
>+      set value(); // set "string" value
>+    }
>+  -->
>+  <binding id="xformswidget-label-accesskey-base"
>+           extends="#xformswidget-label-base">
>+
>+    <implementation>
>+      <method name="formatAccesskeyOn">
>+        <parameter name="aAccesskey"/>
>+        <parameter name="aTextNode"/>

nit: Patch is fine, but I don't like the name of this function since you aren't really formatting the accesskey.  How about setAccesskey(), setAccessKeyOnLabel(), identifyAccesskey() or specifyAccesskey()?  Maybe formatWithAccesskey()?  That kinda works since you are formatting the label, I guess.

either way, it's not worth holding up a patch because of a method name.  Just something to think about.  r+

Comment 7

[aaronr]

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

>+                // XXX: if label has element node children, we skip accesskey
>+                // underlining.

BTW, does this need to be an XXX?  I don't see anything to do or any follow up that we need to consider.  Please remove the XXX if you don't need it.

Comment 8

[alexander :surkov (:asurkov)]

Unmarking dependency on 344369 because the fix of that bug is not going to be on branch and it fixes only toolkit verision. Later when seamonkey will migrate on toolkit and xforms extension platform will be 1.9 then it's reasonable to use xul:label binding to show an accesskey.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> nit: Patch is fine, but I don't like the name of this function since you aren't
> really formatting the accesskey.  How about setAccesskey(),
> setAccessKeyOnLabel(), identifyAccesskey() or specifyAccesskey()?  Maybe
> formatWithAccesskey()?  That kinda works since you are formatting the label, I
> guess.

formatAccesskey was proposed by xul:label binding. But I'm fine with formatWithAccesskey(). I'll change name in following up patch.


(In reply to comment #7)
> (From update of attachment 254970 [details] [diff] [review])
> >+                // XXX: if label has element node children, we skip accesskey
> >+                // underlining.
> 
> BTW, does this need to be an XXX?  I don't see anything to do or any follow up
> that we need to consider.  Please remove the XXX if you don't need it.
> 

I guess we still need it. I took the XXX section from xf:label binding for XHTML. It's really not clear with me how xf:label should underline accesskey if xf:label contains any nodes. Probably I should to file new bug and refer on it in the XXX section. Does it work for you?

Comment 10

[aaronr]

(In reply to comment #9)
> (In reply to comment #6)
> 
> > nit: Patch is fine, but I don't like the name of this function since you aren't
> > really formatting the accesskey.  How about setAccesskey(),
> > setAccessKeyOnLabel(), identifyAccesskey() or specifyAccesskey()?  Maybe
> > formatWithAccesskey()?  That kinda works since you are formatting the label, I
> > guess.
> 
> formatAccesskey was proposed by xul:label binding. But I'm fine with
> formatWithAccesskey(). I'll change name in following up patch.
> 
> 
> (In reply to comment #7)
> > (From update of attachment 254970 [details] [diff] [review] [details])
> > >+                // XXX: if label has element node children, we skip accesskey
> > >+                // underlining.
> > 
> > BTW, does this need to be an XXX?  I don't see anything to do or any follow up
> > that we need to consider.  Please remove the XXX if you don't need it.
> > 
> 
> I guess we still need it. I took the XXX section from xf:label binding for
> XHTML. It's really not clear with me how xf:label should underline accesskey if
> xf:label contains any nodes. Probably I should to file new bug and refer on it
> in the XXX section. Does it work for you?
> 

I'd change the comment then to say something like: "XXX: How should we handle underlining if label contains non-text nodes?" or something like that.  When I read the comment that is there now, I saw it as just a statement.  I didn't realize that was a question that we still needed to answer.

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch2

for checking in

Comment 12

[aaronr]

smaug checked into trunk for surkov today

Comment 13

[aaronr]

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