Closed Bug 824901 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(2 files, 1 obsolete file)

Attached patch testcaseSplinter Review
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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #695968 - Flags: review?(trev.saunders)
Attached patch patch2Splinter Review
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/6ac39337919a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.