265203 - When caret is at end of one link, focus displays to next link

Status: VERIFIED FIXED

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

Description

[Ginn Chen]

Trun on Caret Browsing
Try the test case attached,

Press right arrow,
When caret displays at end of "Mozilla", "Firefox" is focused.

Comment 1

[Ginn Chen]

Attachment: Test Case

Comment 2

[Ginn Chen]

Attachment: patch

Aaron, can I remove these code?

Comment 3

[Aaron Leventhal]

Comment on attachment 162680 [details] [diff] [review]
patch

We can't get rid of it, we need to make it smarter. Otherwise, when the caret
appears right in front of some links, they won't get focused, because the
selection is not in the <a>, but at the end of the previous node.

Comment 4

[Ginn Chen]

(In reply to comment #3)
> (From update of attachment 162680 [details] [diff] [review])
> We can't get rid of it, we need to make it smarter. Otherwise, when the caret
> appears right in front of some links, they won't get focused, because the
> selection is not in the <a>, but at the end of the previous node.
> 

Agree,
But can you give me a test case to show that?
I find if so, actually caret appears 1px out of the focus box.(without my patch)
Do we really need focus the link? Or let user press right arrow again?

Comment 5

[Ginn Chen]

Attachment: smarter patch

Comment 6

[Ginn Chen]

Comment on attachment 162808 [details] [diff] [review]
smarter patch

This one is smarter. Also I dislike separate visual selection and logical
selection.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Just checking for |parentContent->Tag() == nsHTMLAtoms::a| is wrong in general
for XML.  It's even wrong for HTML (named anchors).

Perhaps you want to use nsContentUtils::IsDraggableLink (which perhaps needs
renaming to something more like "IsLink()")?  If so, and if this code is called
a lot, we can optimize IsDraggableLink() a bit, I suspect.

Comment 8

[Aaron Leventhal]

Attachment: Testcase: when you arrow to the start of a link, the link should get focus

Comment 9

[Aaron Leventhal]

Comment on attachment 162808 [details] [diff] [review]
smarter patch

How about just using nsIContent::IsFocusable()? Will that work?

Comment 10

[Ginn Chen]

(In reply to comment #9)
> (From update of attachment 162808 [details] [diff] [review])
> How about just using nsIContent::IsFocusable()? Will that work?
> 

No, it doesn't work for me.

Comment 11

[Ginn Chen]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 162808 [details] [diff] [review])
> > How about just using nsIContent::IsFocusable()? Will that work?
> > 
> 
> No, it doesn't work for me.

Sorry, it doesn't work for startContent, but work for parentContent.
I'll post new patch soon.

Comment 12

[Ginn Chen]

Attachment: smaller patch

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 163386 [details] [diff] [review]
smaller patch

How often is this code called?	IsFocusable() is not all that cheap; if we plan
to call it a lot, we should work on making it cheaper...

Past that, what exactly is this patch doing?  It would be good to document it
in the code.

Comment 14

[Ginn Chen]

(In reply to comment #13)
> (From update of attachment 163386 [details] [diff] [review])
> How often is this code called?	IsFocusable() is not all that cheap; if we plan
> to call it a lot, we should work on making it cheaper...
> 
Every caret moves (e.g. press arrow key) when Caret Browsing is on

> Past that, what exactly is this patch doing?  It would be good to document it
> in the code.
> 
OK

Comment 15

[Ginn Chen]

(In reply to comment #8)
> Created an attachment (id=163318)
> Testcase: when you arrow to the start of a link, the link should get focus
> 
Aaron, my concern is why we need focus a link if caret is at end of prev frame?
Actually we just focus the link, but not move caret to position 0 of the
textframe of the link.
So we are separating caret and focus to 2 differant frames, and caret displays
1px before focus dot border.

On the other side, we are not focusing a link if caret is at beginning of next
frame.

Comment 16

[Aaron Leventhal]

> Aaron, my concern is why we need focus a link if caret is at end of prev frame?
But the user model is not the same as our data model. To the user, they are
looking at the characters on the screen, not the internal frame tree. They have
just arrowed up to a link. The next right arrow goes inside the link. Yet,
conceptually there is a position where they are on the link but not inside of it.

> On the other side, we are not focusing a link if caret is at beginning of next
> frame.
We could, but I felt that when the caret should apply to what it's in front of.

Comment 17

[Aaron Leventhal]

Comment on attachment 163386 [details] [diff] [review]
smaller patch

Interesting solution but I think the problem is really different.

I think the problem is that the caret is not near the start of the new link.
The start of the new link and the caret's x position are far apart.

If you take the first testcase in this bug, and turn "Mozilla" into ordinary
text, the problem will still occur when you right arrow to the end of Mozilla,
even with this patch.

So, I would make the solution dependent on whether the caret is really
immediately before the thing to receive focus.

Comment 18

[Ginn Chen]

The problem is logical next frame may be visually far away from the prev frame.

Is it worth to do such a favour for user?
I suggest to remove the code. Keep it simple. like first version patch.

Comment 19

[Ginn Chen]

Comment on attachment 162680 [details] [diff] [review]
patch

Aaron, can you review it again?
I don't think we can find a simple method to determine whether the caret is
really
immediately before the thing to receive focus.
And it's unnecessary.

Comment 20

[Aaron Leventhal]

Comment on attachment 162680 [details] [diff] [review]
patch

Ginn, if I have something like:

Hello Google
      ------
I want to have the link Google receive focus if the caret is moved right before
the G.

So, I think a better fix is to test the caret position and see if it right at
the beginning of the text frame with "Google".

Comment 21

[Aaron Leventhal]

This is very roughly what's needed to fix the test (this only happens when the
caret is at the end of a frame, and the next thing is a link):

nsPoint framePt;
nsRect caretRect;
nsIView *frameClosestView = frame->GetClosestView(&framePt);
nsIView *caretView;
caret->GetCaretCoordinates(eClosestViewCoordinates, domSelection, &caretRect,
isCollapsed, &caretView);
if (caretView == frameClosestView && caretRect.y == framePt.y && 
    (caretRect.x == framePt.x || caretRect.x == framePt.x -1)) {
    // Focus the link
    // XXX will caretRect.x be exactly equal to framePt.x or how much will it be
off by?
    }

Comment 22

[Ginn Chen]

Attachment: patch to compare caret position

This patch works for me on Solaris GTK2+XFT build.

Comment 23

[Aaron Leventhal]

Attachment: Switch logic so that we don't have to undo changes when caret is in wrong place. Instead, only use newCaret vars if caret at start of next item

Comment 24

[Aaron Leventhal]

Comment on attachment 176711 [details] [diff] [review]
patch to compare caret position 

Most recent patch reverses the logic.

Comment 25

[Brian Ryner (not reading)]

Comment on attachment 176732 [details] [diff] [review]
Switch logic so that we don't have to undo changes when caret is in wrong place. Instead, only use newCaret vars if caret at start of next item

>+            newCaretContent = newCaretFrame->GetContent();            
>+          }
>+          while (!newCaretContent || newCaretContent == startContent);

Can you move the "while" up next to the closing bracket (separated by a space)?
 This makes it easier to see that the while is part of that block.

sr=me with that change.

Comment 26

[Aaron Leventhal]

Ginn, let me know if this does the job.

Checking in nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <-- 
nsEventStateManager.cpp
new revision: 1.562; previous revision: 1.561
done

Comment 27

[Ginn Chen]

It works. Thanks.

Comment 28

[Firefox Bug Husbandry Bot]

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