409604 - Accesskey on HTML label, area and legend elements don't work

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Description

[luihm]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2

With markup like <label accesskey="q">Test: <select><option>a</option></select></label>, Firefox 3 Beta 2 won't focus on the select box when the accesskey is pressed. However, it did in Firefox 2.

Reproducible: Always

Steps to Reproduce:
1. Go to feedhouse.mozillazine.org
2. Press Shift + Alt + s. (This is the accesskey to the search box near the top right of the page.)

Actual Results:  
In this case, the History menu is activated (Alt + s). In cases where there is no menu with that shortcut, nothing happens.

Expected Results:  
The search box should receive the focus.

Comment 1

[(no longer active)]

Confirming.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2007122820 Minefield/3.0b3pre

Comment 2

[:Gavin Sharp [email: [e-mail]]]

I can reproduce on the Mac using Ctrl+S (ui.key.contentAccess is set to 2).

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007122804 Minefield/3.0b3pre

Comment 3

[Simon Bünzli]

Works with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112605 Minefield/3.0b2pre

Broken with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007113005 Minefield/3.0b2pre

Thus most probably a regression of bug 143065.

Comment 4

[Damon Sicore (:damons)]

+'ing this with P3 priority. 

Comment 6

[(no longer active)]

Attachment: Patch (v1)

