494267 - Cmd+A selects all text on page when FlashPlayer has keyboard focus

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jeff Mott]

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; .NET CLR 2.0.50727; .NET CLR 3.0.04506.648; .NET CLR 3.5.21022; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729)
Build Identifier: All - FF2, FF3

See Summary and repro steps.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.flashdesignerzone.com/tutorials/t1011.php or any other page that has mixed HTML and Flash, where the Flash has input text.  Use any version of FF with any version of Flash Player.
2. Click to put focus on input text, type some text.
3. Press Cmd+A to select all
Actual Results:  
Text in the text field is selected, but so is all the text on the page

Expected Results:  
Only the text in the text field should be selected

Works correctly in Safari, and even in FF with other hot keys like Cmd+C, so I don't think it's a player problem.

P.S.  I've been told before that I should file these bugs with Product=Core and Component=Plug-ins, but I've never been able to find out *how* to do that, those aren't options on this page.

Comment 1

[Samuel Sidler (old account; do not CC)]

(In reply to comment #0)
> P.S.  I've been told before that I should file these bugs with Product=Core and
> Component=Plug-ins, but I've never been able to find out *how* to do that,
> those aren't options on this page.

There's an "Other Applications" option and a "Core" option under that.

Comment 2

[Marcia Knous [:marcia]]

Confirming, I see this using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090522 Shiretoko/3.5pre.

As Jeff states in Comment 0 this does work using Safari.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

This is definitely a Firefox bug, and a very old one.

You also see it with my DebugEventsPlugin from bug 441880 (if you
alter its test page (test.html) by adding text above and below the
plugin).  It happens in every current version of Firefox 3.0.X and
3.5.X, and also in FF 2.0.0.20.

It won't be easy to fix properly.  Or to put this another way, the
only plausible short-term fix is an ugly hack :-(

On the 1.9.0/1.9.1/1.9.2 branches, this regressed with the patch for
bug 428047, which allowed many cmd+key combinations to "work" (in the
browser) even when the keyboard focus is in a plugin (e.g. cmd-q and
cmd-t).  We probably won't be able to roll back this patch.

The alternative is to make exceptions to the behavior introduced in
the patch for bug 428047.  The list of exceptions should (I think)
definitely include cmd-a.  Maybe it should also include other cmd+key
combinations.

What do you think, Josh?

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Here's a patch for this bug.

It implements the exclusion list I mentioned in comment #3.  So far
the list only contains one item -- cmd-a.

Axel Hecht helped me to find out how to search the Mozilla tree for
all locales at once (you can do this using MXR at
http://mxr.mozilla.org/l10n-central/).  With a little trial-and-error
I discovered that what a particular cmd+key means to the browser when
the keyboard focus is in a plugin is determined by the following line
in browser.dtd:

  <!ENTITY selectAllCmd.key         "A">

Using the following MXR search, you can see for yourself that the
cmd+key combination for "Select all" (when the keyboard focus is in a
plugin) is currently the same in all locales (cmd-a).  I suspect it's
likely to stay this way.

http://mxr.mozilla.org/l10n-central/search?string=selectAllCmd.key&find=browser.dtd

An exclusion list is still an ugly solution.  But the fact that we
don't have to worry about localization for cmd-a makes me feel better
about using it.  And I still can't think of a better short-term
solution (one that makes sense to land on the 1.9.2 branch).

A tryserver build will follow in a few hours.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Sigh.  I just noticed that the pt-PT localization has the following:

  <!ENTITY selectAllCmd.key "t">

I don't know how I missed this.

But I also just downloaded the pt-PT localization of FF 3.5.2, and on
it cmd-a still does "select all" and cmd-t still does "new tab" (at
least when the focus isn't in a plugin).  When the focus *is* in a
plugin, cmd-t selects all text in a browser page (and does nothing in
a Flash plugin) (with or without my patch, presumably).

I'll look into this further, but I don't think this should block my
current patch.  As best I can tell, it won't make things any worse in
the pt-PT locale.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my patch from comment #4:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla494267/bugzilla494267-macosx.dmg

Of course it contains just the en-US locale.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 393804 [details] [diff] [review]
Fix

I think I've found a way to localize my patch!

I should be able to post a new one tomorrow.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (localized)

Here's a localized version of my patch.

An en-US tryserver build should follow in a few hours.

And if I can figure out how, I'll also do a pt-PT tryserver build.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

> And if I can figure out how, I'll also do a pt-PT tryserver build.

As best I can tell this isn't possible.

I've made (and tested with) a local pt-PT build (on the 1.9.1 branch).
If anyone's interested, I can put a copy up on my people.mozilla.com
account and post a link here.

My new patch (from comment #8) does fix this bug in the pt-PT build --
cmd-t no longer selects text outside a plugin when the keyboard focus
is in the plugin.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Here's the en-US tryserver build made with my rev1 patch from comment #8:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla494267-rev1/bugzilla494267-rev1-macosx.dmg

Comment 11

[Josh Aas]

Before committing to something like this we should figure out what WebKit does to decide who handles the key command. They should have the same problem we do so it would be good to figure out how they handle it. Safari 4 does not have this problem.

Comment 12

[Josh Aas]

Reversing blocking decision, we'd obviously like to fix this but it is not a regression.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

This bug is clearly a consequence of the patch for bug 428047, so I'm
not sure what the point is of looking at what WebKit does.

But I'll take a look anyway -- specifically at what WebKit (and/or
Safari) does with cmd+key combinations while the focus is in a plugin.

Comment 14

[Josh Aas]

(In reply to comment #13)
> This bug is clearly a consequence of the patch for bug 428047, so I'm
> not sure what the point is of looking at what WebKit does.

WebKit should have the same problem that forced us to take the patch on bug 428047. The point is that they somehow solved the problem without consequences like this bug. We should find out what they did so we know if the solution in this bug is necessary or if there is essentially a better alternative to the patch that caused this bug, one that will not cause this bug. Maybe they did exactly what the patch on this bug does, I don't know, but I want to know.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

By looking at WebKit code and running Safari in gdb, I've figured out
what happens when you press cmd-a in this bug's example:

1) [WebHTMLView performKeyEquivalent:] is called from OS code
   (specifically from [NSWindow performKeyEquivalent:], which calls
   each of it's NSViews' performKeyEquivalent: methods in turn).

   WebHTMLView is an NSView subclass that appears to correspond to a
   web page.

2) [WebHTMLView performKeyEquivalent:] calls super (i.e. [NSView
   performKeyEquivalent:], which returns '0' (i.e. 'false').

3) [WebHTMLView performKeyEquivalent:] returns '0' (i.e. 'false').

4) [WebBaseNetscapePluginView selectAll:] is called from OS code
   (specifically from [NSMenu performKeyEquivalent:]), and does what's
   needed when cmd-a is pressed while the keyboard focus is in a
   plugin.

In short, the WebKit (and Safari) basically let the OS do what it
wants, and the right thing happens more or less by magic.

I tried several different ways to get the same thing to happen in FF,
but all failed.  (Among other things, I called [super
performKeyEquivalent:] and returned '0' from [ChildView
performKeyEquivalent:] (while the focus was in a plugin), after having
given ChildView a selectAll: method -- the selectAll: method never got
called.)

I don't think WebKit will help us with this bug.

Comment 16

[Josh Aas]

Comment on attachment 394130 [details] [diff] [review]
Fix rev1 (localized)

This isn't the right way to fix this, it just make the situation more confusing and volatile.

WebKit isn't using "magic," we need to figure out what is actually going on there and how it relates to what we are doing. In the long run the advanced key handling spec is the answer here, in the mean time lets figure out the specifics.

http://trac.webkit.org/browser/trunk/WebKit/mac/Plugins/WebBaseNetscapePluginView.mm

It looks to me like they do let the menu bar handle the key event but it doesn't handle it the way we do, it sends selectAll: to the view like a normal Cocoa application and then the view sends a hardcoded cmd-a event to the plugin. This would send the wrong key code in any locale that doesn't use cmd-a for select all, but the point is that they are not handling the actual key event. The plugin is simply informed of an intended operation, select all, then makes an assumption about the event.

Comment 17

[Josh Aas]

Comment on attachment 394130 [details] [diff] [review]
Fix rev1 (localized)

Actually, the more I think about it the more I think this patch might be the right approach. I just didn't like the explanation given for it at the time. Requesting review from myself again so I can revisit it.

Comment 18

[Josh Aas]

To be clear, NPAPI does not allow for a "right" approach here, actually. There is no way to tell if a plugin handled an event, and plugins want all key events. That is why we need the advanced key handling spec in the long run. The approach taken in Steven's patch might be the best we can do in the mean time though, and in fact it is not very different from what WebKit does. It might even be better since it doesn't involve hard-coding the event.

Comment 19

[Josh Aas]

Comment on attachment 394130 [details] [diff] [review]
Fix rev1 (localized)

Lets take this, thanks Steven. Can you add "key_copy" and "key_paste" to that list before landing and post the modified patch here? Might as well cover those bases too, WebKit does.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

I just landed my patch (updated) on the trunk:
http://hg.mozilla.org/mozilla-central/rev/deb6e0b6f4f0

> Can you add "key_copy" and "key_paste" to that list before landing
> and post the modified patch here? Might as well cover those bases
> too, WebKit does.

But I didn't do this.

"key_copy" and "key_paste" already work correctly -- they get sent to
the plugin if the focus is there, and otherwise not.  And there's a
lot of hidden complexity here (not least in what the various plugins
expect us to do).  So I'd rather not "fix" something that isn't
broken.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 394130 [details] [diff] [review]
Fix rev1 (localized)

We should probably let this bake on the trunk for about a week ... but
I assume approval will take at least that long :-)

Comment 22

[Steven Michaud [:smichaud] (Retired)]

Landed on the 1.9.2 branch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3abef873e69f