Last Comment Bug 789621 - getTextAttributes doesn't work with magic offsets
: getTextAttributes doesn't work with magic offsets
Status: RESOLVED FIXED
[good first bug][mentor=hub@mozilla.c...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla21
Assigned To: gui.iscaro
:
: alexander :surkov
Mentors:
Depends on:
Blocks: textattra11y
  Show dependency treegraph
 
Reported: 2012-09-07 17:01 PDT by alexander :surkov
Modified: 2013-01-13 07:25 PST (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
HyperTextAccessible implemented functions from nsIAccessibleText and nsIAccessibleEditableText should use ConvertMagicOffset when possible (3.81 KB, patch)
2013-01-07 07:48 PST, gui.iscaro
surkov.alexander: review+
Details | Diff | Splinter Review
Version two (6.45 KB, patch)
2013-01-09 03:35 PST, gui.iscaro
surkov.alexander: review+
Details | Diff | Splinter Review
Adding author and commit message (6.60 KB, patch)
2013-01-12 05:34 PST, gui.iscaro
no flags Details | Diff | Splinter Review

Description alexander :surkov 2012-09-07 17:01:03 PDT
steps to fix:
1) use ConvertMagicOffset in HyperTextAccessible::GetTextAttributes
2) add a test under attributes/test_text.html (-1 end of test offset works)
Comment 1 alexander :surkov 2012-09-07 17:05:17 PDT
Please check all methods from nsIAccessibleText/nsIAccessibleEditableText that they work with magic offsets.
Comment 2 russellspivey 2012-09-08 13:11:35 PDT
(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?
Comment 3 alexander :surkov 2012-09-13 20:59:20 PDT
(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.
Comment 4 gui.iscaro 2013-01-05 04:35:12 PST
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?
Comment 5 alexander :surkov 2013-01-05 07:31:44 PST
(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.
Comment 6 gui.iscaro 2013-01-05 08:28:14 PST
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?
Comment 7 alexander :surkov 2013-01-05 08:59:08 PST
(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
Comment 8 gui.iscaro 2013-01-07 07:48:30 PST
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!
Comment 9 David Bolter [:davidb] 2013-01-07 08:42:40 PST
(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
Comment 10 gui.iscaro 2013-01-07 15:04:13 PST
Hey David,
I will make a request Alexander to do it.
Thanks!
Comment 11 alexander :surkov 2013-01-08 17:11:22 PST
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
Comment 12 gui.iscaro 2013-01-09 03:35:06 PST
Created attachment 699673 [details] [diff] [review]
Version two

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!
Comment 13 Daniel Holbert [:dholbert] 2013-01-11 09:25:43 PST
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]
Comment 14 gui.iscaro 2013-01-12 05:34:29 PST
Created attachment 701454 [details] [diff] [review]
Adding author and commit message

Hello Daniel,

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

Thanks!
Comment 15 Ryan VanderMeulen [:RyanVM] 2013-01-12 12:28:52 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc8044790a95
Comment 16 Ryan VanderMeulen [:RyanVM] 2013-01-13 07:25:12 PST
https://hg.mozilla.org/mozilla-central/rev/fc8044790a95

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