310626 - Flash text input broken

Status: RESOLVED FIXED

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

Description

[Josh Aas]

Flash text input seems to be broken.

Reproduce:
1. Load flash with text input
2. Click in text input area
3. Type

Actual:
No text appears in text box

What should happen:
Text appears in text box

Comment 1

[Marcia Knous [:marcia]]

I tested this using today's trunk build (Mozilla/5.0 (Macintosh; U; PPC Mac OS X
Mach-O; en-US; rv:1.9a1) Gecko/20050930 Firefox/1.6a1) and am unable to input
any text into the text area on the Macromedia site.

Comment 2

[Simon Fraser [no longer active]]

Did bug 298961 regress this? I didn't think that it changed anything
focus-related. What build did this regress in?

Comment 3

[Mark Mentovai]

Regressed on the trunk between 092706 and 092806, and on the branch between
092906 and 093005.

http://bonsai.mozilla.org/cvsquery.cgi?branch=HEAD&date=explicit&mindate=2005-09-27+05%3A00&maxdate=2005-09-28+07%3A00
http://bonsai.mozilla.org/cvsquery.cgi?branch=MOZILLA_1_8_BRANCH&date=explicit&mindate=2005-09-29+05%3A00&maxdate=2005-09-30+07%3A00

This was caused by bug 309704.  Backing that patch out restores typing in the
Flash plugin.

Comment 4

[Aaron Leventhal]

Is it only broken on Mac? When I tested I had no problem, but I did not test on Mac.

Comment 5

[Aaron Leventhal]

We obviously better back out bug 309704 :(

Comment 6

[Aaron Leventhal]

Maybe if we just put all of the IsFocusable() methods back in then we can fix
both bug 309704 and reverse this regression.

Comment 7

[Aaron Leventhal]

Attachment: Needs testing

Comment 8

[Aaron Leventhal]

I believe I know a low risk fix for this. We need to put a simple IsFocusable()
impl for several plugin-related classes.

The new default impl for tabindex to -1 makes them not focusable.

Comment 9

[Aaron Leventhal]

Attachment: Needs testing #2

Comment 10

[Aaron Leventhal]

Works in 2/23
Broken in 7/29 (so splitwindow is not the culprit)

Comment 11

[Mark Mentovai]

Comment on attachment 198104 [details] [diff] [review]
Needs testing #2

This one does the trick, except:

Index: content/html/content/src/nsHTMLAppletElement.cpp
===================================================================
+    { if (aTabiIndex) *aTabIndex = -1; return PR_TRUE; }

should be aTabIndex both times.

Comment 12

[Aaron Leventhal]

(In reply to comment #10)
> Works in 2/23
> Broken in 7/29 (so splitwindow is not the culprit)

Commented in wrong bug.

Comment 13

[Aaron Leventhal]

Attachment: 1) Always return PR_TRUE for IsFocusable, 2) In aTabIndex out param, return tabindex if manually set in attribute

Comment 14

[Adam Guthrie]

*** Bug 310579 has been marked as a duplicate of this bug. ***

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 198105 [details] [diff] [review]
1) Always return PR_TRUE for IsFocusable, 2) In aTabIndex out param, return tabindex if manually set in attribute

In a tree updated minutes ago I cannot reproduce the bug[1].
If I apply the patch it regresses the testcase that follows.
Do I need to have some other patch as well?

[1] well, I can enter text, but if I first click the flash
text input enter som text and then click the HTML input
above it I see two carets and the keyboard input goes to the
input I hover the mouse over... I assume this is a separate
bug though...

Comment 16

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Clicking on the text should not show a focus outline.

Comment 17

[Mats Palmgren (inactive)]

> I assume this is a separate bug though...

... because I see that with or without your patch.

Comment 18

[Aaron Leventhal]

Mats, I think the bug only shows up on Mac OS X.

Comment 19

[Aaron Leventhal]

Mats, I'm not sure how to fix that testcase. But personally I could live with it
for the release. How can we tell when the object contents are from a plugin or
are rendered by Gecko? I guess that should determine what IsFocusable() returns.

