279378 - OBJECT should not be IsFocusable() when replaced by alternate content

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

OBJECT should not be IsFocusable() when replaced by alternate content.

STEPS TO REPRODUCE:
1. Load URL
2. type ALT-g

ACTUAL RESULTS:
The <object> receives focus - TAB once to get to "Font size".

EXPECTED RESULTS:
"Font size" <select> element should receive focus on ALT-g.

(Note that the URL depends on bug 81481 being fixed - testcase coming up
that does not have that dependency).

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 2

[Gérard Talbot]

Attachment: Reduced testcase from original page

Instructions: <alt>+g should (expected results) transfer focus to the next
focusable element in the subtree which is the select

Actual results in Mozilla 1.8b build 2005012005:
Focus is captured by an <object>

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note that if the <object> is being shown (eg if it's a plugin), it _should_ get
focus.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase (alternative content)

Comment 5

[Mats Palmgren (inactive)]

Attachment: Testcase (Flash content)

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Something like this perhaps...

Comment 7

[Mats Palmgren (inactive)]

Does anyone know of a URL using Flash that actually have a textfield (or
something that receives focus) inside the Flash content?

Comment 8

[Aaron Leventhal]

(In reply to comment #7)
> Does anyone know of a URL using Flash that actually have a textfield (or
> something that receives focus) inside the Flash content?

Yes, here's a very cool example: http://marketrac.nyse.com/mt/

Comment 9

[Mats Palmgren (inactive)]

(In reply to comment #8)
> Yes, here's a very cool example: http://marketrac.nyse.com/mt/

Very cool, but it only works in IE on Windows unfortunately. Even playing around
with UA strings etc won't get past their "browser detection":
http://marketrac.nyse.com/mt/detector/detector.js

Any simpler example? any kind of "form control" inside a Flash object will do,
I just want to verify that I didn't break keyboard navigation into the plugin.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Mats, bug 257488 and its dups and dependencies should have some testcases.

Comment 11

[Mats Palmgren (inactive)]

Thanks Boris, it seems this is already broken and the attached patch doesn't
make a difference in this respect.

I tried http://www.macromedia.com/ -
try tabbing to the search field at the top of the page, the focus is transfered
to the plugin (and get stuck there) but the items inside it never receives
focus. After clicking anyware on the plugin I can tab around inside it.

I also tried the testcase https://bugzilla.mozilla.org/attachment.cgi?id=171966
click Play, then wait for the animation to stop, now select the text "Object"
on the page and then TAB - again the plugin receives focus but not any item
inside it.  This seems to be a separate problem (I'm expecting that when
the plugin receives focus it should transfer it to the first focusable
item inside it).

Comment 12

[Mats Palmgren (inactive)]

Attachment: Testcase #3 (ALT for <object image/gif>)

Comment 13

[Mats Palmgren (inactive)]

Attachment: Testcase #4 (ALT for <object text/html>)

Comment 14

[Mats Palmgren (inactive)]

(The stuck focus on plugins is bug 93149.)

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (also fixes bug 284610)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176344 [details] [diff] [review]
Patch rev. 2 (also fixes bug 284610)

This really needs review from aaronlev, I think...

>Index: content/html/content/src/nsHTMLAppletElement.cpp

>+  // <applet> is not focusable if it was replaced by alternative content.

Even if it has explicit tabindex set?  Wouldn't we allow a div to be focusable
in that case, say?  Why not an applet?	When there is no plugin it should act
_exactly_ like div, imo.

>Index: content/html/content/src/nsHTMLObjectElement.cpp

>+    // If the frame isn't a nsIObjectFrame then it's a container frame for
>+    // alternative content and it is not focusable.

Again, why does this behavior differ from that for <div>?

>Index: content/html/content/src/nsHTMLSharedElement.cpp

Same issue here.

>Index: layout/generic/nsIObjectFrame.h

>+  NS_IMETHOD IsContentFocusable(PRBool* aResult) = 0;

Documentation?

>Index: layout/generic/nsObjectFrame.cpp

>+nsObjectFrame::IsContentFocusable(PRBool* aResult)
>+{
>+  NS_ENSURE_ARG_POINTER(aResult);

Feel free to assert this, but please don't ensure it.  People shouldn't be
passing null here.

>+  *aResult = mIsSupportedImage ? PR_FALSE :
>+               mIsSupportedDocument ? PR_TRUE :
>+                 mInstanceOwner != nsnull;

How about:

nsIFrame* child = mFrames.FirstChild();
*aResult = 
  (firstChild && firstChild->GetType() != nsLayoutAtoms::imageFrame) ||
  mInstanceOwner;

?  No need to store the booleans, then...

Also, what's the behavior we want when our child is the div created for the
"broken plugin" case?  If we don't want that focusable, then perhaps:

*aResult = 
  (firstChild && firstChild->GetType() == nsLayoutAtoms::subDocumentFrame) ||
  mInstanceOwner;

Comment 17

[Mats Palmgren (inactive)]

Comment on attachment 176344 [details] [diff] [review]
Patch rev. 2 (also fixes bug 284610)

> Even if it has explicit tabindex set?
> When there is no plugin it should act _exactly_ like div, imo.

I agree.

Comment 18

[Mats Palmgren (inactive)]

Bug 310626 will hopefully fix this ;-)

Comment 19

[Gérard Talbot]

I tried all 5 testcases with Seamonkey 1.5a rv:1.9a1 build 2005111109 and Firefox 1.5 rv:1.8 build 20051111 and, as far as I can see, except for attachment 176338 [details] which is not loaded in Firefox 1.5, this bug is FIXED.

Comment 20

[Mats Palmgren (inactive)]

In the tescases from bug 114641, e.g attachment 90614 [details], a focus outline
is displayed when you click on the alternate content.

Comment 21

[Gérard Talbot]

Yes. Good point!

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix for that

With all the fixing, backing out, etc, that happened, this never really got fixed for <applet> somehow...

Comment 23

[Boris Zbarsky [:bzbarsky]]

Checked in.