Closed Bug 269318 Opened 16 years ago Closed 16 years ago

TAB doesn't start from selection text after mouse click

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ginnchen+exoracle, Assigned: aaronlev)

References

(Blocks 1 open bug)

Details

(Keywords: access, Whiteboard: sunport17)

Attachments

(1 file, 1 obsolete file)

go to bugzilla.mozilla.org
1) click the inputbox
2) select some text by mouse
3) press TAB, the image of mozilla.org will be focused (BUG)
4) select some text by mouse
5) press TAB, the focusable next the selection text will be focused (CORRECT)
root cause:
when press TAB/shift+TAB, mLastFocusedWith should be set to eEventFocusedByKey,
before calling ShiftFocus()
Status: NEW → ASSIGNED
OS: Windows XP → All
Attached patch patch (obsolete) — Splinter Review
Attachment #165670 - Flags: review?(aaronleventhal)
Blocks: focusnav
Comment on attachment 165670 [details] [diff] [review]
patch

We wouldn't even need mLastFocusedWith if we always do it right before we call
ShiftFocus.

We need to remember the reason it's like this. We did this on purpose.

I think it was something about being able to select text with the mouse and not
affect the focus position. But that doesn't seem to work. Anyone remember what
it was about?
Attachment #165670 - Flags: review?(aaronleventhal) → review-
The behavior reported as incorrect (tab not starting from the selection) only
happens after a specific sequence of focus events, so it doesn't seem
intentional to me.  I hit this bug about once a week but I hadn't filed it
because I couldn't figure out a sequence of steps to reproduce.
Looking at this makes me wonder when this broke:
If you select some text and then press tab or shift+tab, I don't think it should
deselect your text.
I disagree; I think tabbing from a text selection should move the caret (along
with focus) and therefore deselect the text, like it does now.  I think focus
and text selection should match whenever possible (cf mpt's comments in bug 36848).
Did we ever get this correct?
Comment on attachment 165670 [details] [diff] [review]
patch

I found the reason we have mLastFocusedWith at all. It's for the fix in bug
131139. I don't think we need it anymore at all. I have a better fix to get the
desired functionality. Always setting eEventFocusedByKey = eEventFocusedByKey
right before we use it in ShiftFocus() would make it useless, so this patch is
certainly not correct.
Remove mLastFocusedWith, which is no longer needed because of this change. All
it did was help with situations where a link is focused while there is a
selection.
Assignee: ginn.chen → aaronleventhal
Attachment #165670 - Attachment is obsolete: true
Attachment #166255 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #166255 - Flags: review?(ginn.chen)
Comment on attachment 166255 [details] [diff] [review]
If element gets focused, move selection to focus if  focus isn't already inside the selection. mLastFocusedWith is no longer needed for making tab not move relative to selection after a mouse click

looks good to me r=ginn
Attachment #166255 - Flags: review?(ginn.chen) → review+
Whiteboard: sunport17
Comment on attachment 166255 [details] [diff] [review]
If element gets focused, move selection to focus if  focus isn't already inside the selection. mLastFocusedWith is no longer needed for making tab not move relative to selection after a mouse click

>+      // Move caret to focus only if focus not already contained in selection
Can you explain what the point of this bit is? I'm not sure what effect I
should be looking for.
(In reply to comment #11)
> (From update of attachment 166255 [details] [diff] [review])
> >+      // Move caret to focus only if focus not already contained in selection
> Can you explain what the point of this bit is? I'm not sure what effect I
> should be looking for.
> 

I'm fixing bug 131139 the way it was originally suggested. Basically, when you
right click in a selection, you should get the option to copy the selection,
even if it was a link that you clicked on. This means the selection needs to be
retained when you right click in it. The bug also talked about click dragging a
link within the selection. I went with the behavior they did not want to break
in both cases -- don't ruin the selection just because the user is doing
something within it.

If you right-click or click-drag outside of of the selection, it's intuitive
that you're doing something new. Whatever you click on should get focused but
the selection should go away.

Fixing bug 131139 that way, as the bug had suggested, removes the need for
storing mLastFocusedWith. Now we can always tab from the selection.
Comment on attachment 166255 [details] [diff] [review]
If element gets focused, move selection to focus if  focus isn't already inside the selection. mLastFocusedWith is no longer needed for making tab not move relative to selection after a mouse click

Ah, I get you now; context clicking outside the selection should kill the
selection.
Attachment #166255 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Checking in nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <-- 
nsEventStateManager.cpp
new revision: 1.545; previous revision: 1.544
done
Checking in nsEventStateManager.h;
/cvsroot/mozilla/content/events/src/nsEventStateManager.h,v  <-- 
nsEventStateManager.h
new revision: 1.125; previous revision: 1.124
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
> Ah, I get you now; context clicking outside the selection should kill the
> selection.

So did this fix bug 135813 then?
(In reply to comment #15)
>>Ah, I get you now; context clicking outside the selection should kill the
>>selection.
>So did this fix bug 135813 then?
Partially... for instance, if you right-click a link.
This checkin caused regressions bug 305985 and 308624.  These have been bugging
me for months -- but as a general feeling that the focus just wasn't where I
expected it to be.  It was hard to translate that to clear steps to reproduce.
Depends on: 305985, 308624
Commenting out the MoveCaretToFocus call that this patch added is sufficient to
fix those regressions, fwiw.
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.