Closed Bug 265203 Opened 16 years ago Closed 16 years ago

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

Categories

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

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: ginnchen+exoracle, Assigned: aaronlev)

References

Details

(Keywords: access, Whiteboard: sunport17)

Attachments

(3 files, 4 obsolete files)

Trun on Caret Browsing
Try the test case attached,

Press right arrow,
When caret displays at end of "Mozilla", "Firefox" is focused.
Attached file Test Case
Attached patch patch (obsolete) — Splinter Review
Aaron, can I remove these code?
Attachment #162680 - Flags: review?(aaronleventhal)
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.
Attachment #162680 - Flags: review?(aaronleventhal) → review-
(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?
Status: NEW → ASSIGNED
Attached patch smarter patch (obsolete) — Splinter Review
Comment on attachment 162808 [details] [diff] [review]
smarter patch

This one is smarter. Also I dislike separate visual selection and logical
selection.
Attachment #162808 - Flags: review?(aaronleventhal)
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 on attachment 162808 [details] [diff] [review]
smarter patch

How about just using nsIContent::IsFocusable()? Will that work?
Attachment #162808 - Flags: review?(aaronleventhal) → review-
(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.
(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.
Attached patch smaller patch (obsolete) — Splinter Review
Attachment #162808 - Attachment is obsolete: true
Attachment #163386 - Flags: superreview?(bzbarsky)
Attachment #163386 - Flags: review?(aaronleventhal)
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.
(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
(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.
> 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 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.
Attachment #163386 - Flags: superreview?(bzbarsky)
Attachment #163386 - Flags: review?(aaronleventhal)
Attachment #163386 - Flags: review-
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.
Whiteboard: sunport17
Blocks: caretnav
Keywords: access, sec508
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.
Attachment #162680 - Flags: review- → review?(aaronleventhal)
Attachment #163386 - Attachment is obsolete: true
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".
Attachment #162680 - Flags: review?(aaronleventhal) → review-
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?
    }
This patch works for me on Solaris GTK2+XFT build.
Attachment #162680 - Attachment is obsolete: true
Attachment #176711 - Flags: review?(aaronleventhal)
Attachment #176732 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #176732 - Flags: review?(ginn.chen)
Comment on attachment 176711 [details] [diff] [review]
patch to compare caret position 

Most recent patch reverses the logic.
Attachment #176711 - Flags: review?(aaronleventhal) → review-
Attachment #176732 - Flags: review?(ginn.chen) → review+
Attachment #176732 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview?(bryner)
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.
Attachment #176732 - Flags: superreview?(bryner) → superreview+
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
It works. Thanks.
Status: RESOLVED → VERIFIED
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.