13.60 KB, text/x-python
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
93.12 KB, patch
Ginn Chen: review+
|Details | Diff | Splinter Review|
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.
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.
I think we're broken in XUL textboxes, but this could be a separate bug.
Created attachment 199941 [details] [diff] [review] Fix for XUL
Attachment #199941 - Flags: review?(ginn.chen) → review?(parente)
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).
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
ref: accessible/src/base/nsCaretAccessible.cpp line 190-235 perhaps we should fix some return NS_OK; here.
Created attachment 220238 [details] [diff] [review] patch
Assignee: nobody → ginn.chen
Status: NEW → ASSIGNED
Attachment #220238 - Flags: review?(aaronleventhal)
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 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?(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).
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.
(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.
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
> 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.
*** Bug 340782 has been marked as a duplicate of this bug. ***
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: ginn.chen → aaronleventhal
Status: ASSIGNED → NEW
Created attachment 226579 [details] [diff] [review] Work in progress: reimplement caret and selection handling for accessible text
Attachment #220238 - Attachment is obsolete: true
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
Created attachment 226823 [details] [diff] [review] Implements for <input>, <textarea> and <xul:textbox> as well
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.
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.
Created attachment 227133 [details] [diff] [review] More fixes for GetText[Before|At|After]Offset
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
Created attachment 227176 [details] [diff] [review] Better support for <br>, but still requires patch from bug 342596 to compile
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
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. ...
> 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().
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.