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

VERIFIED FIXED in M12

Status

()

Core
Layout: Form Controls
P3
normal
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: Akkana Peck, Assigned: buster)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PDT+] 12/17 Not checked into M12 branch yet, would like it!)

Attachments

(4 attachments)

(Reporter)

Description

18 years ago
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.

Updated

18 years ago
Target Milestone: M12
(Assignee)

Updated

18 years ago
Summary: first click doesn't get through to text field → [dogfood] first click doesn't get through to text field
(Assignee)

Comment 1

18 years ago
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.

Updated

18 years ago
Whiteboard: [PDT+]

Comment 2

18 years ago
Putting on PDT+ radar.
(Assignee)

Updated

18 years ago
Whiteboard: [PDT+] → [PDT+] 12/17 estimated time to fix
(Assignee)

Updated

18 years ago
Blocks: 18500
Status: NEW → ASSIGNED
Whiteboard: [PDT+] 12/17 estimated time to fix → [PDT+] fix in hand. will get code review tomorrow.
(Assignee)

Comment 3

18 years ago
chris, permission to check in tomorrow afternoon, assuming the code review goes
well? This also fixes 18500.
(Assignee)

Updated

18 years ago
Blocks: 20192

Comment 4

18 years ago
how big and where is the fix?
we might want to take this on the m12 branch
(Assignee)

Comment 5

18 years ago
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.
(Assignee)

Comment 6

18 years ago
Created attachment 3539 [details] [diff] [review]
patch
(Assignee)

Comment 7

18 years ago
Created attachment 3540 [details]
changed header file
(Assignee)

Comment 8

18 years ago
Created attachment 3541 [details]
chnaged source file
(Assignee)

Comment 9

18 years ago
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.
(Assignee)

Comment 10

18 years ago
Created attachment 3542 [details]
a slightly improved fix
(Assignee)

Comment 11

18 years ago
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

18 years ago
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

18 years ago
buster: yes. go for it on the trunk. thanks
(Assignee)

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Whiteboard: [PDT+] fix in hand. will get code review tomorrow. → [PDT+]
(Assignee)

Comment 14

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

Comment 15

18 years ago
Could bug# 22023 be related to this?

Updated

18 years ago
Status: RESOLVED → REOPENED
Target Milestone: M13 → M12

Comment 16

18 years ago
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.

Updated

18 years ago
Resolution: FIXED → ---
Whiteboard: [PDT+] → [PDT+] 12/17 Not checked into M12 branch yet, would like it!
(Assignee)

Updated

18 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 17

18 years ago
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

18 years ago
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.

Updated

18 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED

Comment 19

18 years ago
this is all checked in and fixed, marking resolved and verified

Updated

18 years ago
Status: RESOLVED → VERIFIED

Comment 20

18 years ago
verified with m12 candidate build from 12/20
You need to log in before you can comment on or make changes to this bug.