Closed Bug 1220897 Opened 6 years ago Closed 6 years ago

fix IAccessible2::get_accessibleWithCaret

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: surkov, Assigned: surkov)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #8682218 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8682218 [details] [diff] [review]
patch

>diff --git a/accessible/windows/ia2/ia2Accessible.cpp b/accessible/windows/ia2/ia2Accessible.cpp
>--- a/accessible/windows/ia2/ia2Accessible.cpp
>+++ b/accessible/windows/ia2/ia2Accessible.cpp
>@@ -658,24 +658,25 @@ ia2Accessible::get_accessibleWithCaret(I
>     return CO_E_OBJNOTCONNECTED;
> 
>   int32_t caretOffset = -1;
>   Accessible* accWithCaret = SelectionMgr()->AccessibleWithCaret(&caretOffset);
>   if (acc->Document() != accWithCaret->Document())
>     return S_FALSE;
> 
>   Accessible* child = accWithCaret;
>-  while (child != acc)
>+  while (child && child != acc)

checking if child is a document should be faster, and really this should be its own patch.
Attachment #8682218 - Flags: review?(tbsaunde+mozbugs)
(In reply to Trevor Saunders (:tbsaunde) from comment #1)

> and really this should be
> its own patch.

what's the point?
Attached patch patch2 (obsolete) — Splinter Review
Attachment #8682218 - Attachment is obsolete: true
Attachment #8682737 - Flags: review?(tbsaunde+mozbugs)
(In reply to alexander :surkov from comment #2)
> (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> 
> > and really this should be
> > its own patch.
> 
> what's the point?

to be clear about the purpose of it is?
Comment on attachment 8682737 [details] [diff] [review]
patch2

>diff --git a/accessible/windows/ia2/ia2Accessible.cpp b/accessible/windows/ia2/ia2Accessible.cpp
>--- a/accessible/windows/ia2/ia2Accessible.cpp
>+++ b/accessible/windows/ia2/ia2Accessible.cpp
>@@ -658,24 +658,25 @@ ia2Accessible::get_accessibleWithCaret(I
>     return CO_E_OBJNOTCONNECTED;
> 
>   int32_t caretOffset = -1;
>   Accessible* accWithCaret = SelectionMgr()->AccessibleWithCaret(&caretOffset);
>   if (acc->Document() != accWithCaret->Document())
>     return S_FALSE;
> 
>   Accessible* child = accWithCaret;
>-  while (child != acc)
>+  while (!child->IsDoc() && child != acc)
>     child = child->Parent();
> 
>   if (!child)

this is now buggy because child could be non null and point at the document.
Attachment #8682737 - Flags: review?(tbsaunde+mozbugs)
Attached patch patchSplinter Review
hope this one is ok :)
Attachment #8682737 - Attachment is obsolete: true
Attachment #8683746 - Flags: review?(tbsaunde+mozbugs)
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> (In reply to alexander :surkov from comment #2)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> > 
> > > and really this should be
> > > its own patch.
> > 
> > what's the point?
> 
> to be clear about the purpose of it is?

proper commit message should work here
Summary: IAccessible2::get_accessibleWithCaret should addref a returned object → fix IAccessible2::get_accessibleWithCaret
Attachment #8683746 - Flags: review?(tbsaunde+mozbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/4761642b93dc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Assignee: nobody → surkov.alexander
You need to log in before you can comment on or make changes to this bug.