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

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[david.dailey]

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.

Comment 1

[Robert Longson [:longsonr]]

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.

Comment 2

[Matt]

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

Comment 3

[Robert Longson [:longsonr]]

http://hg.mozilla.org/mozilla-central/rev/cabb8925dcd3 is the cause. http://hg.mozilla.org/mozilla-central/rev/771509552a2e is the last working build.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Neil, can you take a look?

Comment 5

[Neil Deakin]

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?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 7

[Neil Deakin]

OK, I'll do some further testing.

Comment 8

[Neil Deakin]

Attachment: use 0 as default tabindex for <object>

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.

Comment 9

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

Needed, but I don't think it blocks the next release. If we get a reviewed and baked patch, we'll approve it.

Comment 10

[Neil Deakin]

Attachment: add test

Comment 11

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/b8c6a4f81a58

Comment 12

[Mats Palmgren (inactive)]

Additionally: http://hg.mozilla.org/mozilla-central/rev/5a0eb7abe752

Comment 13

[LegNeato]

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.

Comment 14

[Mounir Lamouri (:mounir)]

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...

Comment 15

[Mounir Lamouri (:mounir)]

Bugs for IsHTMLFocusable and GetTabIndex: bug 597242 and bug 597241

Comment 16

[Neil Deakin]

Can you explain the alternative way you plan on fixing this bug?

Comment 17

[Mounir Lamouri (:mounir)]

(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.