Closed Bug 269318 Opened 16 years ago Closed 16 years ago
TAB doesn't start from selection text after mouse click
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
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
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+
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.
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.