First attempt:  The problem is in the IsAccessKeyTarget() function.  This function tests for the element being focusable, which does not cover the whole range of HTML elements for which access keys should be supported (that is, A, AREA, BUTTON, INPUT, LABEL, LEGEND, and TEXTAREA <http://www.w3.org/TR/REC-html40/interact/forms.html#access-keys>).

This patch changes the behavior of that function for HTML content to check explicitly for one of those elements.  The behavior remains unchanged for XUL elements.

Comment 7

[(no longer active)]

Attachment: Unit test

I wrote a unit test to make sure that this doesn't regress in the future.  The test case fully passes when using a build with the patch in attachment 302307 [details] [diff] [review] applied.

Comment 8

[(no longer active)]

This can be tested automatically, so I'm removing in-litmus? and asking in-testsuite.  This can be +ed when attachment 302367 [details] [diff] [review] is checked in.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  const PRBool isHTML = aContent->IsNodeOfType(nsINode::eHTML);
+  if (isHTML) {

Just "if (aContent->IsNodeOfType(...))"

+    nsIAtom* tag = aContent->Tag();
+
+    if (tag != nsGkAtoms::a &&
+        tag != nsGkAtoms::area &&
+        tag != nsGkAtoms::button &&
+        tag != nsGkAtoms::input &&
+        tag != nsGkAtoms::label &&
+        tag != nsGkAtoms::legend &&
+        tag != nsGkAtoms::textarea)
+      return PR_FALSE;
+  } else if (aFrame->IsFocusable())

Don't do else after return. The else stuff can just stand alone.

Other than those nits, this is great!

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For bonus points, reference the spec in a comment.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 302367 [details] [diff] [review]
Unit test

Lovely! For bonus points you could test some elements that should NOT support access keys.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> +  const PRBool isHTML = aContent->IsNodeOfType(nsINode::eHTML);
> +  if (isHTML) {
> 
> Just "if (aContent->IsNodeOfType(...))"
> 
> +    nsIAtom* tag = aContent->Tag();
> +
> +    if (tag != nsGkAtoms::a &&
> +        tag != nsGkAtoms::area &&
> +        tag != nsGkAtoms::button &&
> +        tag != nsGkAtoms::input &&
> +        tag != nsGkAtoms::label &&
> +        tag != nsGkAtoms::legend &&
> +        tag != nsGkAtoms::textarea)
> +      return PR_FALSE;
> +  } else if (aFrame->IsFocusable())
> 
> Don't do else after return. The else stuff can just stand alone.
> 
> Other than those nits, this is great!
> 

Looking at these checks I can assume that xul:label shouldn't work as well as some accesskey on xforms. Am I wrong?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, he's not changing behaviour for non-HTML elements.

Comment 14

[(no longer active)]

(In reply to comment #9)
> +  const PRBool isHTML = aContent->IsNodeOfType(nsINode::eHTML);
> +  if (isHTML) {
> 
> Just "if (aContent->IsNodeOfType(...))"

Well, I use the isHTML variable at the end of the function as well.  The previous behavior was to return false if all the checks in the function fail.  For HTML elements, we need to return true if all the checks in the function fails.  Therefore, I changed the last return statement to |return isHTML;|.  If I address your nit above, I need to call nsIContent::IsNodeOfType() once again at the end of the function, and I don't see why that would be better than the current code.  Right?

I'll address the other nit and the bonus point in a future version of the patch, as soon as I get your input on the above issue.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> Yes, he's not changing behaviour for non-HTML elements.
> 

I mean I'm not sure whether we should check IsFocusable() at all and I'm not fun to check tag names there as well because we spread logic through files, instead I'd like more not to register accesskey if element can't handle accesskey.

Comment 16

[(no longer active)]

(In reply to comment #15)
> (In reply to comment #13)
> > Yes, he's not changing behaviour for non-HTML elements.
> > 
> 
> I mean I'm not sure whether we should check IsFocusable() at all and I'm not
> fun to check tag names there as well because we spread logic through files,
> instead I'd like more not to register accesskey if element can't handle
> accesskey.

There are two aspects of an element which determine whether it can handle access keys or not.  The first one is the element's nature (for example, if it's an HTML area or div element), and the second one is its current state (for example, if it's hidden or not, or if it can get the focus (e.g., it's not a disabled XUL element) or not.  The IsAccessKeyTarget() function seems more appropriate for the latter class of checks.  So, I agree that maybe it's better to move the checks in my patch to the access key registration code.  But we can't dismiss the check for being focusable, I guess.

For the other part of your comment, the only way to handle that (which I see) is adding a virtual method to nsIContent, such as CanHandleAccessKeys() and call it either at registration or invocation time, and return false from that method for all HTML elements but those which can handle access keys, and return true for all non-HTML content.

Let's wait to hear from Robert on his viewpoint on the implementation approach.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd actually like to get rid of access key registration altogether and just walk the content tree every time. See discussions in bug 143065. But that's not a good idea for Gecko 1.9. I'd welcome a patch for that after Gecko 1.9. :-)

So the problem in this bug is basically just that HTML <label>s are not focusable so these tests fail? Maybe the correct fix is in nsEventStateManager, where we call IsAccessKeyTarget, the code that gets the frame should for <label>s get the frame of the label's target content instead. After all, we don't want to choose the label as the access key target if its target control is not focusable.

The best way to do that is probably to move nsHTMLLabelElement::GetFirstFormControl and nsHTMLLabelElement::GetForContent to nsContentUtils, say nsContentUtils::GetAccessKeyTargetContent. That will have to use do_QueryInterface to QI to nsIDOMHTMLLabelElement to call GetHtmlFor; if the QI fails, i.e. the content is not a label, that content itself would be returned as the target content.

Does that make sense?

Comment 18

[(no longer active)]

Attachment: Test case (v2)

(In reply to comment #11)
> (From update of attachment 302367 [details] [diff] [review])
> Lovely! For bonus points you could test some elements that should NOT support
> access keys.

Done.  This new test case also tests all of the HTML elements except those seven elements which support access keys.  The only elements this patch leaves out is those which can't be tested for an onfocus and onclick event, such as <script> or <style> tags.  Because this test handles the non-supported elements dynamically, it should be quite easy to extend to support more elements if necessary.

This test passes with the patch in attachment 302307 [details] [diff] [review] applied.

Comment 19

[alexander :surkov (:asurkov)]

Here I can see three points:

1) html label (and I believe xul label and xforms label as well ) doesn't work
due to they aren't focusable

2) all xforms controls are implemented by XBL bindings plus CSS display
property (like inline, -moz-box and so on) and I think their frames aren't
focusable

3) html div and span elements are material for custom widgets of DHTML. If html
author makes them focusable and add some semantics to them then I guess he may
want to use accesskeys on them as well.

How we can deal with these items? Though I would like to get some comments from
Aaron or Marco for third item.

Comment 20

[(no longer active)]

(In reply to comment #19)
> Here I can see three points:
> 
> 1) html label (and I believe xul label and xforms label as well ) doesn't work
> due to they aren't focusable

I'm not sure about XForms label, but XUL labels' access keys work perfectly: in Firefox, go to the Preferences dialog, the Main panel, and try Alt+H (the access key for "Home page" which is defined on a label with a control attribute.

> 2) all xforms controls are implemented by XBL bindings plus CSS display
> property (like inline, -moz-box and so on) and I think their frames aren't
> focusable

Actually, XForms are implemented via XTF, and XTF was supported in bug 143065.  However, I'm not sure if XForms labels work correctly or not.

> 3) html div and span elements are material for custom widgets of DHTML. If html
> author makes them focusable and add some semantics to them then I guess he may
> want to use accesskeys on them as well.

How can HTML authors make such elements focusable?  At any rate, the HTML4 spec clearly mentions that access keys are not supported on those elements, so I don't think we should worry about them.  <http://www.w3.org/TR/REC-html40/interact/forms.html#access-keys>

Comment 21

[(no longer active)]

(In reply to comment #17)
> So the problem in this bug is basically just that HTML <label>s are not
> focusable so these tests fail?

Actually, the same problem occurs with HTML <legend> element as well, because it's not focusable either.  Not that I have personally ever used that element in real life.  :P

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, still, I think the right approach is to find the frame for the content that will be focused when we press the access key, instead of the frame for the content that has the access key on it.

Comment 23

[(no longer active)]

I agree; it seems more logical.

How can I access the frame for the <legend> HTML elements?  I'm going to post an updated patch implementing this strategy, which also passes the test case I posted.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See the codee for nsHTMLLegendElement::SetFocus. I think you add that code to the new nsContentUtils helper I was talking about, and then you can call that helper from nsHTMLLegendElement::SetFocus.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> (In reply to comment #19)
> > Here I can see three points:
> > 
> > 1) html label (and I believe xul label and xforms label as well ) doesn't work
> > due to they aren't focusable
> 
> I'm not sure about XForms label, but XUL labels' access keys work perfectly: in
> Firefox, go to the Preferences dialog, the Main panel, and try Alt+H (the
> access key for "Home page" which is defined on a label with a control
> attribute.

It's interesting to know implementation of xul:label acceskey.

> > 2) all xforms controls are implemented by XBL bindings plus CSS display
> > property (like inline, -moz-box and so on) and I think their frames aren't
> > focusable
> 
> Actually, XForms are implemented via XTF, and XTF was supported in bug 143065. 
> However, I'm not sure if XForms labels work correctly or not.

I'll build firefox with enabled xforms and check. Possibly we shouldn't check focusable for them in the meantime.

> > 3) html div and span elements are material for custom widgets of DHTML. If html
> > author makes them focusable and add some semantics to them then I guess he may
> > want to use accesskeys on them as well.
> 
> How can HTML authors make such elements focusable? 

