Last Comment Bug 339287 - support accesskey attribute
: support accesskey attribute
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: alexander :surkov
: Hixie (not reading bugmail)
Mentors:
Depends on: 400774
Blocks: xformsa11y 370487
  Show dependency treegraph
 
Reported: 2006-05-25 19:54 PDT by alexander :surkov
Modified: 2007-10-22 17:48 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
xhtml testcase (4.84 KB, application/xhtml+xml)
2006-10-14 07:34 PDT, alexander :surkov
no flags Details
try (203.18 KB, patch)
2006-12-20 10:15 PST, alexander :surkov
no flags Details | Diff | Review
patch (43.00 KB, patch)
2006-12-20 10:17 PST, alexander :surkov
bugs: review-
Details | Diff | Review
patch2 (56.77 KB, patch)
2007-01-03 10:19 PST, alexander :surkov
bugs: review-
Details | Diff | Review
patch3 (4.58 KB, patch)
2007-01-11 04:29 PST, alexander :surkov
no flags Details | Diff | Review
patch4 (61.38 KB, patch)
2007-01-11 04:58 PST, alexander :surkov
bugs: review-
Details | Diff | Review
patch5 (61.53 KB, patch)
2007-01-16 10:35 PST, alexander :surkov
bugs: review+
mats: superreview-
Details | Diff | Review
Testcase #2, HTML label event issue (1.55 KB, text/html)
2007-01-31 18:00 PST, Mats Palmgren (:mats)
no flags Details
patch6 (62.27 KB, patch)
2007-02-01 04:57 PST, alexander :surkov
mats: superreview+
Details | Diff | Review
patch7 (63.05 KB, patch)
2007-02-02 02:42 PST, alexander :surkov
jst: superreview+
Details | Diff | Review
patch8 (63.14 KB, patch)
2007-02-12 16:58 PST, alexander :surkov
no flags Details | Diff | Review

Description alexander :surkov 2006-05-25 19:54:40 PDT
At least two controls doesn't support @accesskey attribute. Thease are <input appearance="full" type="xsd:date"/> and <output appearance="full" type="xsd:date"/>.
Comment 1 alexander :surkov 2006-10-14 07:28:50 PDT
I think we should get rid all inheritances of accesskey attribute by underlying widgets. Almost always this approach works but I guess it's wrong since document where xforms elements are hosted has two and the same accesskeys. One accesskey attribue is on xforms elmenent, other accesskey attribute is on underlying control.

It looks more right to register directly accesskeys via nsEventStateManager. Here I can see two ways:
1) XForms controls behave like it do analogues controls from host document. This looks reasonable and nsEventStateManger should works fine since it knows about xul and xhtml. But some xforms controls hasn't underlying controls of host document (like select appearance='full') and nsEventStateManager will unable to focus xforms controls.
2) XForms controls behave in the same manner independent of sort of host document. Imo that looks not bad too. But here we should let to know to nsEventStateManger how to focus xforms controls.

What do you, guys, think about it?
Comment 2 alexander :surkov 2006-10-14 07:34:45 PDT
Created attachment 242271 [details]
xhtml testcase

The controls those accesskey isn't work for:

input[type="xsd:date"][appearance="full"] - control hasn't unique underlying widget
trigger[appearance="minimal"] - due to bug 356660
select/select1[appearance="full"] - control hasn't unique underlying widget
range - I think it's our bad :).
Comment 3 alexander :surkov 2006-10-16 00:46:14 PDT
Talked with smaug on irc, we figured out nice way (imo) how to change nsEventStateMangager so that xforms can use it. The idea is to move part of nsEventStateManager::handleAccesskey() where focusing of element is happen to nsIContent implementations. So that nsGenericElement/nsXULElement/nsIXTFElementWrapper should implement that method.

The question about approach how we will handle accesskeys for xforms is steal open.

