357736 - Firefox 2 and Flash: regression with caret animation in text fields

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Darin Fisher]

Firefox 2 and Flash: regression with caret animation in text fields

Something changed between Firefox 1.5 and 2 that causes Flash text field carets to no longer animate.

For example, go to site:
http://www.permadi.com/tutorial/flashTextUnderline/index.html

Scroll down the page to the blue rectangle titled "Input Text Field" click on the "Type something" text in blue you'll notice that the flash caret doesn't appear however, selecting one or more characters causes character selection to show up but clicking to show an insertion bar will never appear.  This same website works fine in FF 1.5

I don't have a regression range to go along with this bug :(

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061023 Minefield/3.0a1

1.9a1_2006072814 had still a cursor but not 1.9a1_2006072816.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #1)
> 1.9a1_2006072814 had still a cursor but not 1.9a1_2006072816

Seems to me like bug 271442 is the most likely cause, then.

Comment 3

[Martijn Wargers (dead)]

Attachment: flash for testcase

Comment 4

[Martijn Wargers (dead)]

Attachment: testcase

Comment 5

[Martijn Wargers (dead)]

Yeah, most likely a regression from bug 271442.

Comment 6

[graham sanderson]

Note, that if you click on text in the wmode=opaque input text field, then click anywhere in the first flash movie, then the caret appears in the second one. Clicking elsewhere on the page does not have this effect.

Comment 7

[graham sanderson]

Attachment: Modified testcase with 4 movies

This definitely seems to be a problem with Flash figuring out which movie has the focus.

You can click on test in either opaque movie and the caret will only be shown (and indeed will be shown) when the focus is on the second non-opaque movie.

I'm just hypothesizing, but if this is caused by our now returning the correct enclosing HWND for nsPluginInstancePeerVariable_NetscapeWindow rather than the document window (which we do so that non windowed flash in scrolling/floating DIVs can actually be clicked on at all), then we have the issue of the variable being used for two incompatible things.

This could presumably be fixed in flash without adding another variable, depending on what logic they are currently using to figure out who has the focus in the opaque case.

In the short term what we try to could do is to return the doccument window instead of the containing window if the two have the same origin (that way anything that worked before would work both for clicking and for caret), and the containing window in any other case (because presumably being able to interact with the movie at all is more important than it showing the caret).

That way we are still better than what we had before, and can figure out how to get Flash to both see correct coordinates and show a caret in an absolutely positioned or scrolling DIV.

Comment 8

[graham sanderson]

(sorry meant to finish the last paragraph with "later")

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That sounds good. Thanks for following up

Comment 10

[graham sanderson]

Note, since it isn't explicitly stated here, this only affects windowless flash plugins on Windows.

Comment 11

[graham sanderson]

Note can someone confirm that prior to the 271442 fix, there are actually problems with multiple movies (e.g. looking at the 4 movie test case).

Click around a little, and you end up with carets flashing in both opaque windows.

Comment 12

[graham sanderson]

In fact event the two movie test case shows problems on Firefox 1.5.0.6 also:

If you click in the first movie; caret AOK
Then click in the second movie; caret AOK
Click back in the first movie; caret AOK
Click in the second movie; NO CARET!

I bring this up because the new tentative patch I made does fix 271442 AND seems to preserve this existing broken behaviour - it seems the Flash plugin is generally a bit confused as to when to show the caret for windowless controls irrespective of what window we give it.

As said before, if we include the new fix then it should be possible for an Adobe fix to fix the problem correctly later, but it does make our definition of the netscapeWindow a bit convoluted; i.e.

"It's the containing document's window unless you are in a windowless plugin whose containing window's origin differs from that of the document's window in which case it is the containing window"

As I said this definition allows us to return the document window as before when it doesn't matter, but the containing window in the cases where it is necessary to correctly calculate mouse event coordinates.

I guess making as many things work as possible is the best solution for now, though presumably with an Adobe fix we could always return the containing window, and not do any code change on the Firefox side.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Clearing the blocking 1.8 flag since we've, uh, shipped .. :)

Comment 14

[graham sanderson]

good point ;-)

Is there anyone at Adobe/Macromedia in the loop; the point being that while I do have a fix for the regression; how it currently is (in Firefox 2.0) is arguably a more sensible definition for the HWND parameter.

