312093 - Caret-moved events are not always reported

Status: RESOLVED FIXED

(Firefox :: Disability Access, defect)

Description

[Willie Walker]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en) AppleWebKit/412.7 (KHTML, like Gecko) Safari/412.5
Build Identifier: Firefox 1.5 Beta 2

When caret browsing mode is enabled via F7, caret-moved events are not always reported when the caret 
is moved.

Reproducible: Always

Steps to Reproduce:
1. Go to www.google.com
2. Enable caret browsing mode (F7)
3. Run the at-spi event-listener-test application
4. Navigate around the web page using the arrow keys

Actual Results:  
Caret-moved events are not always reports.

Expected Results:  
Caret-moved events should be reported each time the caret changes position on the display.

Comment 1

[Ginn Chen]

Willie, do you have stable reproduce steps for this bug?
e.g. Put caret before which char, and press 'right' for x times, press 'right' again, it doesn't report.

Thanks.

Comment 2

[Aaron Leventhal]

I think we're broken in XUL textboxes, but this could be a separate bug.

Comment 3

[Aaron Leventhal]

Attachment: Fix for XUL

Comment 4

[Aaron Leventhal]

Comment on attachment 199941 [details] [diff] [review]
Fix for XUL

I'm moving this patch over to a separate bug for caret events in XUL textboxes,
where it belongs (bug 312941).

Comment 5

[Willie Walker]

I'm not sure if this is actually what Aaron has described.  Here's some
reproducable steps:

1) Run the at-spi event-listener-test 
2) Run firefox
3) Go to
http://www.google.com/firefox?client=firefox-a&rls=org.mozilla:en-US:official
4) Enable caret browsing mode
5) Click on the "E" in "Enjoy using Firefox?" or whatever text happens
   to be to the right of the lighbulb icon below the search area
6) Press the left arrow key several times

As you do #4, the caret will move across several blank areas, but
no events will be issued.

Comment 6

[Ginn Chen]

Some dicussion between Will and me.

Ginn>
To my understand, there are 3 or more phenomena.
1) "Tab" key moves the caret without emitting caret moved event. (See bugzilla 304805)
2) Sometimes, arrow key moves the caret without emitting caret moved event.
3) Sometimes, no symmetry to keyboard actions.

Will>
I think we could go for several days and find a large number of
phenomena.  The main point is that the caret navigation mode of
Firefox is currently fraught with enough problems so as to make
it not very useful for blind access.

I had a meeting with IBM regarding caret navigation, and they
said the same thing regarding the difficulty:  it's very hard to
fix since it is in a very sensitive part of the code base.

We talked a fair amount about alternatives, such as a Javascript
plugin and/or having the screen reader do the work.  I think we
reached the conclusion that it's just not worth the effort to
try to continue work on the core caret navigation for the
purposes of blind access.  But we also reached the conclusion that
it probably should not be completely abandoned since it's still OK
for sighted people with physical impairments.

So...I guess I'm saying that, from the Orca perspective, I'm fine
with postponing any work on this particular bug right now.

Comment 7

[Willie Walker]

Attachment: Standalone test case to demonstrate the problem.

Comment 8

[Willie Walker]

I think it is time to look at this problem again.  The lack of any caret or focus events whenever the caret moves is a serious detriment to any hope of making Firefox 2 a viable option for accessible browing.

Comment 9

[Ginn Chen]

ref:
accessible/src/base/nsCaretAccessible.cpp
line 190-235

perhaps we should fix some return NS_OK; here.

Comment 10

[Ginn Chen]

Attachment: patch

Comment 11

[Ginn Chen]

I'm confused with this part of code.
Sometimes mCurrentDOMNode is an AnchorElement if I move caret from left to right; if caret comes from right, mCurrentDOMNode is not.

As far as I tested, with this patch is better than before.

Another change is, when selection changes, it fires text-selection-change event together with caret-moved.

Willie Walker and Peter Korn confirmed it should fire caret-moved event for link.
(actually it already does without the patch, the code doesn't work as the comments wished)

Also, we still have no caret-moved event or wrong offset data with some cases. (they're not regressions)
1) Caret position doesn't belong to TextFrame
2) Caret is over TextArea, but not inside. (a very old problem of caret browsing)
3) When caret is shifting out of a link by pressing ctrl+right, offset may counts wrong.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

I can't usefully sr this; I have no idea what this code is doing nor why, nor time to sort it out.  Please ask someone else for sr.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know this code either.

Why is it so hard to get all caret events? We should be able to hook you into the nsCaret for each presshell so that you get notified whenever the caret moves (basically, from nsCaret::SetCaretDOMSelection and nsCaret::NotifySelectionChanged).

Comment 14

[Ginn Chen]

