Closed Bug 789621 Opened 10 years ago Closed 10 years ago

getTextAttributes doesn't work with magic offsets

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: surkov, Assigned: cabelitostos)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=hub@mozilla.com][lang=c++])

Attachments

(1 file, 2 obsolete files)

steps to fix:
1) use ConvertMagicOffset in HyperTextAccessible::GetTextAttributes
2) add a test under attributes/test_text.html (-1 end of test offset works)
Please check all methods from nsIAccessibleText/nsIAccessibleEditableText that they work with magic offsets.
(In reply to alexander :surkov from comment #1)
> Please check all methods from nsIAccessibleText/nsIAccessibleEditableText
> that they work with magic offsets.

I'm new and I'd like to work on this! How do we know if nsIAccessibleText/nsIAccessibleEditableText methods work with magic offsets?
(In reply to russellspivey from comment #2)
> (In reply to alexander :surkov from comment #1)
> > Please check all methods from nsIAccessibleText/nsIAccessibleEditableText
> > that they work with magic offsets.
> 
> I'm new and I'd like to work on this! How do we know if
> nsIAccessibleText/nsIAccessibleEditableText methods work with magic offsets?

These interfaces are implemented by generic/HyperTextAccessible class. You need to check out if those implementations use ConvertMagicOffset method to convert the given offsets.

For example, nsIAccessibleText interface has getTextAttributes method and it is implemented by HyperTextAccessible::GetTextAttributes. As you see the argument int32_t aOffset isn't processed by ConvertMagicOffset.
Hello Alexander,

Can I pick thi bug?
Btw I'm new in the FF community.
And before you say something, let me see if I understood what I need to do.
I shall look on every implemented function of the interfaces nsIAccessibleText and nsIAccessibleEditableText and see if its implementation is using ConvertMagicOffset before using the offset?
(In reply to gui.iscaro from comment #4)
> Hello Alexander,
> 
> Can I pick thi bug?

welcome

> Btw I'm new in the FF community.
> And before you say something, let me see if I understood what I need to do.
> I shall look on every implemented function of the interfaces
> nsIAccessibleText and nsIAccessibleEditableText and see if its
> implementation is using ConvertMagicOffset before using the offset?

that's right. you might want to look one level deeply if the method uses helper functions.
Assignee: nobody → cabelitostos
Thanks Alexander.
What's the deadline for this?(In reply to alexander :surkov from comment #5)
> (In reply to gui.iscaro from comment #4)
> > Hello Alexander,
> > 
> > Can I pick thi bug?
> 
> welcome
> 
> > Btw I'm new in the FF community.
> > And before you say something, let me see if I understood what I need to do.
> > I shall look on every implemented function of the interfaces
> > nsIAccessibleText and nsIAccessibleEditableText and see if its
> > implementation is using ConvertMagicOffset before using the offset?
> 
> that's right. you might want to look one level deeply if the method uses
> helper functions.

Thanks Alexander.
What's the deadline for this?
(In reply to gui.iscaro from comment #6)

> What's the deadline for this?

please don't get lost but if you do then let us know so we can get back the bug into the pull
This is a first version.
Can someone review it please?
I would like to know if I did everything right.
Thanks!
(In reply to gui.iscaro from comment #8)
> Created attachment 698700 [details] [diff] [review]
> HyperTextAccessible implemented functions from nsIAccessibleText and
> nsIAccessibleEditableText should use ConvertMagicOffset when possible
> 
> This is a first version.
> Can someone review it please?
> I would like to know if I did everything right.
> Thanks!

Hi and thanks for the patch! Normally you request review from someone
https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed
Hey David,
I will make a request Alexander to do it.
Thanks!
Attachment #698700 - Flags: review?(surkov.alexander)
Comment on attachment 698700 [details] [diff] [review]
HyperTextAccessible implemented functions from nsIAccessibleText and nsIAccessibleEditableText should use ConvertMagicOffset when possible

Review of attachment 698700 [details] [diff] [review]:
-----------------------------------------------------------------

I suspect that scrollSubstringTo/scrollSubstringToPoint won't be working in edge cases like scrollSubstringTo(-1, -1) when text is empty. But it can be left for followup. Otherwise it looks good.

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +1039,5 @@
>  
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
> +  aOffset = ConvertMagicOffset(aOffset);

please use local variable instead

@@ +1805,5 @@
>    if (aSelectionNum < 0)
>      return NS_ERROR_INVALID_ARG;
>  
> +  aStartOffset = ConvertMagicOffset(aStartOffset);
> +  aEndOffset = ConvertMagicOffset(aEndOffset);

same here

::: accessible/tests/mochitest/attributes/test_text.html
@@ +720,5 @@
>    <p id="area19">uncolored
>      <mark>colored</mark> uncolored
>    </p>
> +
> +  <p id="area20" style="font-size: 15pt;">offset test</p>

it'd be good to refer to this bug number (see <a target="_blank" items)

@@ +725,2 @@
>     
>  </body>

nit: while you are here: please remove excess whitespace
Attachment #698700 - Flags: review?(surkov.alexander) → review+
Attached patch Version two (obsolete) — Splinter Review
Hello Alexander,
Thanks for you review. I made the changes that you asked, also I didn't change the methods scrollSubstringTo/scrollSubstringToPoint like you said.

Can you review the patch again please?
Thanks!
Attachment #699673 - Flags: review?(surkov.alexander)
Attachment #699673 - Flags: review?(surkov.alexander) → review+
Keywords: checkin-needed
Comment on attachment 698700 [details] [diff] [review]
HyperTextAccessible implemented functions from nsIAccessibleText and nsIAccessibleEditableText should use ConvertMagicOffset when possible

Gui: Could you follow the steps here...
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
...to get the author (your name / email address, however you want them formatted in our commit history) & a commit message included in the patch?

Thanks!

[Also: I'm marking the first patch here as obsolete, since it looks like the second patch is intended to replace it]
Attachment #698700 - Attachment is obsolete: true
Hello Daniel,

Sorry for the incorrect patch format, and thanks for warning me.
I think we're all set now.

Thanks!
Attachment #701454 - Flags: checkin?(dbolter)
Attachment #699673 - Attachment is obsolete: true
Attachment #701454 - Flags: checkin?(dbolter)
https://hg.mozilla.org/mozilla-central/rev/fc8044790a95
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.