Closed
Bug 943612
Opened 11 years ago
Closed 11 years ago
don't use GetPosAndText to convert offsets to DOM range
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
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)
(deleted),
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
a second strike to get rid of GetPosAndText
Attachment #8338832 -
Flags: review?(trev.saunders)
Comment 1•11 years ago
|
||
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.
Assignee | ||
Comment 2•11 years ago
|
||
(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?
Comment 3•11 years ago
|
||
(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.
Assignee | ||
Comment 4•11 years ago
|
||
(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? :)
Comment 5•11 years ago
|
||
(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
Assignee | ||
Comment 6•11 years ago
|
||
(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
Comment 7•11 years ago
|
||
(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.
Assignee | ||
Comment 8•11 years ago
|
||
(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
Comment 9•11 years ago
|
||
(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?
Assignee | ||
Comment 10•11 years ago
|
||
so do you suggest to return (contnet, 0) instead (parent, indexInParent), correct? That should work.
Comment 11•11 years ago
|
||
(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?
Assignee | ||
Comment 12•11 years ago
|
||
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.
Comment 13•11 years ago
|
||
(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
Assignee | ||
Comment 14•11 years ago
|
||
(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).
Comment 15•11 years ago
|
||
(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?
Assignee | ||
Comment 16•11 years ago
|
||
is it better?
Attachment #8338832 -
Attachment is obsolete: true
Attachment #8338832 -
Flags: review?(trev.saunders)
Attachment #8345381 -
Flags: review?(trev.saunders)
Comment 17•11 years ago
|
||
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?
Assignee | ||
Comment 18•11 years ago
|
||
(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
Comment 19•11 years ago
|
||
> > >+ // 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.
Assignee | ||
Comment 20•11 years ago
|
||
(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?
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #8345381 -
Attachment is obsolete: true
Attachment #8345381 -
Flags: review?(trev.saunders)
Attachment #8346580 -
Flags: review?(trev.saunders)
Comment 22•11 years ago
|
||
> > > 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)
Assignee | ||
Comment 23•11 years ago
|
||
(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?
Comment 24•11 years ago
|
||
(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?
Assignee | ||
Comment 25•11 years ago
|
||
(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?
Updated•11 years ago
|
Attachment #8346580 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Comment 26•11 years ago
|
||
Comment 27•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•11 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•