The problem is not to be notified, but it need to get nsIAccessibleText and caretOffset. 
I'm not sure if we can get them from nsCaret.

Comment 15

[Aaron Leventhal]

(In reply to comment #14)
> The problem is not to be notified, but it need to get nsIAccessibleText and
> caretOffset. 
> I'm not sure if we can get them from nsCaret.

Ginn, I didn't understand this. Could you say it again in a different way?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

From nsCaret, you can get the caret's associated content element, offset within the element, and also the hint --- everything that we use to determine the caret position. I'm not sure what else you could need.

Comment 17

[Ginn Chen]

It needs more tests. I didn't test it with seamonkey/composer yet.
And it causes a warning in GetCaretOffset if selection is not collapsed.

We're already using NotifySelectionChanged, so every caret movement will get into this section.
With MSAA, it only need to fire EVENT_LOCATION_CHANGE with "this".
But for ATK, we need to find out which nsAccessibleHyperText the caret is in.
nsAccessibleHyperText contains many text node and links, so we can't get it from nsCaret simply, the offset is also different.
Actually, nsCarent->GetCaretContent() equals to domSel->GetFocusNode

Comment 18

[Ginn Chen]

> 3) When caret is shifting out of a link by pressing ctrl+right, offset may
> counts wrong.
This happens because gLastFocusedNode is still the link.
We can add another judgement in nsAccessibleHyperText::GetCaretOffset.

Another issue is BR frame, "\n" is not in mTextChildren of nsAccessibleHyperText.
So GetCaretOffset will fail.

I'll file another bug for these issues.

Comment 19

[Ginn Chen]

bug 337400 logged

Comment 20

[Tim Miao]

*** Bug 340782 has been marked as a duplicate of this bug. ***

Comment 21

[Aaron Leventhal]

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

GetParentBlockNode() is now goig away via bug 340289 but you should be able to get the nsIAccessibleText via something like nsIDocument::GetAccessibleInParentChain(focusNode).  Most of the time you should be able to just get an accessible for the focusNode and get the parent for that, but it won't work if there is no accessible to create for the focusNode.

Comment 22

[Aaron Leventhal]

Fixing for Firefox 3, not Firefox 2

Comment 23

[Aaron Leventhal]

Attachment: Work in progress: reimplement caret and selection handling for accessible text

Comment 24

[Aaron Leventhal]

Attachment: Initial support for selection and caret.

One problem is that the caret events report a different position when moving right than left. For example, it appears that you move into the beginning of an inline child when you right arrow onto it. However, you cannot left arrow to the same beginning of that object. Instead you end up at the end of the previous one.

I'd like to follow that problem up in a later bug.

Comment 25

[Aaron Leventhal]

Attachment: Implements for <input>, <textarea> and <xul:textbox> as well

Comment 26

[Aaron Leventhal]

Attachment: Implement nsIAccessibleEditableText as well

The only big thing nsHyperTextAccessible still needs is GetTextHelper() to work again (without moving selection). We'll also need to do more with contenteditable when it gets checked in.

There is one odd case that doesn't always work. After InsertText() in a blank textfield, things are in a strange state, for example, the next InsertText() doesn't seem to work. I will probably file a separate bug for that.

Comment 27

[Aaron Leventhal]

Attachment: Also implement beginnings of GetText[Before|At|After]Offset (without affecting selection)

Sorry Ginn, this is turning into another big patch which does a lot, but it's hard to separate it into individual pieces.

Comment 28

[Aaron Leventhal]

Attachment: More fixes for GetText[Before|At|After]Offset

Comment 29

[Aaron Leventhal]

Attachment: Also add support for newlines \n

- This patch requires patch from bug 342596 in order to build (and to make GetText[Before|At|After]Offset work)
- This patch requires patch from bug 340667 in order for <br> newline code to be useful

Comment 30

[Aaron Leventhal]

Attachment: Better support for <br>, but still requires patch from bug 342596 to compile

Comment 31

[Aaron Leventhal]

Attachment: Support new patch for bug 342596 (PeekOffset changes) and make sure we do something reasonable when GetText[At|Before|After] offset is on a whitespace character

Comment 32

[Ginn Chen]

Looks good to me.
I'll doublecheck it when I'm back.

some minor issues,

+NS_IMETHODIMP nsHTMLBRAccessible::GetName(nsAString& aName)
should we call aName.Truncate()?

- * Adds a selection bounded by the specified offsets.
+ i* Adds a selection bounded by the specified offsets.
...

Comment 33

[Aaron Leventhal]

> nsHTMLBRAccessible::GetName
I changed it so we set aName to the newline char, instead of appending the newline char. That way we don't need Truncate().

Comment 34

[Firefox Bug Husbandry Bot]

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