Given that focus does seem to be broken somewhat for windowless flash plugins anyway (see my multiple flash movie posts above), if there was any hope of a patch to Flash, then presumably this issue would go away and we wouldn't have to put more Flash workaround code in here.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should take the regression fix, even if it's making things more complicated, and we can simplify things later if we get buy-in from Adobe.

Comment 16

[graham sanderson]

ok; i'll resync to latest Fx2.0 code do some more testing then post a patch

Comment 17

[graham sanderson]

Attachment: Potential 1.8 patch to resolve this issue

implementation as described above

Comment 18

[Jeff Mott]

Yes, please put your workaround in place, we're not in a position to release a patch for this any time soon.

When I do get a chance to work on this, is there a new NPVERS_ value that I can check to know that I'm in Firefox 2 or any other browser that supports the new code?  Not suggesting we create one just for this, rather I'm hoping one is already in place that I can piggyback on.

Comment 19

[graham sanderson]

Perhaps we could add two new properties (in addition to the existing "netscape window"...

one being "document window" which always returns that
one being "enclosing window" which always returns the closest window.

Of course all this is just a stop-gap until (as I'm guessing we do for Fx 3.0, we need a richer windowless interface).

Comment 20

[Johnny Stenback (:jst)]

(In reply to comment #18)
> When I do get a chance to work on this, is there a new NPVERS_ value that I can
> check to know that I'm in Firefox 2 or any other browser that supports the new
> code?  Not suggesting we create one just for this, rather I'm hoping one is
> already in place that I can piggyback on.

I don't think there's a version you can reliably piggyback on (might work for now but who knows what never versions of other browsers will do to update their plugin support etc). But a plugin can get at the browsers user agent string through NPN_UserAgen(), and that should be usable (even tho not necessarily 100% guaranteed to always represent reality).

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 244134 [details] [diff] [review]
Potential 1.8 patch to resolve this issue

I think we can simplify this by creating a new nonvirtual nsIFrame method:

nsIFrame* GetAncestorWithWidget() const;

simliar to GetAncestorWithView which already exists in nsIFrame.h/nsFrame.cpp. Then your patch in nsObjectFrame just has to
1) call GetAncestorWithWidget on the current frame
2) Call ancestorWithWidget->GetOffsetTo(GetPresContext()->PresShell()->GetRootFrame())
3) Return the document widget if the offset is (0,0), otherwise return ancestorWithWidget->GetWindow()

Comment 22

[graham sanderson]

OK; I'll make the change - I figured there probably would be some utility functions to help, but I'm not too familiar with the code... is there a good site for a source code XRef?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No problem, that's why I'm here.

http://lxr.mozilla.org

Comment 24

[Mike Connor [:mconnor]]

We do want to get this as soon as possible, but with the absence of a reviewed patch, we're not going to block on this for 1.8.1.1

Comment 25

[graham sanderson]

Attachment: Propose fix

I tried coding this using a new nsIFrame method that walked up the frame tree looking for those with a frame with a widget, but that didn't seem to work as I expected... while figuring out why, I discovered some existing methods which do the trick anyway.

Comment 26

[graham sanderson]

To clarify my previous post: what I meant to say is that in implementing the suggested getAncestorWithWidget, I was climbing the frame tree looking for a frame with a view with a widget.

For whatever reason (I'm not exactly sure how the frame structure and view structure differ in the case of scrollable views) this didn't appear to be equivalent to climbing the frame tree to the first view and then the view tree to the first view with a widget.

In any case, when considering doing the later, I decided to just find the view for the nearest widget to the plugin, and check its offset from the root view for the view manager (which and correct me if I'm wrong) would have the same origin as the containing document's widget. This didn't require adding any new methods.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 246641 [details] [diff] [review]
Propose fix

excellent!

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked into trunk.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 246641 [details] [diff] [review]
Propose fix

We may want this on branch --- not sure.

Comment 30

[Jay Patel [:jay]]

Comment on attachment 246641 [details] [diff] [review]
Propose fix

Approved for 1.8 branch, a=jay for drivers.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fixed on branch.

Comment 32

[Carsten Book [:Tomcat]]

Confirm Verified Fixed for 1.8.1.2 and trunk - tested with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.2pre) Gecko/2007011004 BonEcho/2.0.0.2pre and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a2pre) Gecko/2007011404 Minefield/3.0a2pre