Note: There are a few cases of duplicates in user autocompletion which are being worked on.

provide accessible label for xul

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
11 years ago
a year ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({fixed1.8.0.12, fixed1.8.1.4})

Trunk
fixed1.8.0.12, fixed1.8.1.4
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

3.83 KB, application/vnd.mozilla.xul+xml
Details
12.18 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
We should have xf:label for xul that shows accessekey like there is the one for xhtml.
(Assignee)

Updated

11 years ago
Depends on: 344369
(Assignee)

Updated

11 years ago
Blocks: 367826
(Assignee)

Comment 1

11 years ago
Created attachment 254970 [details] [diff] [review]
patch

offhand patch
Attachment #254970 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

11 years ago
You can find examples at bug 339287.
(Assignee)

Updated

11 years ago
Attachment #254970 - Flags: review?(aaronr)

Comment 3

11 years ago
(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?
(Assignee)

Comment 4

11 years ago
Created attachment 254977 [details]
testcase

sorry, I thought that bug has testcase for XUL.
(Assignee)

Comment 5

11 years ago
(In reply to comment #3)

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

for 1.8 too.

Comment 6

11 years ago
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+
Attachment #254970 - Flags: review?(aaronr) → review+

Comment 7

11 years ago
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.
(Assignee)

Comment 8

11 years ago
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.
No longer depends on: 344369
(Assignee)

Comment 9

11 years ago
(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?

Updated

11 years ago
Attachment #254970 - Flags: review?(Olli.Pettay) → review+

Comment 10

11 years ago
(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.
(Assignee)

Comment 11

11 years ago
Created attachment 255196 [details] [diff] [review]
patch2

for checking in
Attachment #254970 - Attachment is obsolete: true

Comment 12

11 years ago
smaug checked into trunk for surkov today
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Comment 13

10 years ago
checked into 1.8 branch on 2007-04-12
checked into 1.8.0 branch on 2007-04-16
Keywords: fixed1.8.0.12, fixed1.8.1.4
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.