Last Comment Bug 305985 - after clicking in blank content area, focus stays in some widgets when it should have left
: after clicking in blank content area, focus stays in some widgets when it sho...
Status: VERIFIED FIXED
: fixed1.8
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Aaron Leventhal
: Hixie (not reading bugmail)
Mentors:
: 308624 (view as bug list)
Depends on:
Blocks: 269318
  Show dependency treegraph
 
Reported: 2005-08-25 16:20 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2009-02-02 14:38 PST (History)
7 users (show)
asa: blocking1.8b5+
asa: blocking1.9a1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Simplified test case. Click in listbox, click to right of combo box, press down arrow (503 bytes, text/html)
2005-09-20 19:46 PDT, Aaron Leventhal
no flags Details
Remove unnecessary call to GetDocSelectionLocation (unrelated) and unecessary if conidtion for setting selection before current focused node (the fix) (2.24 KB, patch)
2005-09-20 21:38 PDT, Aaron Leventhal
mats: review-
Details | Diff | Review
Testcase #2 (180 bytes, text/html)
2005-09-21 17:38 PDT, Mats Palmgren (:mats)
no flags Details
Fix the regression part of the problem. Also fixes bug 308624. (1.29 KB, patch)
2005-09-22 06:51 PDT, Aaron Leventhal
mats: review+
bryner: superreview+
asa: approval1.8b5+
Details | Diff | Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-08-25 16:20:08 PDT
Steps to reproduce:
 1. load bug 268497
 2. make the window wide enough so that you can see past the right edge of
    the flags comboboxes
 3. click in the "CC:" listbox (so you select somebody's email address)
 4. click in the blank space to the right of the combobox to the right of
    the text "blocking1.9a1" (or whatever the flags will be sometime in the
    future)
 5. press the down arrow

Expected results:
 5. page scrolls down

Actual results:
 5. "CC" listbox scrolls down

Firefox Linux trunk, morning of August 25.

I'm not sure yet if this is a regression, but I think it is.
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-08-25 16:21:01 PDT
(Note that if you click on the "blocking1.9a1" *text*, it works as expected.)
Comment 2 Mats Palmgren (:mats) 2005-08-25 18:41:15 PDT
Bug also occurs in Mozilla 2005-04-30 on Linux so it's not a recent regression.
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-09-14 23:34:05 PDT
Like bug 308624, this is a regression between suite Linux GTK1 builds
2004-12-01-08-trunk and 2004-12-02-11-trunk.  (Although in the latter, the
scrolling within the listbox was even wackier than it is now.)
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-09-15 00:00:10 PDT
I confirmed by backout that this was caused by the fix for bug 269318 (which
still backs out cleanly with cvs up -j... -j...).  This is one of those bugs
that isn't necessarily obvious but makes the experience of using our software a
good bit worse, so nominating for blocking1.8b5.
Comment 5 Uri Bernstein (Google) 2005-09-15 00:24:05 PDT
Confirming on OS X => All/All
Comment 6 Aaron Leventhal 2005-09-20 19:46:52 PDT
Created attachment 196877 [details]
Simplified test case. Click in listbox, click to right of combo box, press down arrow
Comment 7 Aaron Leventhal 2005-09-20 21:38:59 PDT
Created attachment 196882 [details] [diff] [review]
Remove unnecessary call to GetDocSelectionLocation (unrelated) and unecessary if conidtion for setting selection before current focused node (the fix)
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-09-21 10:24:40 PDT
Does this also fix bug 308624?
Comment 9 Aaron Leventhal 2005-09-21 12:13:08 PDT
(In reply to comment #8)
> Does this also fix bug 308624?

Yes.
Comment 10 Mats Palmgren (:mats) 2005-09-21 17:35:54 PDT
Comment on attachment 196882 [details] [diff] [review]
Remove unnecessary call to GetDocSelectionLocation (unrelated) and unecessary if conidtion for setting selection before current focused node (the fix)

I think the current code in MoveCaretToFocus() is actually correct.

'SelectNodeContents(node)' results in
    DoSetRange(node, 0, node, node.childCount);

'SetStartBefore(node)' results in
    DoSetRange(node.Parent, node.Parent.IndexOf(node), ...)

So the current code puts the caret "outside" if it's a leaf
and "inside" if not. What you propose would put it outside
always, which I think is wrong - see upcoming testcase.

The problem in the original testcase is that we fail to find any
content near the click.
The caret for that testcase should have been placed after the
second <select> and then the scroll problem would not occur.
Comment 11 Mats Palmgren (:mats) 2005-09-21 17:38:18 PDT
Created attachment 196975 [details]
Testcase #2

1. Turn on caret browsing
2. type TAB once

Current result: caret is placed before the 'l' in 'line2'
Result with patch: caret is placed after the '1' in 'line1'
Comment 12 Aaron Leventhal 2005-09-22 06:33:40 PDT
(In reply to comment #10)
> (From update of attachment 196882 [details] [diff] [review] [edit])
> The problem in the original testcase is that we fail to find any
> content near the click.
> The caret for that testcase should have been placed after the
> second <select> and then the scroll problem would not occur.
Yes, that could be considered a problem with fixing but I don't believe that's
the regression.

The regression is that we're selecting a form control's entire node when it is
not a leaf. This happens when you click inside a textarea or select. At that
point the document's caret should be right before the form control.

Comment 13 Aaron Leventhal 2005-09-22 06:51:40 PDT
Created attachment 197037 [details] [diff] [review]
Fix the regression part of the problem. Also fixes bug 308624.

Mats, I would like the "not finding content for the click" part of the problem
to be dealt with in a separate bug since it has been around for longer and
isn't the regression I caused from bug 269318.
Comment 14 Aaron Leventhal 2005-09-22 12:47:29 PDT
*** Bug 308624 has been marked as a duplicate of this bug. ***
Comment 15 Aaron Leventhal 2005-09-22 13:53:47 PDT
This bug requires that you first click on a form control that is not a leaf,
such as a <textarea> or <select>

It was a regression from 269318, because because after a mouse click we now
MoveCaretToFocus() which is selecting the entire node unless it is a leaf. For a
leafd it it sets the caret right before the node.

We really want that leaf behavior for all form controls otherwise the entire
<textarea> or <select> gets selected when you click in it. Then the next time
you click in a blank area that doesn't correspond to a content node you're still
inside that prev widget.
Comment 16 Brian Ryner (not reading) 2005-09-22 14:33:35 PDT
Comment on attachment 197037 [details] [diff] [review]
Fix the regression part of the problem. Also fixes bug 308624.

I'm not crazy about testing for HTML_FORM_CONTROL, but Aaron and I couldn't
come up with anything better that covers all of the cases.
Comment 17 Tracy Walker [:tracy] 2005-09-23 12:01:51 PDT
Verified with Windows and Linux Firefox trunk builds 2005-09-23-07-trunk

Note You need to log in before you can comment on or make changes to this bug.