269318 - TAB doesn't start from selection text after mouse click

Status: RESOLVED FIXED

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

Description

[Ginn Chen]

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)

Comment 1

[Ginn Chen]

root cause:
when press TAB/shift+TAB, mLastFocusedWith should be set to eEventFocusedByKey,
before calling ShiftFocus()

Comment 2

[Ginn Chen]

Attachment: patch

Comment 3

[Aaron Leventhal]

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?

Comment 4

[Jesse Ruderman]

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.

Comment 5

[Aaron Leventhal]

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.

Comment 6

[Jesse Ruderman]

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).

Comment 7

[Aaron Leventhal]

Did we ever get this correct?

Comment 8

[Aaron Leventhal]

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.

Comment 9

[Aaron Leventhal]

Attachment: 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

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.

Comment 10

[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

Comment 11

[[e-mail]]

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.

Comment 12

[Aaron Leventhal]

(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 13

[[e-mail]]

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.

Comment 14

[Aaron Leventhal]

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

Comment 15

[:deanis74]

> Ah, I get you now; context clicking outside the selection should kill the
> selection.

So did this fix bug 135813 then?

Comment 16

[[e-mail]]

(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.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Commenting out the MoveCaretToFocus call that this patch added is sufficient to
fix those regressions, fwiw.

Comment 19

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381