324156 - Every xul element is focusable with onclick=this.focus()

Status: REOPENED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase,

With the testcase, I would expect a green background, but I get a red background.
See that I've set the -moz-user-focus style to ignore, so the element should not be focusable.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Neil Deakin]

nsXULElement::Focus should be calling nsIFrame::IsFocusable to determine if an element can be focused.

Also of note is that the accompanying nsXULElement::Blur function doesn't actually do anything.

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase2, testing blur

Comment 4

[Martijn Wargers (dead)]

Attachment: patch

Thanks Neil!
So something like this, then?

Comment 5

[Neil Deakin]

Comment on attachment 214324 [details] [diff] [review]
patch

Looks good, if you add a null-check for the frame.

if(!frame || !frame->IsFocusable()) {

Also, use four space indents to match the rest of the file.

Comment 6

[Martijn Wargers (dead)]

Attachment: patch, updated to comments

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 214352 [details] [diff] [review]
patch, updated to comments

>Index: content/xul/content/src/nsXULElement.cpp

First of all the indents here are weird.  Are you using spaces while the file uses tabs or vice versa?

>+        nsIContent* content = NS_STATIC_CAST(nsIContent*, this);
>+        nsIFrame* frame = shell->GetPrimaryFrameFor(content);

Shouldn't need this -- just pass |this| to GetPrimaryFrameFor.

sr=bzbarsky with those fixed.

Comment 8

[Martijn Wargers (dead)]

Attachment: patch2

Sorry, but actually, the code should perhaps be in setfocus? Because that's also the case in genericelement.cpp:
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#2159

And maybe the scrollintoview code should also be in xulelement.cpp

Boris, no, the file uses spaces and I am doing also.

Comment 9

[Neil Deakin]

In fact, you can just get rid of nsXULElement::SetFocus and use the inherited version, as a more correct check for the disabled state is already done in IsFocusable.

Comment 10

[Martijn Wargers (dead)]

Attachment: patch3

So, like this?

Comment 11

[Neil Deakin]

Comment on attachment 214433 [details] [diff] [review]
patch3

Looks OK.

I worry that there may be some code that is relying on the old behaviour though. The <tabbox> has some special handling for focusing the tab, but only drawing the focus ring on second click, so make sure that still works.

Comment 12

[Martijn Wargers (dead)]

Hmm, yes, behavior has changed with tabs.
The main problem I'm seeing is that Ctrl+Left Arrow and Ctrl+Right Arrow is only working one time. After that, the newly selected tab isn't working anymore, so using the keyboard shortcut again isn't possible.
So is that something that needs to be fixed in tabbox.xml?

Comment 13

[Martijn Wargers (dead)]

Attachment: patch4

Well, this fixes the issues for me.
I guess it makes sense that the settimeout is necessary now.

Comment 14

[Martijn Wargers (dead)]

Argh! The whole time I meant to ask Neil Parkway for review...

Comment 15

[Neil Deakin]

Comment on attachment 214491 [details] [diff] [review]
patch4

Probably a good idea to have the other Neil look at it too anyway

Comment 16

[[e-mail]]

Comment on attachment 214491 [details] [diff] [review]
patch4

Not just tabboxes, but textboxes and editable menulists rely on this behaviour. This patch breaks aTextbox.focus();

Comment 17

[Martijn Wargers (dead)]

Attachment: patch5

Indeed, oops.
Well, menulist was already working. I just needed to add textbox in xul.css for the -moz-user-focus: normal; rule.

Comment 18

[Neil Deakin]

Trying to change the behaviour of the focus() method as it is currently implemented will lead to too many problems. I think what should happen here is to just define and document what the focus() method actually does, assuming of course that that behaviour makes sense.

Seems to me that it just focuses the content or the first focusable descendant.

Comment 19

[[e-mail]]

Comment on attachment 215302 [details] [diff] [review]
patch5

I said editable menulists, and this patch breaks shift-tabbing out of textboxes anyway.

Comment 20

[[e-mail]]

(In reply to comment #18)
>Seems to me that it just focuses the content or the first focusable descendant.
That's similar to document.commandDispatcher.advanceFocusIntoSubtree(element);

Comment 21

[Martijn Wargers (dead)]

Hmm, the case for editable menulists is not really solvable as far as I can see.
I guess it's better to let the behavior of the focus method just stay as it is (as Neil Deakin) suggested. It's more a weird xul oddity than an annoying bug.

The blur function should be added, though, I think.

Comment 22

[Martijn Wargers (dead)]

On the other hand, why is this -moz-user-focus: ignore; rule added for editable menulists?
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/global/resources/content/xul.css&rev=1.96&root=/cvsroot#57
I thought it was for preventing of getting a caret in the input when clicking at the drop-down button, but that's not the case, because I can see that happening now in current builds.

Comment 23

[Martijn Wargers (dead)]

I'm now handling the blur part in bug 330705.

Comment 24

[[e-mail]]

(In reply to comment #22)
>On the other hand, why is this -moz-user-focus: ignore; rule added for editable menulists?
To take the menulist out of the tab order, because it has a focusable child.