Caret-moved events are not always reported

RESOLVED FIXED

Status

()

Firefox
Disability Access
--
major
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Willie Walker, Assigned: Aaron Leventhal)

Tracking

({access, sec508})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 10 obsolete attachments)

13.60 KB, text/x-python
Details
93.12 KB, patch
Ginn Chen
: review+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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

12 years ago
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.
(Assignee)

Comment 2

12 years ago
I think we're broken in XUL textboxes, but this could be a separate bug.
(Assignee)

Updated

12 years ago
Keywords: access, sec508
(Assignee)

Comment 3

12 years ago
Created attachment 199941 [details] [diff] [review]
Fix for XUL
Attachment #199941 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #199941 - Flags: review?(ginn.chen)
(Assignee)

Updated

12 years ago
Attachment #199941 - Flags: review?(ginn.chen) → review?(parente)
(Assignee)

Comment 4

12 years ago
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).
Attachment #199941 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #199941 - Flags: review?(parente)
(Assignee)

Updated

12 years ago
Attachment #199941 - Attachment is obsolete: true
(Reporter)

Comment 5

12 years ago
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

12 years ago
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.

Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 7

12 years ago
Created attachment 219144 [details]
Standalone test case to demonstrate the problem.
(Reporter)

Comment 8

12 years ago
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.
(Reporter)

Updated

12 years ago
Blocks: 333488

Comment 9

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

perhaps we should fix some return NS_OK; here.

Comment 10

12 years ago
Created attachment 220238 [details] [diff] [review]
patch
Assignee: nobody → ginn.chen
Status: NEW → ASSIGNED
Attachment #220238 - Flags: review?(aaronleventhal)

Comment 11

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #220238 - Flags: superreview?(bzbarsky)
Attachment #220238 - Flags: review?(aaronleventhal)
Attachment #220238 - Flags: review+
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.
Attachment #220238 - Flags: superreview?(bzbarsky)
(Assignee)

Updated

12 years ago
Attachment #220238 - Flags: superreview?(jst)
(Assignee)

Updated

12 years ago
Attachment #220238 - Flags: superreview?(jst) → superreview?(roc)
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

12 years ago
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.
(Assignee)

Comment 15

12 years ago
(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?

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.

Updated

12 years ago
Attachment #220238 - Flags: superreview?(roc)

Comment 17

12 years ago
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

12 years ago
> 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.

Updated

12 years ago
Depends on: 337400

Comment 19

12 years ago
bug 337400 logged
No longer depends on: 337400

Updated

12 years ago
Depends on: 337400

Comment 20

12 years ago
*** Bug 340782 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

12 years ago
Depends on: 340829
(Assignee)

Comment 21

12 years ago
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.
Attachment #220238 - Flags: review+ → review-
(Assignee)

Comment 22

12 years ago
Fixing for Firefox 3, not Firefox 2
Blocks: 333492
No longer blocks: 333488
(Assignee)

Updated

12 years ago
Assignee: ginn.chen → aaronleventhal
Status: ASSIGNED → NEW
(Assignee)

Comment 23

12 years ago
Created attachment 226579 [details] [diff] [review]
Work in progress: reimplement caret and selection handling for accessible text
Attachment #220238 - Attachment is obsolete: true
(Assignee)

Comment 24

12 years ago
Created attachment 226669 [details] [diff] [review]
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.
Attachment #226579 - Attachment is obsolete: true
(Assignee)

Comment 25

12 years ago
Created attachment 226823 [details] [diff] [review]
Implements for <input>, <textarea> and <xul:textbox> as well
Attachment #226669 - Attachment is obsolete: true
Attachment #226823 - Flags: review?(ginn.chen)
(Assignee)

Comment 26

12 years ago
Created attachment 226898 [details] [diff] [review]
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.
Attachment #226823 - Attachment is obsolete: true
Attachment #226898 - Flags: review?(ginn.chen)
Attachment #226823 - Flags: review?(ginn.chen)
(Assignee)

Updated

12 years ago
Blocks: 342308
(Assignee)

Updated

12 years ago
Blocks: 340830
(Assignee)

Updated

12 years ago
No longer blocks: 340830
Depends on: 340830
(Assignee)

Updated

12 years ago
No longer blocks: 342308
Depends on: 342308
(Assignee)

Comment 27

12 years ago
Created attachment 227033 [details] [diff] [review]
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.
Attachment #226898 - Attachment is obsolete: true
Attachment #227033 - Flags: review?(ginn.chen)
Attachment #226898 - Flags: review?(ginn.chen)
(Assignee)

Updated

12 years ago
Depends on: 342596
(Assignee)

Comment 28

12 years ago
Created attachment 227133 [details] [diff] [review]
More fixes for GetText[Before|At|After]Offset
Attachment #227133 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #227033 - Attachment is obsolete: true
Attachment #227033 - Flags: review?(ginn.chen)
(Assignee)

Comment 29

12 years ago
Created attachment 227147 [details] [diff] [review]
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
Attachment #227147 - Flags: review?(ginn.chen)
(Assignee)

Updated

12 years ago
Attachment #227133 - Attachment is obsolete: true
Attachment #227133 - Flags: review?
(Assignee)

Comment 30

12 years ago
Created attachment 227176 [details] [diff] [review]
Better support for <br>, but still requires patch from bug 342596 to compile
Attachment #227147 - Attachment is obsolete: true
Attachment #227176 - Flags: review?(ginn.chen)
Attachment #227147 - Flags: review?(ginn.chen)
(Assignee)

Updated

12 years ago
Blocks: 320357
(Assignee)

Comment 31

12 years ago
Created attachment 228309 [details] [diff] [review]
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
Attachment #227176 - Attachment is obsolete: true
Attachment #228309 - Flags: review?(ginn.chen)
Attachment #227176 - Flags: review?(ginn.chen)

Comment 32

12 years ago
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.
...
(Assignee)

Comment 33

12 years ago
> 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().

Updated

12 years ago
Attachment #228309 - Flags: review?(ginn.chen) → review+
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.