Closed Bug 943612 Opened 11 years ago Closed 10 years ago

don't use GetPosAndText to convert offsets to DOM range

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access, Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
a second strike to get rid of GetPosAndText
Attachment #8338832 - Flags: review?(trev.saunders)
Comment on attachment 8338832 [details] [diff] [review]
patch

>+  if (child->IsTextLeaf()) {
>+    nsIContent* content = child->GetContent();
>+    int32_t idx = 0;
>+    RenderedToContentOffset(content->GetPrimaryFrame(), innerOffset, &idx);

that can fail right? shouldn't you check it?

>+  nsINode* node = child->GetNode();
>+  nsINode* parentNode = node->GetParentNode();
>+  return parentNode ?
>+    DOMPoint(parentNode, parentNode->IndexOf(node) + innerOffset) :
>+    DOMPoint();

hm, if its not a text leaf and has width > 1 then doesn't it have to be a hypertext and you could call OfsetToDOMPoint() on it?

>+  operator bool() const { return node; }

I'm not really sure making this implicit instead of an explicit .set() or something is great.
(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> Comment on attachment 8338832 [details] [diff] [review]
> patch
> 
> >+  if (child->IsTextLeaf()) {
> >+    nsIContent* content = child->GetContent();
> >+    int32_t idx = 0;
> >+    RenderedToContentOffset(content->GetPrimaryFrame(), innerOffset, &idx);
> 
> that can fail right? shouldn't you check it?

ok if you want

> >+  nsINode* node = child->GetNode();
> >+  nsINode* parentNode = node->GetParentNode();
> >+  return parentNode ?
> >+    DOMPoint(parentNode, parentNode->IndexOf(node) + innerOffset) :
> >+    DOMPoint();
> 
> hm, if its not a text leaf and has width > 1 then doesn't it have to be a
> hypertext and you could call OfsetToDOMPoint() on it?

not really, it's DOM point, it shouldn't be accessible hierarchy oriented.

> >+  operator bool() const { return node; }
> 
> I'm not really sure making this implicit instead of an explicit .set() or
> something is great.

please elaborate, I did that to allow "if (point)" what looked handy, what disadvantages?
(In reply to alexander :surkov from comment #2)
> (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> > Comment on attachment 8338832 [details] [diff] [review]
> > patch
> > 
> > >+  if (child->IsTextLeaf()) {
> > >+    nsIContent* content = child->GetContent();
> > >+    int32_t idx = 0;
> > >+    RenderedToContentOffset(content->GetPrimaryFrame(), innerOffset, &idx);
> > 
> > that can fail right? shouldn't you check it?
> 
> ok if you want

until we fix it to not fail that would seem best.

> > >+  nsINode* node = child->GetNode();
> > >+  nsINode* parentNode = node->GetParentNode();
> > >+  return parentNode ?
> > >+    DOMPoint(parentNode, parentNode->IndexOf(node) + innerOffset) :
> > >+    DOMPoint();
> > 
> > hm, if its not a text leaf and has width > 1 then doesn't it have to be a
> > hypertext and you could call OfsetToDOMPoint() on it?
> 
> not really, it's DOM point, it shouldn't be accessible hierarchy oriented.

Well, your converting and you already use accessible tree a bit.

It seems like it would be nice to have property that point is either in accessible in which case point is accessible->node, offset or if its within a child  child->GetPoint(offset - offsetOfChild)

> > >+  operator bool() const { return node; }
> > 
> > I'm not really sure making this implicit instead of an explicit .set() or
> > something is great.
> 
> please elaborate, I did that to allow "if (point)" what looked handy, what
> disadvantages?

people can now use if (point) even if that's not really what they meant.  It just looks not quiet obvious what if (some struct) means.  You can also end up converting a point to a bool in other context you might not expect it I believe.
(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> It seems like it would be nice to have property that point is either in
> accessible in which case point is accessible->node, offset or if its within
> a child  child->GetPoint(offset - offsetOfChild)

single offset in the hypertext can be described by more than one DOM point. Anyway we feed the DOM point to content/layout so why would we care to relate it this or that way with hypertext accessible if it already describes the offset?

> people can now use if (point) even if that's not really what they meant.  It
> just looks not quiet obvious what if (some struct) means.  You can also end
> up converting a point to a bool in other context you might not expect it I
> believe.

ok, if you're not fan of overridden bool operators, then what would you suggest instead "if (point)"? if (point.node) or something more fancy? :)
(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> 
> > It seems like it would be nice to have property that point is either in
> > accessible in which case point is accessible->node, offset or if its within
> > a child  child->GetPoint(offset - offsetOfChild)
> 
> single offset in the hypertext can be described by more than one DOM point.

when?

> Anyway we feed the DOM point to content/layout so why would we care to
> relate it this or that way with hypertext accessible if it already describes
> the offset?

I don't see how we would be, its just making logic simpler.

> > people can now use if (point) even if that's not really what they meant.  It
> > just looks not quiet obvious what if (some struct) means.  You can also end
> > up converting a point to a bool in other context you might not expect it I
> > believe.
> 
> ok, if you're not fan of overridden bool operators, then what would you
> suggest instead "if (point)"? if (point.node) or something more fancy? :)

if (point.node) seems fine
(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> (In reply to alexander :surkov from comment #4)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > 
> > > It seems like it would be nice to have property that point is either in
> > > accessible in which case point is accessible->node, offset or if its within
> > > a child  child->GetPoint(offset - offsetOfChild)
> > 
> > single offset in the hypertext can be described by more than one DOM point.
> 
> when?

offset 0 in

<p>hello</p>

can be described by (p, 0) and (textnode, 0)

> > Anyway we feed the DOM point to content/layout so why would we care to
> > relate it this or that way with hypertext accessible if it already describes
> > the offset?
> 
> I don't see how we would be, its just making logic simpler.

I think I miss your point

> if (point.node) seems fine

ok if you want
(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > (In reply to alexander :surkov from comment #4)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > > 
> > > > It seems like it would be nice to have property that point is either in
> > > > accessible in which case point is accessible->node, offset or if its within
> > > > a child  child->GetPoint(offset - offsetOfChild)
> > > 
> > > single offset in the hypertext can be described by more than one DOM point.
> > 
> > when?
> 
> offset 0 in
> 
> <p>hello</p>
> 
> can be described by (p, 0) and (textnode, 0)

yeah, ok so which should function return?

> > > Anyway we feed the DOM point to content/layout so why would we care to
> > > relate it this or that way with hypertext accessible if it already describes
> > > the offset?
> > 
> > I don't see how we would be, its just making logic simpler.
> 
> I think I miss your point

I don't see the need for the code that handles the hyper text offset being within a child.  I think the algorithm should just be
base case this accessible is a text leaf then dom point is (mContent, offset)if offset >= 0 && offset < length else (NULL, 0)
otherwise this accessible must be hypertext accessible so find accessible containing hypertext offset and ask it for the related dom point.

Which is simple and easy to reason about.
(In reply to Trevor Saunders (:tbsaunde) from comment #7)

> > offset 0 in
> > 
> > <p>hello</p>
> > 
> > can be described by (p, 0) and (textnode, 0)
> 
> yeah, ok so which should function return?

it will return (textnode, 0) I think

> I don't see the need for the code that handles the hyper text offset being
> within a child.

are you about text children? can you point code you refer to pls?

>  I think the algorithm should just be
> base case this accessible is a text leaf then dom point is (mContent,
> offset)if offset >= 0 && offset < length else (NULL, 0)

mostly that's what the current alg does

> otherwise this accessible must be hypertext accessible so find accessible
> containing hypertext offset and ask it for the related dom point.

a non text child doesn't need to be hypertext accessible, for example, image accessible

anyway that recursive thing doesn't seem simpler because if we've got non text leaf child then we just return a DOM point for it
(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> 
> > > offset 0 in
> > > 
> > > <p>hello</p>
> > > 
> > > can be described by (p, 0) and (textnode, 0)
> > 
> > yeah, ok so which should function return?
> 
> it will return (textnode, 0) I think

seems like that's worth specifying.

> > I don't see the need for the code that handles the hyper text offset being
> > within a child.
> 
> are you about text children? can you point code you refer to pls?

not sure I understand first question, I'm talking about
>+  nsINode* node = child->GetNode();
>+  nsINode* parentNode = node->GetParentNode();
>+  return parentNode ?
>+    DOMPoint(parentNode, parentNode->IndexOf(node) + innerOffset) :
>+    DOMPoint();
> }

> >  I think the algorithm should just be
> > base case this accessible is a text leaf then dom point is (mContent,
> > offset)if offset >= 0 && offset < length else (NULL, 0)
> 
> mostly that's what the current alg does
> 
> > otherwise this accessible must be hypertext accessible so find accessible
> > containing hypertext offset and ask it for the related dom point.
> 
> a non text child doesn't need to be hypertext accessible, for example, image
> accessible

sure, if its not hypertext then you just return  (mContent, 0) I guess because its only 1 char wide right? or what does the patch return?

> anyway that recursive thing doesn't seem simpler because if we've got non
> text leaf child then we just return a DOM point for it

huh?
so do you suggest to return (contnet, 0) instead (parent, indexInParent), correct? That should work.
(In reply to alexander :surkov from comment #10)
> so do you suggest to return (contnet, 0) instead (parent, indexInParent),
> correct? That should work.

for case when offset is in child that is not text? yeah, I guess child->GetContent(), 0 would make sense, but add comment explaining what its for?
I missed the point we have innerOffset that can be either 0 or 1, 0 means before, 1 means after the child. Doing (child, 0) doesn't work for innerOffest = 1 case. So I'd keep the logic of DOM points relative to the parent, that's what we had and it works for before and after cases.
(In reply to alexander :surkov from comment #12)
> I missed the point we have innerOffset that can be either 0 or 1, 0 means
> before, 1 means after the child. Doing (child, 0) doesn't work for
> innerOffest = 1 case. So I'd keep the logic of DOM points relative to the
> parent, that's what we had and it works for before and after cases.

can we do (child, 0) or (child, 1) for those cases respectively, or do we not handle offset == length + 1 anywhere else?

I guess what we have is ok if you had comments explaining why its that way / what it handles / what it does
(In reply to Trevor Saunders (:tbsaunde) from comment #13)

> can we do (child, 0) or (child, 1) for those cases respectively, or do we
> not handle offset == length + 1 anywhere else?

I don't think len + 1 offset makes sense

> I guess what we have is ok if you had comments explaining why its that way /
> what it handles / what it does

can you give a hint what comment you would like. A general practice is if you need to set a point before element then you do (parent, indexInParent), if inside the elm then (elm, 0), if inside the elm at the end then (elm, childCount), if right after the elm then (parent, indexInParent + 1).
(In reply to alexander :surkov from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #13)
> 
> > can we do (child, 0) or (child, 1) for those cases respectively, or do we
> > not handle offset == length + 1 anywhere else?
> 
> I don't think len + 1 offset makes sense
> 
> > I guess what we have is ok if you had comments explaining why its that way /
> > what it handles / what it does
> 
> can you give a hint what comment you would like. A general practice is if
> you need to set a point before element then you do (parent, indexInParent),
> if inside the elm then (elm, 0), if inside the elm at the end then (elm,
> childCount), if right after the elm then (parent, indexInParent + 1).

well, for a start you could add comment like that saying what method does exactly.

Then for this part maybe say something like

// Handle case of non text embedded object. point is either before or after object.

you could also assert innerOffset == 0 || innerOffset == 1 right?
Attached patch patch2 (obsolete) — Splinter Review
is it better?
Attachment #8338832 - Attachment is obsolete: true
Attachment #8338832 - Flags: review?(trev.saunders)
Attachment #8345381 - Flags: review?(trev.saunders)
Comment on attachment 8345381 [details] [diff] [review]
patch2

>-          aRange->SetEnd(editorRoot, 0);
>-
>-          return NS_OK;
>-        }
>+        return DOMPoint(editorRoot.get(), 0);

why do you need .get() ?

>+  // The point is either before or after the element.

add something about the accessible being an embedded object?

>+  operator bool() const { return node; }

I thought you were going to get rid of this?

>+   * Covert the given offset into DOM point.
>+   */

s/covert/convert/  Also why not say what values it returns for different offsets?
(In reply to Trevor Saunders (:tbsaunde) from comment #17)

> >+        return DOMPoint(editorRoot.get(), 0);
> 
> why do you need .get() ?

ok

> >+  // The point is either before or after the element.
> 
> add something about the accessible being an embedded object?

if we ever supported hypertext accessibles as not embedded objects (bug 861681) then the comment wouldn't be applied anymore while the logic stays unchanged

> >+  operator bool() const { return node; }
> 
> I thought you were going to get rid of this?

ok, forgot it.

> 
> s/covert/convert/ 

ok

 Also why not say what values it returns for different
> offsets?

surely one offset can be described by many DOM points but which one will be returned is rather an implementation detail
> > >+  // The point is either before or after the element.
> > 
> > add something about the accessible being an embedded object?
> 
> if we ever supported hypertext accessibles as not embedded objects (bug
> 861681) then the comment wouldn't be applied anymore while the logic stays
> unchanged

its not clear thats true suppose we made it so hyper text child takes up more than 1 char in parent then I don't think we'd want to keep this logic.

>  Also why not say what values it returns for different
> > offsets?
> 
> surely one offset can be described by many DOM points but which one will be
> returned is rather an implementation detail

it doesn't really seem that way especially since we just talked about why it had to work the way it does.
(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> > > >+  // The point is either before or after the element.
> > > 
> > > add something about the accessible being an embedded object?
> > 
> > if we ever supported hypertext accessibles as not embedded objects (bug
> > 861681) then the comment wouldn't be applied anymore while the logic stays
> > unchanged
> 
> its not clear thats true suppose we made it so hyper text child takes up
> more than 1 char in parent then I don't think we'd want to keep this logic.

ok

> >  Also why not say what values it returns for different
> > > offsets?
> > 
> > surely one offset can be described by many DOM points but which one will be
> > returned is rather an implementation detail
> 
> it doesn't really seem that way especially since we just talked about why it
> had to work the way it does.

I meant if we returned (p, 1) or (a, 0) for <p>hello <a href="">hello</p> for offset 6 then it doesn't really make difference since they both describe that offset, no?
Attached patch patch3Splinter Review
Attachment #8345381 - Attachment is obsolete: true
Attachment #8345381 - Flags: review?(trev.saunders)
Attachment #8346580 - Flags: review?(trev.saunders)
> > >  Also why not say what values it returns for different
> > > > offsets?
> > > 
> > > surely one offset can be described by many DOM points but which one will be
> > > returned is rather an implementation detail
> > 
> > it doesn't really seem that way especially since we just talked about why it
> > had to work the way it does.
> 
> I meant if we returned (p, 1) or (a, 0) for <p>hello <a href="">hello</p>
> for offset 6 then it doesn't really make difference since they both describe
> that offset, no?

I thought that was the case we were just discussing and there was a reason it couldn't be (a, 0)
(In reply to Trevor Saunders (:tbsaunde) from comment #22)

> > I meant if we returned (p, 1) or (a, 0) for <p>hello <a href="">hello</p>
> > for offset 6 then it doesn't really make difference since they both describe
> > that offset, no?
> 
> I thought that was the case we were just discussing and there was a reason
> it couldn't be (a, 0)

yes but if it was (a, 0) then I bet nobody was hurt (the content/layout part shouldn't really care), so that's why I'm saying it's rather implementation detail and it shouldn't be a big deal to move it to the method description. But if you think it's better to have it then would you please give me wording you would like and I will just add it?
(In reply to alexander :surkov from comment #23)
> (In reply to Trevor Saunders (:tbsaunde) from comment #22)
> 
> > > I meant if we returned (p, 1) or (a, 0) for <p>hello <a href="">hello</p>
> > > for offset 6 then it doesn't really make difference since they both describe
> > > that offset, no?
> > 
> > I thought that was the case we were just discussing and there was a reason
> > it couldn't be (a, 0)
> 
> yes but if it was (a, 0) then I bet nobody was hurt (the content/layout part
> shouldn't really care), so that's why I'm saying it's rather implementation
> detail and it shouldn't be a big deal to move it to the method description.

doesn't comment 12 explain why that isn't the case?

> But if you think it's better to have it then would you please give me
> wording you would like and I will just add it?

just use last bit of comment 14?
(In reply to Trevor Saunders (:tbsaunde) from comment #24)

> > > I thought that was the case we were just discussing and there was a reason
> > > it couldn't be (a, 0)
> > 
> > yes but if it was (a, 0) then I bet nobody was hurt (the content/layout part
> > shouldn't really care), so that's why I'm saying it's rather implementation
> > detail and it shouldn't be a big deal to move it to the method description.
> 
> doesn't comment 12 explain why that isn't the case?

not sure I can see how

> > But if you think it's better to have it then would you please give me
> > wording you would like and I will just add it?
> 
> just use last bit of comment 14?

ok, do you need a patch for that? anything else?
Attachment #8346580 - Flags: review?(trev.saunders) → review+
https://hg.mozilla.org/mozilla-central/rev/6aad311883cc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: