245349 - The null plugin's "Click here to get the plugin" not accessible

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Aaron Leventhal]

Steps:
1. Make sure Flash is not installed.
2. Go to a page with a Flash animation, e.g.
http://home.twcny.rr.com/jcurry/flslots.html
3. Load API testing tools (MSAA inspect)
4. Try to tab to "Click here to get the plugin" button.
 *  What happens: one tab key swallowed, but focus goes nowhere. No focus
visible and no focus change reported in MSAA Inspec
5. Make mouse pointer hover over "click here to get the plugin" button.
 *  What happens: no button object apparent, NullPluginClass has no child objects

Expected: 
1) focus displayed on button, 
2) accessibility API focus events for button fired 
3) button object exposed via MSAA / ATK

Jessie Li -- can you check to see what happens under ATK? Is this bug
Windows-only or should it be all/all?

Comment 1

[Aaron Leventhal]

Another thing we need to make sure that works is hitting Enter or space bar on
the "Click here to get the plugin" button.

Bug 83574 is one way of fixing this bug.

Comment 2

[Aaron Leventhal]

Comments from IRC:
Neil wonders if object, embed { -moz-user-focus: normal; } would help.
Aaronlev and mkaply note that when you tab to the plugin, it does go somewhere.
Focus disappears but then you can tab out.
Doron says natrually, we could make the embed/object point to an chrome:// xul
file probably
bombay ;-)
Aaronlev notes that the XBL plugin bug does have some old patches in it. It
would be useful to fix that for i18n, right now we always give the message in
English, apparently.
Doron says their may be difficulties getting aorund security if we do it with XBL.

Comment 3

[Aaron Leventhal]

Attachment: 1) Insert <a> w/ display:block instead of <div> around puzzle piece & message, 2) rename vars containing 'div' to 'anchor' 3) don't make object frame itself focusable when it's broken,

Comment 4

[Aaron Leventhal]

Using an <a> instead of a <div> as the nice side effect of allowing the "click
here to install plugin" work with screen readers. They know how to deal with the
anchor.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 178949 [details] [diff] [review]
1) Insert <a> w/ display:block instead of <div> around puzzle piece & message, 2) rename vars containing 'div' to 'anchor' 3) don't make object frame itself focusable when it's broken,

sr=jst

Comment 6

[Aaron Leventhal]

Comment on attachment 178949 [details] [diff] [review]
1) Insert <a> w/ display:block instead of <div> around puzzle piece & message, 2) rename vars containing 'div' to 'anchor' 3) don't make object frame itself focusable when it's broken,

One change I'll add when I check-in is
text-decoration: none for the link style.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 178949 [details] [diff] [review]
1) Insert <a> w/ display:block instead of <div> around puzzle piece & message, 2) rename vars containing 'div' to 'anchor' 3) don't make object frame itself focusable when it's broken,

>Index: layout/generic/nsObjectFrame.cpp
>+nsObjectFrame::IsFocusable(PRInt32 *aTabIndex, PRBool 
>+  return nsFrame::IsFocusable(aTabIndex, aWithMouse);

s/nsFrame/nsObjectFrameSuper/, please.

>+  anchor->SetAttr(kNameSpaceID_None, nsHTMLAtoms::href, NS_LITERAL_STRING("#"), PR_TRUE);

Er... That will cause clicking on the image in this anchor to scroll to the top
of the page, no?  That doesn't really seem to be desirable.

>   img->SetAttr(kNameSpaceID_None, nsHTMLAtoms::style,
>-               NS_LITERAL_STRING("display: block; width: 32px; height: 32px;"),
>+    NS_LITERAL_STRING("display: block; border: 0px; width: 32px; height: 32px;"),
>                PR_FALSE);

Please fix indent of PR_FALSE to be indented like the NS_LITERAL_STRING.

Comment 8

[Aaron Leventhal]

Attachment: Address bz's comments. Whitespace issued dealt with by adding NS_NAMED_LITERAL_STRING

Comment 9

[Boris Zbarsky [:bzbarsky]]

But do accessibility tools still see the anchor as an anchor with the href="#"?

Comment 10

[Aaron Leventhal]

(In reply to comment #9)
> But do accessibility tools still see the anchor as an anchor with the href="#"?

Yes they still see it as an anchor.

Comment 11

[Aaron Leventhal]

Comment on attachment 179022 [details] [diff] [review]
Address bz's comments. Whitespace issued dealt with by adding NS_NAMED_LITERAL_STRING

r=bz on IRC

Comment 12

[Aaron Leventhal]

Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.553; previous revision: 3.552
done
Checking in layout/generic/nsIFrame.h;
/cvsroot/mozilla/layout/generic/nsIFrame.h,v  <--  nsIFrame.h
new revision: 3.282; previous revision: 3.281
done
Checking in layout/generic/nsObjectFrame.cpp;
/cvsroot/mozilla/layout/generic/nsObjectFrame.cpp,v  <--  nsObjectFrame.cpp
new revision: 1.493; previous revision: 1.492
done
Checking in layout/generic/nsObjectFrame.h;
/cvsroot/mozilla/layout/generic/nsObjectFrame.h,v  <--  nsObjectFrame.h
new revision: 1.38; previous revision: 1.37
done

Comment 13

[Aaron Leventhal]

This caused bug 288587 and possibly bug 288523

Comment 14

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381