21612 - [dogfood] first click doesn't get through to text field

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[Akkana Peck]

Start up a new browser window.  Select/copy something somewhere else (e.g. from
another app).  Place the mouse over the urlbar and middle-mouse paste.

Result: the urlbar lazily instantiates its webshell (flashes grey and redraws)
and takes focus, but the paste doesn't happen -- the middle click doesn't get
through to the editor's event listener.  Text fields need to pass mouse events
through after they've lazy-instantiated themselves.

Middle-mouse paste seems broken on windows (it does something else) but you can
see this on Windows by trying a double-click in the urlbar.  It should do a word
select, but instead it just does a single click since the first click doesn't
get through.

Comment 1

[buster]

nominating for dogfood.
the problem is that the text control frame isn't passing initial mouse click
(the one that grants it focus) down to the editor.  So although the frame does
all the "I got focus" work properly, the editor doesn't see the click and
therefore doesn't do any of the right things, like place the caret correctly,
paste, double-click select, drag-select, etc.  I've got 4-5 variations of this
bug that all agree this is really annoying.

Comment 2

[leger]

Putting on PDT+ radar.

Comment 3

[buster]

chris, permission to check in tomorrow afternoon, assuming the code review goes
well? This also fixes 18500.

Comment 4

[chris hofmann]

how big and where is the fix?
we might want to take this on the m12 branch

Comment 5

[buster]

the fix is a few dozen lines of pretty safe code, isolated to the text control
frame.  what I've done is added event handling that takes mouse events targeted
at the html text control and redispatches them to the embedded sub document.
This fixes 2 problems:  mouse handling was broken in the case where no
subdocument existed yet (recall this was a badly needed optimization to lazily
instantiate the subdocument), and mouse events that fell in the region between
the text control frame outer edge and the subdocument bounds were lost (this is
the area taken up by any border and padding on the <input> or <textarea> tag.)
I'll attach the diffs momentarily.  I'll get some mac and linux people to run
with my diffs before checking in as well.

Comment 6

[buster]

Attachment: patch

Comment 7

[buster]

Attachment: changed header file

Comment 8

[buster]

Attachment: chnaged source file

Comment 9

[buster]

I've attached a cvs diff -c patch, as well as the full .h and .cpp.  The diff is
not as scary as it looks, because it includes:
1) the fix, primarily changes to nsGfxTextControlFrame::HandleEvent and a new
method  nsGfxTextControlFrame::RedispatchMouseEventToSubDoc

2) some clean-up, because there have been lots of hands in this code recently:
a) global methods (which should be static class methods, but that's a job for
another day) moved to the top of the file
b) found some places where return codes were not being checked
c) changed a couple of variable names to make sense

3)some M13 fixes have been commented out, but of course they show up in the
diff.

Comment 10

[buster]

Attachment: a slightly improved fix

Comment 11

[buster]

this is now tested on WinNT (me and Kin), Mac (me and donc), and Linux(akkana).
r=rickg, with lots of good help from kin.
It solves what it was designed to solve, the problem of the first click being
handled correctly.  It does not address various other focus issues that akkana,
saari, et. al. are dealing with today.  But it has no negative impact on these
issues either.

chofmann:  permission to check in tonight?

Comment 12

[chris hofmann]

final m12 candidates are spinnning now. moving to m13.
if we fall off track and need to respin m12 for some
yet unknown reason we can consider this if you get
a fix in hand.

Comment 13

[chris hofmann]

buster: yes. go for it on the trunk. thanks

Comment 14

[buster]

fixed.  a=chofmann, r=rickg.  NOT part of M12, unless they do a respin.

Comment 15

[Stephen Anderson]

Could bug# 22023 be related to this?

Comment 16

[Paul MacQuiddy]

I have verified that this is fixed with M13.

However, I am going to re-open to put on M12 radar, because it looks like there
will have to be a re-spin, and I would like to see this get in for that.

Sorry for re-opening, I don't know how else to get this on M12 radar.

Comment 17

[buster]

chofmann, jar: do you want this on the branch?  You can just migrate
nsGfxTextControlFrame.h and nsGfxTextControlFrame.cpp in
mozilla/layout/html/forms/src, or let me know and I'll do it.  If you want me to
do it, please include specific instructions, it's been a while since I've messed
with cvs branches.  Thanks!

Comment 18

[Neil Marshall]

Single click now gets through in build 1999121915.

A double click, on the other hand, doesn't.  Its supposed to select the entire
URL and it still takes one initial click (Which is this bug) and then a double
click to select everything.

Comment 19

[Paul MacQuiddy]

this is all checked in and fixed, marking resolved and verified

Comment 20

[Paul MacQuiddy]

verified with m12 candidate build from 12/20