@tabindex

> At any rate, the HTML4 spec
> clearly mentions that access keys are not supported on those elements, so I
> don't think we should worry about them. 
> <http://www.w3.org/TR/REC-html40/interact/forms.html#access-keys>
> 

It sounds a major problem for such custom widgets but even WAI ARIA doesn't address it which extends HTML by someway. I'll try to know what's here.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)

> > > 2) all xforms controls are implemented by XBL bindings plus CSS display
> > > property (like inline, -moz-box and so on) and I think their frames aren't
> > > focusable
> > 
> > Actually, XForms are implemented via XTF, and XTF was supported in bug 143065. 
> > However, I'm not sure if XForms labels work correctly or not.
> 
> I'll build firefox with enabled xforms and check. Possibly we shouldn't check
> focusable for them in the meantime.

they definitely don't work. I'll check wether removing of isFocusable check helps here.

Comment 27

[(no longer active)]

Perhaps the XTF/XForms related issues should be handled in a separate bug?  Would you mind filing one and CCing me?

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #27)
> Perhaps the XTF/XForms related issues should be handled in a separate bug? 
> Would you mind filing one and CCing me?
> 

Let me do some investigations. Probably the fix will be very easy. But if it's not and you would prefer to deal with it in separate bug then I'll file of course.

Comment 29

[(no longer active)]

(In reply to comment #22)
> OK, still, I think the right approach is to find the frame for the content that
> will be focused when we press the access key, instead of the frame for the
> content that has the access key on it.

roc: are we interested in following the HTML spec here?  If yes, then we should definitely check for those seven elements which are capable of accepting an access key.  The approach you suggest should be applied after checking for those elements as well, because we don't want to accept an accesskey for a disabled button, for example.  Am I correct here?

Just out of curiosity, I tried a simple test based on comment 25 with a <div> that I artificially made focusable.  It seems like the <div>'s frame is not still focusable somehow, because the accesskey didn't work on it in the latest nightly.  Here's the test:

<input type="text" tabindex="1" accesskey="i">
<div onfocus="alert('focused!');" accesskey="d" tabindex="2">contents</div>

So, the question is, for which HTML elements does nsIFrame::IsFocusable() return true?

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Only those 7 elements should be registering access keys. For example, DIVs should not be registering access keys. Comment #15 was right. So we shouldn't have to check the element type again here. That's why your DIV testcase doesn't work. Calling IsFocusable on its frame actually will return true, I think.

Comment 32

[Marco Zehe (:MarcoZ)]

(In reply to comment #20)
> > 3) html div and span elements are material for custom widgets of DHTML. If html
> > author makes them focusable and add some semantics to them then I guess he may
> > want to use accesskeys on them as well.
> How can HTML authors make such elements focusable?  At any rate, the HTML4 spec
> clearly mentions that access keys are not supported on those elements, so I
> don't think we should worry about them. 
> <http://www.w3.org/TR/REC-html40/interact/forms.html#access-keys>

If the author implements ARIA (accessible rich internet applications), he can give ann html:div element a role of "link", for example, and give it a tabindex attribute. At that moment, it becomes keyboard accessible, and it is conceivable to also focus such elements with access keys. Aaron, what's your take on this?

Comment 33

[(no longer active)]

While working on a new patch, I found out that this problem happens for area elements as well.  Editing the summary to reflect label, legend and area elements.  A new patch is coming soon.

Comment 34

[(no longer active)]

Attachment: Patch (v2)

This new patch implements roc's suggestions in comment 17.  nsContentUtils::GetAccessKeyTargetContent() checks to see if the content is an HTML label.  If it is, it selects the corresponding control (either via the for attribute, or via searching the label's children).

