460417 - invalid handling of selection changes inside input element

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

when addressbar is clicked then I got an assertion "no IAccessibleText for caret move event". The problem is we get focusContainer - html:div (child of html:input), focusOffset is 1, html:div has one child. Therefore we don't get focusNode correctly. We should use html:div as focus node in this case.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[[e-mail]]

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

>+  nsCOMPtr<nsIDOMNode> focusNode;
>+  aSelection->GetFocusNode(getter_AddRefs(focusNode));
>+  if (!focusNode)
>+    return nsnull;
>+
>+  // Get DOM node that focus DOM point points to.
>+  nsCOMPtr<nsIDOMNode> resultNode = focusNode;
>+
>+  nsCOMPtr<nsIContent> focusContent(do_QueryInterface(focusNode));
Nit: focusNode is never used after this point... maybe call it resultNode?

>+    NS_ASSERTION(focusOffset >= 0 && focusOffset <= static_cast<PRInt32>(childCount),
>+                 "Wrong focus offset in selection!");
>+
>+    // The offset can be after last child of container node that means DOM point
>+    // is placed immediately after the last child. In this case use focusNode
>+    // as result node.
>+    if (focusOffset != childCount) {
Don't you need to cast here too to avoid the signed/unsigned mismatch? (I didn't think to capture the compiler output to check.)

>+  
Nit: whitespace. (Don't feel too bad, you managed to remove some too.)

>+    if (!content || !content->IsNodeOfType(nsINode::eTEXT)) {
Don't you mean content && !

>+  NS_ASSERTION(PR_FALSE, "No nsIAccessibleText for selection change event!");
Please use NS_ERROR or NS_NOTREACHED or similar?

>+  rv= textAcc->GetCaretOffset(&caretOffset);
Nit: space before =

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> >+    if (!content || !content->IsNodeOfType(nsINode::eTEXT)) {
> Don't you mean content && !

I don't think so. It's copy&paste from existing code but !content is true when result node is document (which has text accessible)

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch2

neil's comments + mochitest

Comment 7

[alexander :surkov (:asurkov)]

checked in, http://hg.mozilla.org/mozilla-central/rev/833d54c1b774

Comment 8

[Marco Zehe (:MarcoZ)]

Comment on attachment 344600 [details] [diff] [review]
patch2

This bug, which is a duplicate of bug 459152, is a regression from bug 454377. Since we have approval 1.9.0.5 for bug 454377, which is needed by JAWS 10, requesting approval for this bug as well so we don't regress on Linux once 454377 lands.

Comment 9

[Samuel Sidler (old account; do not CC)]

Comment on attachment 344600 [details] [diff] [review]
patch2

Good catch. Approved for 1.9.0.5. a=ss

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 344600 [details] [diff] [review]
patch2

Too late for 1.9.0.5

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch 1.9.0

Comment 12

[Aaron Leventhal]

Comment on attachment 348933 [details] [diff] [review]
patch 1.9.0

It's basically the same but merged, right?

rs=aaronlev

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)
> (From update of attachment 348933 [details] [diff] [review])
> It's basically the same but merged, right?
> 
> rs=aaronlev

right but it's not automatic merging so I reask reviews.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 348933 [details] [diff] [review]
patch 1.9.0

I guess it's too late for 1.9.0.5 so nominate for 1.9.0.6

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 348933 [details] [diff] [review]
patch 1.9.0

Approved for 1.9.0.6, a=dveditz for release-drivers.

Comment 16

[alexander :surkov (:asurkov)]

Checking in accessible/src/base/nsAccessibilityUtils.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityUtils.cpp,v  <--  nsAccessibilityUtils.cpp
new revision: 1.35; previous revision: 1.34
done
Checking in accessible/src/base/nsAccessibilityUtils.h;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityUtils.h,v  <--  nsAccessibilityUtils.h
new revision: 1.28; previous revision: 1.27
done
Checking in accessible/src/base/nsCaretAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsCaretAccessible.cpp,v  <--  nsCaretAccessible.cpp
new revision: 1.66; previous revision: 1.65
done
Checking in accessible/tests/mochitest/Makefile.in;
/cvsroot/mozilla/accessible/tests/mochitest/Makefile.in,v  <--  Makefile.in
new revision: 1.26; previous revision: 1.25
done
RCS file: /cvsroot/mozilla/accessible/tests/mochitest/test_events_caretmove.html,v
done
Checking in accessible/tests/mochitest/test_events_caretmove.html;
/cvsroot/mozilla/accessible/tests/mochitest/test_events_caretmove.html,v  <--  test_events_caretmove.html
initial revision: 1.1
done

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Marco, can you verify this for 1.9.0.6?

Comment 18

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6