1) Try to redirect accesskey handling to native widget.
2) Call methods of nsIXFormsUIWidget, for example, focus().
Comment 4 Aaron Leventhal 2006-10-16 04:24:23 PDT
Please note that bug 143065 modifies HandleAccessKey quite a bit, and it's basically done just waiting for final touches. Might want to help push that through before messing around there more.
Comment 5 aaronr 2006-10-16 18:23:59 PDT
(In reply to comment #1)
> I think we should get rid all inheritances of accesskey attribute by underlying
> widgets. Almost always this approach works but I guess it's wrong since
> document where xforms elements are hosted has two and the same accesskeys. One
> accesskey attribue is on xforms elmenent, other accesskey attribute is on
> underlying control.
> 
> It looks more right to register directly accesskeys via nsEventStateManager.
> Here I can see two ways:
> 1) XForms controls behave like it do analogues controls from host document.
> This looks reasonable and nsEventStateManger should works fine since it knows
> about xul and xhtml. But some xforms controls hasn't underlying controls of
> host document (like select appearance='full') and nsEventStateManager will
> unable to focus xforms controls.
> 2) XForms controls behave in the same manner independent of sort of host
> document. Imo that looks not bad too. But here we should let to know to
> nsEventStateManger how to focus xforms controls.
> 
> What do you, guys, think about it?
> 


my problem with changing this would be that this would be trunk only.  We'd still have to support the old way on 1.8.1 and 1.8.0.  If we can keep the xforms changes as small and contained as possible, I have no problem with it.
Comment 6 alexander :surkov 2006-10-16 22:37:14 PDT
(In reply to comment #5)

> my problem with changing this would be that this would be trunk only.  We'd
> still have to support the old way on 1.8.1 and 1.8.0.  If we can keep the
> xforms changes as small and contained as possible, I have no problem with it.
> 

Right, it's only for trunk. I think the changes will be the next. In XBL part it is removing only xbl:inherits="accesskey" lines. And plus some special code in XTF part (probably additional method handleAccesskey() of nsXFormsStubElement). To be more precisely I should wait for bug 143065 and bug 356660.
Comment 7 alexander :surkov 2006-12-20 10:15:24 PST
Created attachment 249267 [details] [diff] [review]
try

1) added performAccesskey for nsIContent
2) nsGenericHTMLElement/nsHTMLLabelElement/nsXULElement/nsXTFElementWrapper implement it
3) nsHTMLLabelElement uses the logic proposed in patch4 of bug 222297.
4) nsXTFElementWrapper redirects call to nsXTFElement.
5) xforms xtf element implements the method
Comment 8 alexander :surkov 2006-12-20 10:16:12 PST
Comment on attachment 249267 [details] [diff] [review]
try

wrong patch
Comment 9 alexander :surkov 2006-12-20 10:17:50 PST
Created attachment 249268 [details] [diff] [review]
patch
Comment 10 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-01-02 02:38:22 PST
Comment on attachment 249268 [details] [diff] [review]
patch


>-          // Propagate trusted state to the new event.
>-          nsEventStatus status = nsEventStatus_eIgnore;
>-          nsMouseEvent event(NS_IS_TRUSTED_EVENT(aEvent), NS_MOUSE_CLICK,
>-                             nsnull, nsMouseEvent::eReal);
>-
>-          nsAutoPopupStatePusher popupStatePusher(NS_IS_TRUSTED_EVENT(aEvent) ?
>-                                                  openAllowed : openAbused);
>-

Here NS_IS_TRUSTED_EVENT(aEvent) is used, but you change that to



>+nsGenericHTMLElement::PerformAccesskey(PRBool aKeyCausesActivation)
....
>+    // Click on it if the users prefs indicate to do so.
>+    nsEventStatus status = nsEventStatus_eIgnore;
>+    nsMouseEvent event(nsContentUtils::IsCallerChrome(), NS_MOUSE_CLICK,
>+                       nsnull, nsMouseEvent::eReal);
>+
>+    nsAutoPopupStatePusher popupStatePusher(nsContentUtils::IsCallerChrome() ?
>+                                            openAllowed : openAbused);

nsContentUtils::IsCallerChrome(). Why that change.

+
+    nsEventDispatcher::Dispatch(content, presContext, &event, nsnull, &status);

You don't actually need the |status|. (I know it is currently there in ESM.)




