252984 - Cannot type in any of the fields - forms don't get focus on left-click

Status: VERIFIED FIXED

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

Description

[James Rome]

This Web page will not allow me to type into any of the fields.
Build 2004072307 Windows XP Pro

Comment 1

[R.K.Aa.]

I see this too, testing with 2004072307 WinXP

I have to right-click in the fields and bring up the context - THEN start typing.
The fields simply don't get focus with a common left-click

Comment 2

[Boris Zbarsky [:bzbarsky]]

This broke between 2004-07-05-07 and 2004-07-06-19

It's a regression from the bug 171366 landing -- removing the tabindex="18" from
 the <form> makes the page work again.

Note that clicking in a textfield puts a focus outline on the form, for some odd
reason.

Comment 3

[Boris Zbarsky [:bzbarsky]]

ccing reviewers of bug 171366 too.

James, for further reference, "form manager" is about saving and prefilling form
data, not this sort of problem...

Comment 4

[James Rome]

I am glad it is a regression and not an example of dirty politics. ;-)

I apologize for the mislabeling of the component, but it is not at all clear
what component does what, or even what they do! It would be really nice if
Bugzilla put on some tool tips on the list items.

Comment 5

[Boris Zbarsky [:bzbarsky]]

James, the "Component" link next to the dropdown gives descriptions of all the
components....

Comment 6

[Aaron Leventhal]

We both need to fix something (the site and Mozilla).

Page fix:
tabindex=18 is on the <form> is bad. This creates issues even in IE. For
example, using IE click to the right of the 3 checkboxes at the bottom of the
page. Notice how a bunch of things appear to receive focus. Please get rid of
tabindex on the form itself, because a positive tabindex makes something
focusable and the <form> should not be focusable. Tabindex should go on the
individual focusable controls. For more info on tabindex see
msdn.microsoft.com/workshop/author/dhtml/reference/properties/tabindex.asp

Mozilla fix:
Mozilla does not want to have focusable decendants of something that's already
focusable (unless it's a frame/iframe). This avoids problems like <button>'s or
<a>'s with focusable things inside of them. I think I'll do this with a black
list for button and a instead of the current whitelist approach for iframe and
frame.

Comment 7

[Aaron Leventhal]

Attachment: Use whitelist in nsFrame::FocusableAncestor() and remove nsGenericHTMLElement::HandleDOMEvent()

Very cool -- we actually don't need nsGenericHTMLElement::HandleDOMEvent() any
more because click to focus is now generically handled for everything in
esm::PostHandleEvent(), which was changed in bug 250006 to use IsFocusable().
So, that now handles the case where click needs to focus something like a div
or span with tabindex >= 0

Comment 8

[Aaron Leventhal]

Attachment: Use whitelist in nsFrame::FocusableAncestor() and remove nsGenericHTMLElement::HandleDOMEvent()

Patch with more context this time.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Aaron, the xlink code is wrong (it should not be in the |if (HTML)| block).

Comment 10

[Aaron Leventhal]

Attachment: Oops, Boris is right. Had the xlink condition in the wrong block

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 154477 [details] [diff] [review]
Oops, Boris is right. Had the xlink condition in the wrong block

First of all, this assumes that HTML nodes can't be XLinks.  That's true so far
in Mozilla, but we need to fix that, and I'd rather not build an assumption
about it into more code.

Second, this seems to treat <a> the same whether or not it's actually got a
reasonable href attribute.  So <a name="foo"><input></a> won't work, as far as
I can tell.

Finally, if we _do_ have a button inside a link or something along those lines,
clicking on the button should trigger the _button_, not the link.  We have an
existing bug on this....  It looks like this patch continues enforcing the
current buggy behavior of triggering the link, not the button, no?

Comment 12

[Aaron Leventhal]

Attachment: We actually don't need either nsIFrame::FocusableAncestor() or nsGenericHTMLElement::HandleDOMEvent() anymore

I realize now that FocusableAncestor() was necessary before the rewrite in bug
250006. However, now IsFocusable() does everything it needs without the
focusable ancestor check. There's no longer any danger of the wrong thing
getting focused in something like <a href="Foo">Hello you <b>crazy</b>
animal</a>. This is because nsIFrame/nsIContent::IsFocusable() know what
they're doing and we don't just use -moz-user-focus to decide if something is
focusable (-moz-user-focus is inherited by child elements which would cause the
problem).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 154483 [details] [diff] [review]
We actually don't need either nsIFrame::FocusableAncestor() or nsGenericHTMLElement::HandleDOMEvent() anymore

If things still work right and someone who knows the focus code reviews,
sr=bzbarsky

Comment 14

[Aaron Leventhal]

Comment on attachment 154483 [details] [diff] [review]
We actually don't need either nsIFrame::FocusableAncestor() or nsGenericHTMLElement::HandleDOMEvent() anymore

Brian, this patch is pure code removal.

Comment 15

[Aaron Leventhal]

Comment on attachment 154483 [details] [diff] [review]
We actually don't need either nsIFrame::FocusableAncestor() or nsGenericHTMLElement::HandleDOMEvent() anymore

Forgot to put bryner's email in review flag field. Bryner, this is a pure code
removal patch.

Comment 16

[Aaron Leventhal]

Attachment: Correct patch. In the last one The last patch didn't include the removal of the call to FocusableAncestor() in nsFrame::IsFocusable()

Comment 17

[Aaron Leventhal]

Comment on attachment 154652 [details] [diff] [review]
Correct patch. In the last one The last patch didn't include the removal of the call to FocusableAncestor() in nsFrame::IsFocusable()

Carrying bz's sr=

Comment 18

[Aaron Leventhal]

Checking in layout/base/public/nsIFrame.h;
/cvsroot/mozilla/layout/base/public/nsIFrame.h,v  <--  nsIFrame.h
new revision: 3.249; previous revision: 3.248
done
Checking in layout/html/base/src/nsFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.494; previous revision: 3.493
done
Checking in content/html/content/src/nsGenericHTMLElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,v  <-- 
nsGenericHTMLElement.cpp
new revision: 1.529; previous revision: 1.528
done
Checking in content/html/content/src/nsGenericHTMLElement.h;
/cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.h,v  <-- 
nsGenericHTMLElement.h
new revision: 1.197; previous revision: 1.196
done

Comment 19

[Boris Zbarsky [:bzbarsky]]

*** Bug 253659 has been marked as a duplicate of this bug. ***

Comment 20

[Stefan Borggraefe]

*** Bug 253708 has been marked as a duplicate of this bug. ***

Comment 21

[James Rome]

I am not sure this is fixed yet. I assume the patch is in the 7/30 build. But
http://www.cs.utk.edu/~dunigan/landforms/map.html
does not properly support clicking.

Comment 22

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED with build 2004-08-16-13 on Windows XP.