Closed Bug 554635 Opened 10 years ago Closed 10 years ago

Keystroke handling worked in earlier versions of FF but not in 3.6.2

Categories

(Core :: XUL, defect, P2)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta1+
blocking1.9.2 --- -
status1.9.2 --- wanted

People

(Reporter: david.dailey, Assigned: enndeakin)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2 (.NET CLR 3.5.30729)

First draw a few nodes (by clicking on the screen). Then hit the A key. In other browsers (Opera, IE/ASV, Safari, Chrome), all drawn nodes should be selected. Was working in FF in October 2009. (other keys suffer from the same problem -- t, delete, etc -- see help file for other options, though the shift key is working as it should -- shift-click adds or deletes from the selected collection.

Reproducible: Always

Steps to Reproduce:
1.see above
2.
3.
Actual Results:  
see above


Robert Longson and I talked a bit about this problem. He and Cameron probably saw the software in action at SVGOpen.
David. There are nightly builds here: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/

3.6.x is equivalent to -mozilla-1.9.2

If you can download these nightlies and work out the day that it stopped working by using a binary chop then we're far more likely to be able to fix this.
Marking as new and broken on recent trunk.

broken:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a4pre) Gecko/20100331 Minefield/3.7a4pre

These are on mozilla-central.  If you need a regression window for mozilla-1.9.2, add the keyword again and state so.

Regression window:

works:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090610 Minefield/3.6a1pre
http://hg.mozilla.org/mozilla-central/rev/90d3e6d2cbb9

broken:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090611 Minefield/3.6a1pre
http://hg.mozilla.org/mozilla-central/rev/4430cae50dad

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=90d3e6d2cbb9&tochange=4430cae50dad
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 178324
Component: General → XUL
Product: Firefox → Core
QA Contact: general → xptoolkit.widgets
Neil, can you take a look?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.2: --- → ?
The page only marginally works before the focus changes as well. It only does at first because the child page calls window.focus() upon loading. Focusing some other element will shift focus away and it won't work any more. Clicking the svg area does not shift the focus back to it. In fact, there is no means for the user to refocus the svg area once it's lost.

The page is using an svg page loaded via an <object>, which seems the likely distinction here. Are the docshells hooked up the same for this?

The focus manager relies on GetSubDocumentFor(iframeNode) to navigate down and window->GetFrameElementInternal() to go up. Presumably these aren't set for <object> elements?
> Are the docshells hooked up the same for this?

Yes.

> Presumably these aren't set for <object> elements?

They should be for document-containing <object>s like this one.
OK, I'll do some further testing.
The regression here is caused because nsHTMLObjectElement::IsHTMLFocusable indicates false. This is because the tabindex defaults to -1 for <object> if it isn't specified, thus <object> cannot be focused.

Note that when <object> contains a plugin, IsHTMLFocusable returns true unconditionally.

Changing the implementation of nsHTMLObjectElement::GetTabIndex to default to 0 instead of -1 fixes the regression, and as a bonus makes tab navigation back to the svg frame work again. (Tabbing out doesn't work, but the page explicitly calls preventDefault on all keypresses)

However, I'm not sure what side effects this patch might have.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Needed, but I don't think it blocks the next release. If we get a reviewed and baked patch, we'll approve it.
blocking1.9.2: ? → needed
Attached patch add testSplinter Review
Attachment #438088 - Attachment is obsolete: true
Attachment #439008 - Flags: review?(jonas)
http://hg.mozilla.org/mozilla-central/rev/b8c6a4f81a58
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
blocking2.0: ? → beta1+
Priority: -- → P2
Depends on: 569315
Removing the blocking flag as this wouldn't block a branch release. If you would like to nominate this for a branch, please make sure to have a roll-up patch with all changes.
blocking1.9.2: needed → -
Adding Jonas in CC (he did the review).

The fix in this patch seems to be wrong.
It has changed GetTabIndex to returns 0 in case of tabindex isn't specified thus making <object> always focusable. Actually, IsHTMLFocusable is completely broken and shouldn't look at GetTabIndex considering that GetTabIndex should return 0 or -1 as a default value depending of the focusable state of the element...

At least two bugs have been reported because of that: bug 569315 and bug 596350p

If Neil and Jonas agree, I will revert this change and add tests checking that, by default, an <object> isn't focusable.
I will also open a follow up to clean the mess that IsHTMLFocusable is...
Bugs for IsHTMLFocusable and GetTabIndex: bug 597242 and bug 597241
Can you explain the alternative way you plan on fixing this bug?
(In reply to comment #16)
> Can you explain the alternative way you plan on fixing this bug?

I have no idea how this should be fixed but having <object> focusable by default is clearly not the way to do it. Opera and Chromium do not have <object> focusable by default and this site works for them. I guess the issue must be somewhere else.
Depends on: 596350
Depends on: 720130
You need to log in before you can comment on or make changes to this bug.