Comment 20

[Adam Guthrie]

The reason I marked this as OS: All is because I can reproduce this on the
following page: http://www.croixmarie.com/croixmarie.html (Click the Contact
link on the right). I verified that the input works in a 2005-09-26 build
(before bug 309704 was checked in).

Comment 21

[Boris Zbarsky [:bzbarsky]]

> How can we tell when the object contents are from a plugin or
> are rendered by Gecko?

On branch, we can interrogate the nsIObjectFrame, if any.  On trunk, we can just
look at our current type.

Comment 22

[Masatoshi Kimura [:emk]]

(In reply to comment #15)
> [1] well, I can enter text, but if I first click the flash
> text input enter som text and then click the HTML input
> above it I see two carets and the keyboard input goes to the
> input I hover the mouse over... I assume this is a separate
> bug though...
That's bug 310174.
The fix for bug 309704 caused bug 306209 and bug 310174 again.
Arron's patch for this bug (310626) also fix their regressions.

Comment 23

[Aaron Leventhal]

(In reply to comment #21)
> > How can we tell when the object contents are from a plugin or
> > are rendered by Gecko?
> 
> On branch, we can interrogate the nsIObjectFrame, if any.  On trunk, we can just
> look at our current type.

Do you mean check to see if nsIObjectFrame::GetPluginInstance returns null?

Is it worth taking such such a change for branch, vs. just making alternative
object content focusable by mouse click?

Comment 24

[Aaron Leventhal]

Attachment: Test case for <object> which is not a plugin

Comment 25

[Aaron Leventhal]

Attachment: Handles cases where <object> has not loaded plugin content

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 198265 [details] [diff] [review]
Handles cases where <object> has not loaded plugin content

This is the same as the last patch, did you attach the wrong file?

Comment 27

[Aaron Leventhal]

Attachment: Patch for branch. Handles cases where <object> has not loaded plugin content

Comment 28

[Aaron Leventhal]

Attachment: Patch for trunk. Handles cases where <object> has not loaded plugin content

Comment 29

[Boris Zbarsky [:bzbarsky]]

I'm not likely to be able to get to this until at least Wednesday.  Probably later.

Comment 30

[Mats Palmgren (inactive)]

Comment on attachment 198267 [details] [diff] [review]
Patch for branch. Handles cases where <object> has not loaded plugin content

nsHTMLObjectElement::IsFocusable looks fine to me, but don't we
need to fix <embed> and <applet> in much the same way?

This patch is looks much like my (obsolete) patch for bug 279378,
which had code for that. There are some testcases there you
might find useful too.

Comment 31

[Mats Palmgren (inactive)]

Is the file nsHTMLSharedObjectElement.cpp really used by anyone?

Comment 32

[Masatoshi Kimura [:emk]]

(In reply to comment #31)
That's a work-in-progress file created on bug 236873.

Comment 33

[Aaron Leventhal]

(In reply to comment #30)
> (From update of attachment 198267 [details] [diff] [review] [edit])
> This patch is looks much like my (obsolete) patch for bug 279378,
> which had code for that. There are some testcases there you
> might find useful too.


We need to move on this review. We're closing Tuesday night? Any comments? 

Comment 34

[Jay Patel [:jay]]

When was this checked into the Trunk?  I can verify that the test case in
comment #16 and comment #24 work fine with today's Trunk build:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051003
Firefox/1.6a1

But, the test case in comment #20 is still broken.  If those results are good
enough for now, we should try to get this onto the branch before the tree closes
tonight.

Comment 35

[Masatoshi Kimura [:emk]]

Jay:
My patch attached in bug 310174 will solve the comment #20 problem.
I think it's no problem landing attachment 198267 [details] [diff] [review] onto the branch because the
patch for bug 310174 only landed on trunk.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Why is the <object> case not just calling the superclass when it's not a plugin?
 nsGenericHTMLElement::IsFocusable does something quite different from what this
patch implemented for <object>; is there a reason for the difference?