>+void
>+nsXULElement::PerformAccesskey(PRBool aKeyCausesActivation)
>+{
>+  nsCOMPtr<nsIContent> content(this);

Why do you need |content| ?
Comment 11 alexander :surkov 2007-01-03 10:19:18 PST
Created attachment 250358 [details] [diff] [review]
patch2
Comment 12 alexander :surkov 2007-01-03 10:21:25 PST
(In reply to comment #10)

> >+void
> >+nsXULElement::PerformAccesskey(PRBool aKeyCausesActivation)
> >+{
> >+  nsCOMPtr<nsIContent> content(this);
> 
> Why do you need |content| ?
> 

The content is either this or control that label points to. It is used in code below. Or do you mean something else?
Comment 13 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-01-08 02:11:57 PST
Comment on attachment 250358 [details] [diff] [review]
patch2

> 
>+void
>+nsXULElement::PerformAccesskey(PRBool aKeyCausesActivation,
>+                               PRBool aIsTrustedEvent)
>+{
>+  nsCOMPtr<nsIContent> content(this);
>+
>+  if (Tag() == nsGkAtoms::label) {
>+    nsCOMPtr<nsIDOMElement> element;
>+
>+    nsAutoString control;
>+    GetAttr(kNameSpaceID_None, nsGkAtoms::control, control);
>+    if (!control.IsEmpty()) {
>+      nsCOMPtr<nsIDOMDocument> domDocument =
>+        do_QueryInterface(content->GetDocument());

GetDocument() is deprecated. Use GetCurrentDoc() ?

XForms part looks ok to me.

Question, do we want to hardcode it to XTF that the attribute name
for access key is really |accesskey| in every case? Is it possible that
some xml language uses some other attribute name?
Comment 14 alexander :surkov 2007-01-08 02:27:00 PST
(In reply to comment #13)

> Question, do we want to hardcode it to XTF that the attribute name
> for access key is really |accesskey| in every case? Is it possible that
> some xml language uses some other attribute name?
> 

It's interesting idea though we haven't usecases for this I guess. But I think it's easy enough to support this. Do we need this?
Comment 15 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-01-08 08:51:28 PST
Comment on attachment 250358 [details] [diff] [review]
patch2


>+void
>+nsXULElement::PerformAccesskey(PRBool aKeyCausesActivation,
>+                               PRBool aIsTrustedEvent)
>+{
>+  nsCOMPtr<nsIContent> content(this);
>+
>+  if (Tag() == nsGkAtoms::label) {
>+    nsCOMPtr<nsIDOMElement> element;
>+
>+    nsAutoString control;
>+    GetAttr(kNameSpaceID_None, nsGkAtoms::control, control);
>+    if (!control.IsEmpty()) {

Maybe
if (GetAttr(kNameSpaceID_None, nsGkAtoms::control, control))

>+      nsCOMPtr<nsIDOMDocument> domDocument =
>+        do_QueryInterface(content->GetDocument());
>+      if (domDocument)
>+        domDocument->GetElementById(control, getter_AddRefs(element));
>+    }
>+    // ... that here we'll either change |content| to the element
>+    // referenced by |element|, or clear it.
>+    content = do_QueryInterface(element);
>+  }

What if content is null? You're removing a null check which is there
currently in ESM.

I'd make it possible to define access key attribute name, just to be consistent with
class attribute name.
Comment 16 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-01-08 08:54:52 PST
(In reply to comment #15)

> What if content is null? You're removing a null check which is there
> currently in ESM.
> 

nm, GetPrimaryFrameFor is null safe.
Comment 17 Mats Palmgren (:mats) 2007-01-08 10:18:13 PST
(In reply to comment #16)
> nm, GetPrimaryFrameFor is null safe.

Yes, but you shouldn't call it with null though, it will trigger a NS_WARNING.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsFrameManager.cpp&rev=1.241&root=/cvsroot&mark=344#340
Comment 18 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-01-09 12:47:26 PST
Comment on attachment 250358 [details] [diff] [review]
patch2

r- until comments addressed
Comment 19 alexander :surkov 2007-01-11 04:29:08 PST
Created attachment 251169 [details] [diff] [review]
patch3
Comment 20 alexander :surkov 2007-01-11 04:58:18 PST
Created attachment 251171 [details] [diff] [review]
patch4

fixed accesskey handling for select1/select
label of range with accesskey shows accesskey
Comment 21 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-01-15 11:03:10 PST
Comment on attachment 251171 [details] [diff] [review]
patch4


>+  /**
>+   * The method focues (or activates) element that accesskey is bound to. It is
>+   * called when accesskey was activated.

"Focuses",
"when accesskey is activated", maybe?


>+   *
>+   * @param aKeyCausesActivation - if true then HTML element should be activated

HTML element?

>@@ -424,16 +444,24 @@ nsXTFElementWrapper::UnsetAttr(PRInt32 a
>   else { // try wrapper
>     rv = nsXTFElementWrapperBase::UnsetAttr(aNameSpaceID, aAttr, aNotify);
>   }
> 
>   if (aNameSpaceID == kNameSpaceID_None &&
>       (mNotificationMask & nsIXTFElement::NOTIFY_ATTRIBUTE_REMOVED))
>     GetXTFElement()->AttributeRemoved(aAttr);
> 
>+  if (mNotificationMask & nsIXTFElement::NOTIFY_PERFORM_ACCESSKEY) {
>+    nsCOMPtr<nsIDOMAttr> accesskey;
>+    GetXTFElement()->GetAccesskeyNode(getter_AddRefs(accesskey));
>+    nsCOMPtr<nsIAttribute> attr(do_QueryInterface(accesskey));
>+    if (attr && attr->NodeInfo()->Equals(aAttr, aNameSpaceID))
>+      RegUnregAccessKey(PR_TRUE);
>+  }
>+

Shouldn't this be before the attribute is actually removed from element,
I mean before nsXTFElementWrapperBase::UnsetAttr call.

> 
>+void
>+nsXULElement::PerformAccesskey(PRBool aKeyCausesActivation,
>+                               PRBool aIsTrustedEvent)
>+{
>+  nsCOMPtr<nsIContent> content(this);
>+
>+  if (Tag() == nsGkAtoms::label) {
>+    nsCOMPtr<nsIDOMElement> element;
>+
>+    nsAutoString control;
>+    GetAttr(kNameSpaceID_None, nsGkAtoms::control, control);
>+    if (!control.IsEmpty()) {
>+      nsCOMPtr<nsIDOMDocument> domDocument =
>+        do_QueryInterface(content->GetCurrentDoc());
>+      if (domDocument)
>+        domDocument->GetElementById(control, getter_AddRefs(element));
>+    }
>+    // ... that here we'll either change |content| to the element
>+    // referenced by |element|, or clear it.

Why you kept this part of the comment but not "// If anything fails, this will be null ..." .
Not that the comment is too good in any case. Maybe you could remove the  "...that" comment.

In nsXULElement indentation is 4 spaces.
Comment 22 alexander :surkov 2007-01-16 10:35:10 PST
Created attachment 251657 [details] [diff] [review]
patch5
Comment 23 alexander :surkov 2007-01-16 10:37:20 PST
(In reply to comment #21)
> 
> In nsXULElement indentation is 4 spaces.
> 

I'd like to save 2 spaces because some methods of nsXULElement has 2 spaces and mozilla style assume 2 spaces too.
Comment 24 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2007-01-17 02:13:47 PST
Comment on attachment 251657 [details] [diff] [review]
patch5


>+  /**
>+   * The method focuses (or activates) element that accesskey is bound to. It is
>+   * called when accesskey is activated.
>+   *
>+   * @param aKeyCausesActivation - if true then element should be activated
>+   * @param aIsTrustedEvent - if true then event that is cause of accesskey
>+   *                          execution is trusted.
>+   */


The comment is not actually quite right. Maybe something like
"This method is called when the accesskey bound to an element is activated."

Because nsXULElement is marked to use 4 spaces, better to stick with that.
Comment 25 alexander :surkov 2007-01-17 02:26:17 PST
Comment on attachment 251657 [details] [diff] [review]
patch5

Mats, I'd like to fix smaug's nits after your comments.
Comment 26 Mats Palmgren (:mats) 2007-01-31 17:59:03 PST
Comment on attachment 251657 [details] [diff] [review]
patch5

> Index: content/html/content/src/nsGenericHTMLElement.cpp
> +  // It's hard to say what HTML4 wants us to do in all cases.
> +  esm->ChangeFocusWith(NS_STATIC_CAST(nsIContent*, this),
> +                       nsIEventStateManager::eEventFocusedByKey);

Why NS_STATIC_CAST? can't you just use 'this' as is?
Also, the comment as it stands doesn't add any value IMO. We might
as well just remove it.

> +    nsEventStatus status = nsEventStatus_eIgnore;
> +    nsEventDispatcher::Dispatch(NS_STATIC_CAST(nsIContent*, this), ...

'status' is not used
NS_STATIC_CAST again

> Index: content/html/content/src/nsHTMLLabelElement.cpp
> +void
> +nsHTMLLabelElement::PerformAccesskey(PRBool aKeyCausesActivation,
> +                                     PRBool aIsTrustedEvent)

This makes me worried. See the testcase I will attach -
you have lost the "LABEL:click" event.
Would it be possible to keep the HTML label event handling as is?

There is a GetForContent() method to find the 'for'-target...

> Index: content/xul/content/src/nsXULElement.cpp
> +  const nsStyleVisibility* vis = frame->GetStyleVisibility();
> +  PRBool viewShown = frame->AreAncestorViewsVisible();
> +
> +  if (!viewShown || vis->mVisible == NS_STYLE_VISIBILITY_COLLAPSE ||
> +      vis->mVisible == NS_STYLE_VISIBILITY_HIDDEN)

I would prefer:
   if (vis->mVisible == NS_STYLE_VISIBILITY_COLLAPSE ||
       vis->mVisible == NS_STYLE_VISIBILITY_HIDDEN ||
       !frame->AreAncestorViewsVisible())

> Index: extensions/xforms/resources/content/input-xhtml.xml
>    <binding id="xformswidget-input-boolean"
> ...
> -      <html:input anonid="control" xbl:inherits="accesskey" type="checkbox"
> +      <html:input anonid="control" type="checkbox"
>                    class="xf-value"/>

Will now fit on one line.

Marking sr- for now, until the HTML label event issue is resolved.
Comment 27 Mats Palmgren (:mats) 2007-01-31 18:00:37 PST
Created attachment 253564 [details]
Testcase #2, HTML label event issue
Comment 28 alexander :surkov 2007-02-01 01:47:29 PST
(In reply to comment #26)

> > Index: content/html/content/src/nsHTMLLabelElement.cpp
> > +void
> > +nsHTMLLabelElement::PerformAccesskey(PRBool aKeyCausesActivation,
> > +                                     PRBool aIsTrustedEvent)
> 
> This makes me worried. See the testcase I will attach -
> you have lost the "LABEL:click" event.
> Would it be possible to keep the HTML label event handling as is?

The label logic you see I took from the patch4 of bug 222297. dbaron said there
"This is just yet another workaround for the real problem I described in comment
9.  I'd rather not spread label logic all over the tree if we don't have to. 
What I described in comment 9 shouldn't be very hard to fix."

Here I keep label logic inside label code.

I made this change because current code has trick to substitute for nsEventStateManager::mCurrentTargetContent during 'click' event dispatching. Probably it doesn't make sense now for current events implementation. But I'm not sure. I will be very appritiate if Olli will give some clarification. Since I move accesskey handling into label code then I can't access for mCurrentTargetContent from there by nice way. So, what the way is preferable?
Comment 29 alexander :surkov 2007-02-01 02:01:48 PST
(In reply to comment #26)
> (From update of attachment 251657 [details] [diff] [review])
> > Index: content/html/content/src/nsGenericHTMLElement.cpp
> > +  // It's hard to say what HTML4 wants us to do in all cases.
> > +  esm->ChangeFocusWith(NS_STATIC_CAST(nsIContent*, this),
> > +                       nsIEventStateManager::eEventFocusedByKey);
> 
> Why NS_STATIC_CAST? can't you just use 'this' as is?
> Also, the comment as it stands doesn't add any value IMO. We might
> as well just remove it.

STATIC_CAST is just explicit convertion that is often used in mozilla code.

"A pointer to member type can be explicitly converted into a different pointer to member type if both types are pointers to members of the same class. This form of explicit conversion may also take place if the pointer to member types are from separate classes, however one of the class types must be derived from the other."

Should I try to remove it? I think it may involve warnings or even errors though I didn't test it yet.
Comment 30 alexander :surkov 2007-02-01 04:56:16 PST
(In reply to comment #26)
> (From update of attachment 251657 [details] [diff] [review])
> > Index: content/html/content/src/nsGenericHTMLElement.cpp
> > +  // It's hard to say what HTML4 wants us to do in all cases.
> > +  esm->ChangeFocusWith(NS_STATIC_CAST(nsIContent*, this),
> > +                       nsIEventStateManager::eEventFocusedByKey);
> 
> Why NS_STATIC_CAST? can't you just use 'this' as is?
> Also, the comment as it stands doesn't add any value IMO. We might
> as well just remove it.

I remove NS_STATIC_CAST in nsGenericHTMLElement but I can't do it for nsHTMLLabelElement because I got an error "ambiguous conversions from 'nsHTMLLabelElement *const ' to 'nsISupports *'"

> > +    nsEventStatus status = nsEventStatus_eIgnore;
> > +    nsEventDispatcher::Dispatch(NS_STATIC_CAST(nsIContent*, this), ...
> 
> 'status' is not used
> NS_STATIC_CAST again

Fixed.

> > Index: content/html/content/src/nsHTMLLabelElement.cpp
> > +void
> > +nsHTMLLabelElement::PerformAccesskey(PRBool aKeyCausesActivation,
> > +                                     PRBool aIsTrustedEvent)
> 
> This makes me worried. See the testcase I will attach -
> you have lost the "LABEL:click" event.
> Would it be possible to keep the HTML label event handling as is?

Fixed
Comment 31 alexander :surkov 2007-02-01 04:57:06 PST
Created attachment 253602 [details] [diff] [review]
patch6
Comment 32 Mats Palmgren (:mats) 2007-02-01 21:05:19 PST
Comment on attachment 253602 [details] [diff] [review]
patch6

Looks good so far.  I found one more issue while (briefly) testing some
XForms elements and saw that accesskey for <xf:label> did not work. 
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/xforms/nsXFormsLabelElement.cpp&rev=1.19&root=/cvsroot&mark=110,113-116#107
(the NOTIFY_PERFORM_ACCESSKEY that nsXFormsDelegateStub added is lost)
Could you look through the various XForms classes and add the bit where
needed please. With that sr=mats.
Comment 33 Mats Palmgren (:mats) 2007-02-01 21:06:57 PST
It might also be worth filing a bug on making a better interface for
subclasses to add/remove bits to the NotificationMask so the wrapper
can evolve without having to update every wrapped class...  e.g.
ModifyNotificationMask(PRUint32 bits, PRBool addOrRemove)
Comment 34 alexander :surkov 2007-02-02 01:55:39 PST
(In reply to comment #33)
> It might also be worth filing a bug on making a better interface for
> subclasses to add/remove bits to the NotificationMask so the wrapper
> can evolve without having to update every wrapped class...  e.g.
> ModifyNotificationMask(PRUint32 bits, PRBool addOrRemove)
> 

Good idea, I filed bug 369069 to address it.
Comment 35 alexander :surkov 2007-02-02 02:42:08 PST
Created attachment 253742 [details] [diff] [review]
patch7
Comment 36 alexander :surkov 2007-02-02 02:44:53 PST
(In reply to comment #32)
> (From update of attachment 253602 [details] [diff] [review])
> Looks good so far.  I found one more issue while (briefly) testing some
> XForms elements and saw that accesskey for <xf:label> did not work. 
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/extensions/xforms/nsXFormsLabelElement.cpp&rev=1.19&root=/cvsroot&mark=110,113-116#107
> (the NOTIFY_PERFORM_ACCESSKEY that nsXFormsDelegateStub added is lost)
> Could you look through the various XForms classes and add the bit where
> needed please. With that sr=mats.
> 

Thank you, good catch. Fixed. The other XForms UI controls looks working.
Comment 37 alexander :surkov 2007-02-02 07:37:35 PST
Comment on attachment 253742 [details] [diff] [review]
patch7

As Olli proposed Im asking one more sr, since mats is not listed at http://www.mozilla.org/hacking/reviewers.html
Comment 38 Johnny Stenback (:jst, jst@mozilla.com) 2007-02-07 16:06:02 PST
Comment on attachment 253742 [details] [diff] [review]
patch7

- In nsXULElement::PerformAccesskey():

+    if (Tag() == nsGkAtoms::label) {
+        nsCOMPtr<nsIDOMElement> element;
+
+      nsAutoString control;
+      GetAttr(kNameSpaceID_None, nsGkAtoms::control, control);
...

Clean up the indentation here please (stick to 4-space indentation like the surrounding code does).

sr=jst
Comment 39 alexander :surkov 2007-02-12 16:58:33 PST
Created attachment 254885 [details] [diff] [review]
patch8

needs to be checked in
Comment 40 alexander :surkov 2007-02-13 07:26:31 PST
checked in by smaug, no 'xf-to-branch' whiteboard status, patch is for trunk only.

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