HyperTextAccessible::DOMPointToHypertextOffset fails for node and offset equal to node child count

RESOLVED FIXED in mozilla20

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla20
access
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 695932 [details] [diff] [review]
testcase

example:

<p contentEditable="true" id="p"><span id="span">text</span>ohoho</p>

setting selection to (span, 1) range set selection after 'text' and before 'ohoho'. caretOffset returns html:p length.
(Assignee)

Comment 1

5 years ago
Created attachment 695968 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #695968 - Flags: review?(trev.saunders)
(Assignee)

Comment 2

5 years ago
Created attachment 696039 [details] [diff] [review]
patch2
Attachment #695968 - Attachment is obsolete: true
Attachment #695968 - Flags: review?(trev.saunders)
Attachment #696039 - Flags: review?(trev.saunders)
Comment on attachment 696039 [details] [diff] [review]
patch2

>+    if (!findNode) {
>+      if (aNodeOffset == 0) {

!aNodeOffset ?

>+        if (aNode == GetNode()) {

hm, wouldn't it be better to use GetContent() since you want to operate on body element not document node?

>+          // Case #1: this accessible has no children and thus has empty text,
>+          // we can only be at hypertext offset 0.

do we have tests for this case?

>+      // Set caret after a child of span element, i.e. after 'text' text.
>+      gQueue.push(new moveCaretToDOMPoint("test1", getNode("span"), 1, 4, "test1"));
>+      gQueue.push(new moveCaretToDOMPoint("test2", getNode("span2"), 1, 4, "test2"));

add bug link?
Attachment #696039 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 4

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> Comment on attachment 696039 [details] [diff] [review]
> patch2
> 
> >+    if (!findNode) {
> >+      if (aNodeOffset == 0) {
> 
> !aNodeOffset ?

I prefer to stay explicit with integers.

> >+        if (aNode == GetNode()) {
> 
> hm, wouldn't it be better to use GetContent() since you want to operate on
> body element not document node?

I think I don't know :) In theory nothing prevents stuff be outside of body. So I'd left this unchanged.

> >+          // Case #1: this accessible has no children and thus has empty text,
> >+          // we can only be at hypertext offset 0.
> 
> do we have tests for this case?

seems no, I can add the one

> 
> >+      // Set caret after a child of span element, i.e. after 'text' text.
> >+      gQueue.push(new moveCaretToDOMPoint("test1", getNode("span"), 1, 4, "test1"));
> >+      gQueue.push(new moveCaretToDOMPoint("test2", getNode("span2"), 1, 4, "test2"));
> 
> add bug link?

sure, thanks.
(Assignee)

Comment 5

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/6ac39337919a
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/6ac39337919a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.