Last Comment Bug 824901 - HyperTextAccessible::DOMPointToHypertextOffset fails for node and offset equal to node child count
: HyperTextAccessible::DOMPointToHypertextOffset fails for node and offset equa...
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla20
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: caretsela11y
  Show dependency treegraph
 
Reported: 2012-12-26 23:30 PST by alexander :surkov
Modified: 2013-01-05 16:25 PST (History)
1 user (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (387 bytes, patch)
2012-12-26 23:30 PST, alexander :surkov
no flags Details | Diff | Splinter Review
patch (5.20 KB, patch)
2012-12-27 02:23 PST, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (5.53 KB, patch)
2012-12-27 07:20 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-12-26 23:30:22 PST
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.
Comment 1 alexander :surkov 2012-12-27 02:23:16 PST
Created attachment 695968 [details] [diff] [review]
patch
Comment 2 alexander :surkov 2012-12-27 07:20:04 PST
Created attachment 696039 [details] [diff] [review]
patch2
Comment 3 Trevor Saunders (:tbsaunde) 2012-12-27 08:03:27 PST
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?
Comment 4 alexander :surkov 2012-12-27 08:07:45 PST
(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.
Comment 6 Phil Ringnalda (:philor) 2013-01-05 16:25:50 PST
https://hg.mozilla.org/mozilla-central/rev/6ac39337919a

Note You need to log in before you can comment on or make changes to this bug.