nsEventStateManager::GetAccessKeyTargetContent() first checks to see if the content is a legend.  If it is, the next tabbable element is selected, otherwise, it calls nsContentUtils::GetAccessKeyTargetContent().

IsAccessKeyTarget() is now passed the content returned from nsEventStateManager::GetAccessKeyTargetContent().  The only glitch is that the area element (if it doesn't have tabindex set) is never focusable, so I explicitly check this at the end of the IsAccessKeyTarget().  This way, invisible area elements will not be selected.

The only other thing worth noting is that I rewrote nsHTMLLabelElement::GetForContent() in terms of nsContentUtils::GetAccessKeyTargetContent() to avoid code duplication.

With this patch applied, the test case in attachment 302552 [details] [diff] [review] passes successfully.  roc: this is ready for review.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  /**
+   * Get the content for the access key target.
+   */
+  static nsIContent* GetAccessKeyTargetContent(nsIContent *aContent);

Need to explain this a little more. Say that aContent is the content that contains the access key label and you return the content that should be focused when the user presses the access key.

+  nsCOMPtr<nsIDOMHTMLLabelElement> label = do_QueryInterface(aContent);
+  if (label) {

Avoid deep nesting by writing
  if (!label)
    return aContent;

+        nsIContent *result = nsnull;
+        if (domElement) {
+          CallQueryInterface(domElement, &result);

Use nsCOMPtr<nsIContent> result = do_QueryInterface(domElement);
(that will set result to null if domElement is null)

+nsContentUtils::IsNonLabelFormControl

Just make this a static global function.

It's a pain that legends are handled in nsEventStateManager::GetAccessKeyTargetContent instead of nsContentUtils::GetAccessKeyTargetContent. (And even so, we might not get the same result pressing the accesskey as if we focused the legend.) Can we just make legends focus the next focusable element, find that in nsContentUtils::GetAccessKeyTargetContent, and make nsHTMLLegendElement::SetFocus call nsContentUtils::GetAccessKeyTargetContent? Then we don't need nsEventStateManager::GetAccessKeyTargetContent.

Why not have IsAccessKeyTarget call GetAccessKeyTargetContent and GetPrimaryFrameFor so we don't have to duplicated those lines?

Comment 36

[(no longer active)]

(In reply to comment #35)
> +  /**
> +   * Get the content for the access key target.
> +   */
> +  static nsIContent* GetAccessKeyTargetContent(nsIContent *aContent);
> 
> Need to explain this a little more. Say that aContent is the content that
> contains the access key label and you return the content that should be focused
> when the user presses the access key.

Will do in the next iteration.

> +  nsCOMPtr<nsIDOMHTMLLabelElement> label = do_QueryInterface(aContent);
> +  if (label) {
> 
> Avoid deep nesting by writing
>   if (!label)
>     return aContent;

OK.

> +        nsIContent *result = nsnull;
> +        if (domElement) {
> +          CallQueryInterface(domElement, &result);
> 
> Use nsCOMPtr<nsIContent> result = do_QueryInterface(domElement);
> (that will set result to null if domElement is null)

OK.  I inherited this code from nsHTMLLabelElement, and I suspected if it's calling CallQueryInterface() for some special reason I'm not aware of.

> +nsContentUtils::IsNonLabelFormControl
> 
> Just make this a static global function.

Sure.

> It's a pain that legends are handled in
> nsEventStateManager::GetAccessKeyTargetContent instead of
> nsContentUtils::GetAccessKeyTargetContent. (And even so, we might not get the
> same result pressing the accesskey as if we focused the legend.)

Why is that?  Focusing the legend moves the focus forward if I'm reading the code correctly, which is achieved by nsEventStateManager::GetNextTabbableContent().  Am I making a mistake?

> Can we just
> make legends focus the next focusable element, find that in
> nsContentUtils::GetAccessKeyTargetContent, and make
> nsHTMLLegendElement::SetFocus call nsContentUtils::GetAccessKeyTargetContent?
> Then we don't need nsEventStateManager::GetAccessKeyTargetContent.

nsHTMLLegendElement::SetFocus() just moves the focus forward, and does not get the content which should be focused explicitly.  Moving the focus forward leads to a call to nsEventStateManager::GetNextTabbableContent() (as I explained above) which is what I used in my patch.  Of course I could rewrite nsHTMLLegend::SetFocus() to get the target content explicitly and focus it, if that's what you mean.  The reason I placed the legend related code in nsEventStateManager::GetAccessKeyTargetContent() instead of nsContentUtils::GetAccessKeyTargetContent() is that I was not sure if it's OK to make nsContentUtils dependent on nsEventStateManager.  There doesn't seem to exist such a dependency currently in nsContentUtils.

> Why not have IsAccessKeyTarget call GetAccessKeyTargetContent and
> GetPrimaryFrameFor so we don't have to duplicated those lines?

That's a valid issue.  I'll address this in the next iteration.  Waiting for your input on the legend-related stuff above...

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #36)
> > It's a pain that legends are handled in
> > nsEventStateManager::GetAccessKeyTargetContent instead of
> > nsContentUtils::GetAccessKeyTargetContent. (And even so, we might not get
> > the same result pressing the accesskey as if we focused the legend.)
> 
> Why is that?  Focusing the legend moves the focus forward if I'm reading the
> code correctly, which is achieved by
> nsEventStateManager::GetNextTabbableContent().  Am I making a mistake?

There's no guarantee that GetNextTabbableContent is the same element as what legend gets moving the focus forward. nsEventStateManager::ShiftFocusInternal does a lot of extra checks before it gets to GetNextTabbableContent. Even if we could prove that they always have the same effect, making them use the same code is the only sure way of making sure that stays true in the future.

> > Can we just
> > make legends focus the next focusable element, find that in
> > nsContentUtils::GetAccessKeyTargetContent, and make
> > nsHTMLLegendElement::SetFocus call nsContentUtils::GetAccessKeyTargetContent?
> > Then we don't need nsEventStateManager::GetAccessKeyTargetContent.
> 
> nsHTMLLegendElement::SetFocus() just moves the focus forward, and does not get
> the content which should be focused explicitly.  Moving the focus forward
> leads to a call to nsEventStateManager::GetNextTabbableContent()
> (as I explained
> above) which is what I used in my patch.  Of course I could rewrite
> nsHTMLLegend::SetFocus() to get the target content explicitly and focus it, if
> that's what you mean.  The reason I placed the legend related code in
> nsEventStateManager::GetAccessKeyTargetContent() instead of
> nsContentUtils::GetAccessKeyTargetContent() is that I was not sure if it's OK
> to make nsContentUtils dependent on nsEventStateManager.  There doesn't seem
> to exist such a dependency currently in nsContentUtils.

I think that's OK, they're both in 'content' and there is an interface nsIEventStateManager so you don't even have to include nsEventStateManager.h from nsContentUtils.

Thanks for working through this!

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

Moving bugs that aren't beta 4 blockers to target final release.

Comment 39

[alexander :surkov (:asurkov)]

Richard Schwerdtfeger (Distinguished Engineer, SWG Accessibility Architect/Strategist
Chair, IBM Accessibility Architecture Review Board) gave some clarification about accesskeys on focusable elements:

"From an ARIA widget perspective I don't think we want to be activating the widget (equivalent of a click) - we should be giving the element focus. 

Getting off my soap box I would follow what IE does as that will give you two browsers:

IE does support access keys on all focusable elements. This includes divs and spans. I would suggest that in the interim we follow this approach. "

Should we deal with this in the bug or should I open new one?

Comment 40

[Johnny Stenback (:jst)]

Any progress here? Roc, are you reviewing?

Comment 41

[(no longer active)]

Comment on attachment 305166 [details] [diff] [review]
Patch (v2)

(In reply to comment #40)
> Any progress here? Roc, are you reviewing?

I've been swamped with work lately.  I'll provide a new patch in a few days for roc to review...

Comment 42

[(no longer active)]

Attachment: Patch (v2.1)

Sorry for the delay.

This new patch addresses all of the previous comments on Patch (v2) (attachment 305166 [details] [diff] [review]).  With this patch applied, the test case in attachment 302552 [details] [diff] [review] fully passes.

roc: I hope you can get to review this patch so that we can land it for RC1...  :-)

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks great, just some nits to pick.

+  nsCOMPtr<nsIDOMHTMLLabelElement> label = do_QueryInterface(aContent);
+  if (!label) {
+    nsCOMPtr<nsIDOMHTMLLegendElement> legend = do_QueryInterface(aContent);
+    if (!legend)

This function would be structured better if it was
  if (label) {
    ...
  } else if (legend) {
    ...
  }

+  if (forContent) {
+    NS_ADDREF(forContent);
   }

NS_IF_ADDREF

+    nsCOMPtr<nsIContent> focusContent =
+        nsContentUtils::GetAccessKeyTargetContent(this);
+    nsCOMPtr<nsIDOMElement> focusElement = do_QueryInterface(focusContent);

This last line is not needed right?

+      if (content = IsAccessKeyTarget(content, mPresContext, accKey)) {

I would wrap this in "!= nsnull" to avoid compiler warnings and reader confusion.

I think IsAccessKeyTarget should be GetAccessKeyTarget since it now returns a non-boolean.

sr+ from me with that. You probably should run it by jst as well for review.

Comment 44

[(no longer active)]

Attachment: Patch (v2.2)

Nits addressed.  Asking jst for review, and requesting sr from roc again so that the patch gets an official sr+ flag.

Comment 45

[(no longer active)]

BTW, roc: please don't forget to review the testcase (attachment 302552 [details] [diff] [review]) as well.

Comment 46

[Johnny Stenback (:jst)]

Comment on attachment 314323 [details] [diff] [review]
Patch (v2.2)

r=jst, but a change of this size in focus/event related code scares me at this point in the release.

If there's an easier lower-imapct way to do this for this release I think I'd prefer that. Tests help, and thank you for taking the time to write them! I'd like to have Smaug look at this before it goes in, assuming we'll take it.

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, it is a bit late to be taking this change, although it is a regression.

Comment 48

[Gérard Talbot]

Hello all,

Shouldn't component for this bug be Keyboard: Navigation ? It would help when searching for duplicates...

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 314323 [details] [diff] [review]
Patch (v2.2)


>+  } else if (legend) {
>+    nsCOMPtr<nsIDocument> doc = aContent->GetCurrentDoc();
>+    NS_ASSERTION(doc, "no document");
>+
>+    nsCOMPtr<nsIContent> rootContent = doc->GetRootContent();
>+    nsCOMPtr<nsIPresShell> presShell = doc->GetPrimaryShell();
>+    NS_ASSERTION(presShell, "no presShell");
>+    nsCOMPtr<nsPresContext> presContext = presShell->GetPresContext();
>+    NS_ASSERTION(presContext, "no presContext");
>+    nsIEventStateManager *esm = presContext->EventStateManager();
>+    NS_ASSERTION(esm, "no EventStateManager");
Since this is public-inside-gecko-API, could you please replace those assertions
with NS_ENSURE_TRUE(expr, nsnull);
There used to be quite a few crashes caused by null presshell/context during unloading a page.
And IMHO assertions should be used to indicate cases which shouldn't be possible;
a document without a shell possible.

You have tested bug 81481, right?

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 302552 [details] [diff] [review]
Test case (v2)

This test looks good, but it would also be good to test cases where the label is visible but the control is invisible, and when the label is invisible but the control is visible.

Comment 51

[(no longer active)]

(In reply to comment #49)
> (From update of attachment 314323 [details] [diff] [review])
> 
> >+  } else if (legend) {
> >+    nsCOMPtr<nsIDocument> doc = aContent->GetCurrentDoc();
> >+    NS_ASSERTION(doc, "no document");
> >+
> >+    nsCOMPtr<nsIContent> rootContent = doc->GetRootContent();
> >+    nsCOMPtr<nsIPresShell> presShell = doc->GetPrimaryShell();
> >+    NS_ASSERTION(presShell, "no presShell");
> >+    nsCOMPtr<nsPresContext> presContext = presShell->GetPresContext();
> >+    NS_ASSERTION(presContext, "no presContext");
> >+    nsIEventStateManager *esm = presContext->EventStateManager();
> >+    NS_ASSERTION(esm, "no EventStateManager");
> Since this is public-inside-gecko-API, could you please replace those
> assertions
> with NS_ENSURE_TRUE(expr, nsnull);
> There used to be quite a few crashes caused by null presshell/context during
> unloading a page.
> And IMHO assertions should be used to indicate cases which shouldn't be
> possible;
> a document without a shell possible.

OK, will do.

> You have tested bug 81481, right?

I tested both attachment 94558 [details] and attachment 169813 [details], and neither worked as expected.  This is especially strange since as far as I can tell, little is different between my unit test and attachment 94558 [details] at least.

I'll debug this and attach a new patch.  In the mean time if someone has a bit of free time and can give me a hand in debugging this, please let me know, as I don't have a lot of free time to work on this, and I don't want this to miss 1.9.

(In reply to comment #50)
> (From update of attachment 302552 [details] [diff] [review])
> This test looks good, but it would also be good to test cases where the label
> is visible but the control is invisible, and when the label is invisible but
> the control is visible.

Will do in the next iteration of the patch.

Comment 52

[Mike Schroepfer]

As discussed in FF3 meeting today given the risk of this patch we are looking for a smaller less risky version we can take in dot release

Comment 53

[Timwi]

Am I right in assuming that this means FF3 will have this bug and keep it until FF4?

Comment 54

[Johnny Stenback (:jst)]

Not if we get a safer fix, or we have enough time to bake this fix elsewhere to make sure it's safe enough to take for a 3.0.0.x release AFAICT.

IOW, to get this fixed for Firefox 3 now we'd need a much simplified fix, one that's less likely to cause regressions etc, otherwise it'll have to wait for a followup release.

Comment 55

[(no longer active)]

Any idea of what a simpler fix would be?  Roc, anyone?

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not sure.

I think probably the best thing to do is to take this "on trunk" after we've shipped Firefox 3 and then get it onto some branch after that.

Comment 57

[luihm]

Does the original patch work in all cases? My impression from the comments is that it got discarded because it wasn't the best design. However, it looks pretty simple to me.

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it breaks accesskeys for DIV elements with tabindex, mentioned in comment #32. But if it doesn't, then maybe we can take that patch.

Comment 59

[Timwi]

If nothing else, surely a patch that breaks DIVs (which very rarely have an accesskey) is better than leaving the bug in, which breaks LABELs (a much more common case)?

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess the bigger problem is that that patch makes non-focusable elements valid accesskey targets.

Maybe a safe stop-gap fix would be to extend the IsFocusable check to check IsFocusable || is-label || is-area || is-legend.

Comment 61

[(no longer active)]

(In reply to comment #60)
> Maybe a safe stop-gap fix would be to extend the IsFocusable check to check
> IsFocusable || is-label || is-area || is-legend.

Do you want me to knock up a quick patch like that?

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure, that would be great.

Comment 63

[(no longer active)]

Attachment: Patch (v1.2) + tests

OK, here's a new patch with an updated test case.  The risk should be pretty minimal with the new patch.  The whole test case passes, and I have tested the test cases attached to bug 81481 manually, and they all work.

I think we may need to open a bug to revert this post-1.9, and make sure a proper fix is implemented...

Comment 64

[(no longer active)]

Renominating as blocking now that we have a simple and low risk patch which can land in 1.9.  The patch is covered by an extensive unit test.

Comment 65

[(no longer active)]

(In reply to comment #63)
> Created an attachment (id=316282) [details]
> Patch (v1.2) + tests

I forgot to mention that test has been modified according to comment 50.

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 316282 [details] [diff] [review]
Patch (v1.2) + tests

great. let's do this!

Comment 67

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 316282 [details] [diff] [review]
Patch (v1.2) + tests

a1.9=beltzner

Comment 68

[Damon Sicore (:damons)]

Wouldn't block the release for it as we could take this in a dot release, but we should take the patch.

-'ing.  Please re-nom if you disagree.

Comment 69

[:Gavin Sharp [email: [e-mail]]]

mozilla/content/events/src/nsEventStateManager.cpp 	1.739
mozilla/content/events/test/Makefile.in 	1.16
mozilla/content/events/test/test_bug409604.html 	1.1

Comment 70

[:Gavin Sharp [email: [e-mail]]]

This caused unit test failures on Windows, so I had to back it out.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1208884715.1208887557.12050.gz&fulltext=1

Comment 71

[(no longer active)]

OK, I investigated the test_bug409604.html tests, and here is the results:

* All tests will pass if the window is on the foreground.
* The following is the result of running the tests when the window is in the background:
Passed: 276
Failed: 1
Todo: 0
not ok - (focus) unhandled elements remaining: d,g,h,k,l,m
* The following is the results of the test when the window is minimized.
Passed: 268
Failed: 7
Todo: 0
not ok - (click) unexpected element: c expected: b
not ok - (click) unexpected element: e expected: c
not ok - (click) unexpected element: f expected: e
not ok - (click) unexpected element: i expected: f
not ok - (click) unexpected element: j expected: i
not ok - (focus) unhandled elements remaining: d,g,h,k,l,m
not ok - (click) unhandled elements remaining: j

IINM, the qm-win2k3-02 unit test tinderbox runs the tests with the main window minimized.  I was under the impression that the automated unit tests should be run keeping the browser's window in the foreground, and without moving the mouse or playing with the keyboard...  I'm CCing sayrer for his input.  In the mean time, if someone else can apply the patch to their tree and run the unit test and report the results here, that would be great.

BTW, I'm currently looking at the rest of unit test failures.  I'll report the results here.

Comment 72

[(no longer active)]

It seems that the content/html/content tests failed because of the same reason as well.  I ran the full content/html/content test suite with a build having this patch (with the window focused add on the foreground) and none of the tests failed.

Comment 73

[(no longer active)]

roc, sayrer: what do you think?

Comment 74

[:Gavin Sharp [email: [e-mail]]]

More than just that test would fail if the window really wasn't focused, or was minimized (we've had this problem many times before on those machines). I'll try running them on Windows and see if I can reproduce.

Comment 75

[:Gavin Sharp [email: [e-mail]]]

I can't reproduce the failures on Windows, the tests pass for me.

Comment 76

[(no longer active)]

The tests pass on Windows and Linux for me as well.  The tinderbox which observed the failure is a Win2K3 machine.  Does anyone have any idea why the tests fail on the tinderbox?  Is there any chance of simulating the tinderbox environment on my own machine?

Comment 77

[(no longer active)]

Bug 431464 may fix the test fail issues Gavin reported in comment 70.

Comment 78

[(no longer active)]

Attachment: Patch (v1.2) backed out parts (for check-in)

Comment 79

[(no longer active)]

I tested this patch and its test case now that bug 431464 has landed, and it no longer fails because of the focus issue as described in comment 71.  Attachment 319933 [details] [diff] is the backed out parts of the patch landed in comment 69, which had r,sr+ and a+.  I'm requesting check-in of this patch now that it no longer should fail the unit tests.

Comment 80

[:Gavin Sharp [email: [e-mail]]]

mozilla/content/events/src/nsEventStateManager.cpp 	1.742
mozilla/content/events/test/Makefile.in 	1.18 

Comment 81

[:Gavin Sharp [email: [e-mail]]]

I had to disable part of the test, because it was failing on the windows unit test machines and intermittently on the Linux and Mac machines. See bug 433089.

Comment 82

[rick mans]

This still seems to be an issue concerning A elements. When pressing the accesskey combination the A gets focus, however the user agent does not follow the link (behaviour < FF3)

Comment 83

[(no longer active)]

(In reply to comment #82)
> This still seems to be an issue concerning A elements. When pressing the
> accesskey combination the A gets focus, however the user agent does not follow
> the link (behaviour < FF3)

Hmm, this works for me.  For example, on this very page, press Alt+Shift+L (the accesskey for the "Last Comment" link, and the anchor is navigated to.  Anyway, the only elements this bug touched were the label, area and legend elements, as indicated by the summary.

Comment 84

[rick mans]

(In reply to comment #83)
> (In reply to comment #82)
> > This still seems to be an issue concerning A elements. When pressing the
> > accesskey combination the A gets focus, however the user agent does not follow
> > the link (behaviour < FF3)
> 
> Hmm, this works for me.  For example, on this very page, press Alt+Shift+L (the
> accesskey for the "Last Comment" link, and the anchor is navigated to.  Anyway,
> the only elements this bug touched were the label, area and legend elements, as
> indicated by the summary.
> 

You are correct. I think the issue is that the same accesskey is defined twice for different A tags.

Comment 85

[(no longer active)]

(In reply to comment #84)
> You are correct. I think the issue is that the same accesskey is defined twice
> for different A tags.

In that case, you're getting the expected behavior.  The same thing happens for multiply defined accesskeys in chrome windows as well.

Comment 86

[Timwi]

I agree with Ehsan that this is expected behaviour. Otherwise the second A tag with the same accesskey could not be navigated to (because the first one would always get activated). The solution is simply not to assign the same shortcut twice.

Comment 87

[rick mans]

I agree, it is expected behaviour according to the specs. It was unexpected behaviour at first because FF2 handled it differently.