Closed Bug 310626 Opened 15 years ago Closed 15 years ago

Flash text input broken

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.8beta5

People

(Reporter: jaas, Assigned: aaronlev)

References

()

Details

(Keywords: verified1.8, Whiteboard: [ETA: drivers should decide whether it needs to be tested on trunk first. I don't believe it is risky and is much better than what we have now ])

Attachments

(3 files, 5 obsolete files)

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
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.
Flags: blocking1.8b5?
Did bug 298961 regress this? I didn't think that it changed anything
focus-related. What build did this regress in?
Assignee: sfraser_bugs → nobody
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.
Blocks: 309704
Severity: normal → critical
Component: Plug-ins → Keyboard: Navigation
Target Milestone: --- → mozilla1.8beta5
Is it only broken on Mac? When I tested I had no problem, but I did not test on Mac.
We obviously better back out bug 309704 :(
Maybe if we just put all of the IsFocusable() methods back in then we can fix
both bug 309704 and reverse this regression.
Assignee: nobody → aaronleventhal
Attached patch Needs testing (obsolete) — Splinter Review
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.
Attached patch Needs testing #2 (obsolete) — Splinter Review
Attachment #198101 - Attachment is obsolete: true
Works in 2/23
Broken in 7/29 (so splitwindow is not the culprit)
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.
(In reply to comment #10)
> Works in 2/23
> Broken in 7/29 (so splitwindow is not the culprit)

Commented in wrong bug.
Attachment #198104 - Attachment is obsolete: true
*** Bug 310579 has been marked as a duplicate of this bug. ***
OS: MacOS X → All
Hardware: Macintosh → All
Attachment #198105 - Flags: review?(bzbarsky) → review?(mats.palmgren)
Flags: blocking1.8b5? → blocking1.8b5+
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...
Attached file Testcase #1 (obsolete) —
Clicking on the text should not show a focus outline.
> I assume this is a separate bug though...

... because I see that with or without your patch.
Mats, I think the bug only shows up on Mac OS X.
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.
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).
> 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.
(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.
Blocks: 55751
(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?
Attachment #198188 - Attachment is obsolete: true
Attachment #198105 - Flags: superreview?(bzbarsky)
Attachment #198105 - Flags: review?(mats.palmgren)
Attachment #198105 - Attachment is obsolete: true
Attachment #198265 - Flags: superreview?(bzbarsky)
Attachment #198265 - Flags: review?(mats.palmgren)
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?
Attachment #198265 - Attachment is obsolete: true
Attachment #198265 - Flags: superreview?(bzbarsky)
Attachment #198265 - Flags: review?(mats.palmgren)
Attachment #198267 - Flags: superreview?(bzbarsky)
Attachment #198267 - Flags: review?(mats.palmgren)
Attachment #198268 - Flags: superreview?(bzbarsky)
Attachment #198268 - Flags: review?(mats.palmgren)
I'm not likely to be able to get to this until at least Wednesday.  Probably later.
Attachment #198267 - Flags: superreview?(bzbarsky) → superreview?(jst)
Attachment #198268 - Flags: superreview?(bzbarsky) → superreview?(jst)
Whiteboard: [ETA: need reviews ]
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.
Is the file nsHTMLSharedObjectElement.cpp really used by anyone?
Blocks: 279378
(In reply to comment #31)
That's a work-in-progress file created on bug 236873.
(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? 

Attachment #198267 - Flags: superreview?(jst)
Attachment #198267 - Flags: superreview+
Attachment #198267 - Flags: review?(mats.palmgren)
Attachment #198267 - Flags: review+
Attachment #198268 - Flags: superreview?(jst)
Attachment #198268 - Flags: superreview+
Attachment #198268 - Flags: review?(mats.palmgren)
Attachment #198268 - Flags: review+
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [ETA: need reviews ] → [ETA: testing on trunk before requesting approval ]
Attachment #198267 - Flags: approval1.8b5?
Whiteboard: [ETA: testing on trunk before requesting approval ] → [ETA: drivers should decide whether it needs to be tested on trunk first. I don't believe it is risky and is much better than what we have now ]
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.
Attachment #198267 - Flags: approval1.8b5? → approval1.8b5+
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.
Keywords: fixed1.8
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?
Keywords: fixed1.8verified1.8
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.