Open Bug 324156 Opened 18 years ago Updated 2 years ago

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

Categories

(Core :: Layout, defect)

x86
Windows XP
defect

Tracking

()

REOPENED

People

(Reporter: martijn.martijn, Unassigned)

References

Details

(Keywords: testcase)

Attachments

(3 files, 5 obsolete files)

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.
Attached file testcase
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.
Attached patch patch (obsolete) — Splinter Review
Thanks Neil!
So something like this, then?
Attachment #214324 - Flags: review?(enndeakin)
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.
Attachment #214324 - Flags: review?(enndeakin) → review+
Attached patch patch, updated to comments (obsolete) — Splinter Review
Attachment #214352 - Flags: superreview?(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.
Attachment #214352 - Flags: superreview?(bzbarsky) → superreview+
Attached patch patch2 (obsolete) — Splinter Review
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.
Attachment #214424 - Flags: review?(enndeakin)
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.
Attached patch patch3 (obsolete) — Splinter Review
So, like this?
Attachment #214324 - Attachment is obsolete: true
Attachment #214352 - Attachment is obsolete: true
Attachment #214424 - Attachment is obsolete: true
Attachment #214433 - Flags: review?(enndeakin)
Attachment #214424 - Flags: review?(enndeakin)
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.
Attachment #214433 - Flags: review?(enndeakin) → review+
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?
Attached patch patch4 (obsolete) — Splinter Review
Well, this fixes the issues for me.
I guess it makes sense that the settimeout is necessary now.
Attachment #214433 - Attachment is obsolete: true
Attachment #214491 - Flags: review?(enndeakin)
Argh! The whole time I meant to ask Neil Parkway for review...
Comment on attachment 214491 [details] [diff] [review]
patch4

Probably a good idea to have the other Neil look at it too anyway
Attachment #214491 - Flags: review?(enndeakin) → review?(neil)
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();
Attachment #214491 - Flags: review?(neil) → review-
Attached patch patch5Splinter Review
Indeed, oops.
Well, menulist was already working. I just needed to add textbox in xul.css for the -moz-user-focus: normal; rule.
Attachment #214491 - Attachment is obsolete: true
Attachment #215302 - Flags: review?(neil)
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 on attachment 215302 [details] [diff] [review]
patch5

I said editable menulists, and this patch breaks shift-tabbing out of textboxes anyway.
Attachment #215302 - Flags: review?(neil) → review-
(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);
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.
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.
I'm now handling the blur part in bug 330705.
Blocks: 330705
(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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: