The default bug view has changed. See this FAQ.

after clicking in blank content area, focus stays in some widgets when it should have left

VERIFIED FIXED

Status

()

Core
Event Handling
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: dbaron, Assigned: Aaron Leventhal)

Tracking

({fixed1.8})

Trunk
fixed1.8
Points:
---
Bug Flags:
blocking1.8b5 +
blocking1.9a1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
(Note that if you click on the "blocking1.9a1" *text*, it works as expected.)

Comment 2

12 years ago
Bug also occurs in Mozilla 2005-04-30 on Linux so it's not a recent regression.
(Reporter)

Comment 3

12 years ago
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.)
(Reporter)

Updated

12 years ago
Blocks: 269318
(Reporter)

Comment 4

12 years ago
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.
(Reporter)

Updated

12 years ago
Flags: blocking1.8b5?
Confirming on OS X => All/All
OS: Linux → All
Hardware: PC → All

Updated

12 years ago
Assignee: events → aaronleventhal
Flags: blocking1.9a1+

Updated

12 years ago
Flags: blocking1.8b5? → blocking1.8b5+
(Assignee)

Comment 6

12 years ago
Created attachment 196877 [details]
Simplified test case. Click in listbox, click to right of combo box, press down arrow
(Assignee)

Comment 7

12 years ago
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)
Attachment #196882 - Flags: superreview?(bryner)
Attachment #196882 - Flags: review?(mats.palmgren)
(Reporter)

Comment 8

12 years ago
Does this also fix bug 308624?
(Assignee)

Comment 9

12 years ago
(In reply to comment #8)
> Does this also fix bug 308624?

Yes.
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.
Attachment #196882 - Flags: review?(mats.palmgren) → review-
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'
(Assignee)

Comment 12

12 years ago
(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.

(Assignee)

Updated

12 years ago
Attachment #196882 - Flags: superreview?(bryner)
(Assignee)

Comment 13

12 years ago
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.
Attachment #197037 - Flags: review?(mats.palmgren)
(Assignee)

Updated

12 years ago
Summary: focus stays in listbox select widget when it should have left → after clicking in blank content area, focus stays in some widgets when it should have left
(Assignee)

Comment 14

12 years ago
*** Bug 308624 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

12 years ago
Attachment #197037 - Flags: superreview?(bryner)
(Assignee)

Updated

12 years ago
Whiteboard: [ETA: patch needs review]
(Assignee)

Comment 15

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

Updated

12 years ago
Attachment #197037 - Flags: review?(mats.palmgren) → review+
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.
Attachment #197037 - Flags: superreview?(bryner)
Attachment #197037 - Flags: superreview+
Attachment #197037 - Flags: review?(mats.palmgren)
Attachment #197037 - Flags: review+

Updated

12 years ago
Attachment #197037 - Flags: review?(mats.palmgren) → review+
(Assignee)

Updated

12 years ago
Attachment #197037 - Flags: approval1.8b5?
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [ETA: patch needs review] → [ETA: patch needs approval]

Updated

12 years ago
Attachment #197037 - Flags: approval1.8b5? → approval1.8b5+
(Assignee)

Updated

12 years ago
Keywords: fixed1.8
Verified with Windows and Linux Firefox trunk builds 2005-09-23-07-trunk
Status: RESOLVED → VERIFIED

Updated

8 years ago
Whiteboard: [ETA: patch needs approval]
You need to log in before you can comment on or make changes to this bug.