Last Comment Bug 340088 - provide accessible label for xul
: provide accessible label for xul
Status: RESOLVED FIXED
: fixed1.8.0.12, fixed1.8.1.4
Product: Core Graveyard
Classification: Graveyard
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: alexander :surkov
: Stephen Pride
:
Mentors:
Depends on:
Blocks: 327236 xformsa11y 367826
  Show dependency treegraph
 
Reported: 2006-06-02 03:56 PDT by alexander :surkov
Modified: 2016-07-15 14:46 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (11.23 KB, patch)
2007-02-13 11:00 PST, alexander :surkov
bugs: review+
aaronr: review+
Details | Diff | Splinter Review
testcase (3.83 KB, application/vnd.mozilla.xul+xml)
2007-02-13 11:42 PST, alexander :surkov
no flags Details
patch2 (12.18 KB, patch)
2007-02-15 01:10 PST, alexander :surkov
no flags Details | Diff | Splinter Review

Description alexander :surkov 2006-06-02 03:56:16 PDT
We should have xf:label for xul that shows accessekey like there is the one for xhtml.
Comment 1 alexander :surkov 2007-02-13 11:00:39 PST
Created attachment 254970 [details] [diff] [review]
patch

offhand patch
Comment 2 alexander :surkov 2007-02-13 11:06:01 PST
You can find examples at bug 339287.
Comment 3 aaronr 2007-02-13 11:26:56 PST
(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 2007-02-13 11:42:00 PST
Created attachment 254977 [details]
testcase

sorry, I thought that bug has testcase for XUL.
Comment 5 alexander :surkov 2007-02-13 11:42:35 PST
(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 2007-02-13 14:42:48 PST
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 2007-02-13 17:46:50 PST
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 2007-02-13 19:41:47 PST
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 2007-02-13 19:48:55 PST
(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 2007-02-14 10:39:31 PST
(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 2007-02-15 01:10:10 PST
Created attachment 255196 [details] [diff] [review]
patch2

for checking in
Comment 12 aaronr 2007-02-15 13:07:51 PST
smaug checked into trunk for surkov today
Comment 13 aaronr 2007-04-23 16:08:18 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.