789621 - getTextAttributes doesn't work with magic offsets

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

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 (:asurkov)]

Please check all methods from nsIAccessibleText/nsIAccessibleEditableText that they work with magic offsets.

Comment 2

[russellspivey]

(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 (:asurkov)]

(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]

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 (:asurkov)]

(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]

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 (:asurkov)]

(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]

Attachment: 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] (NeedInfo me for attention)]

(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]

Hey David,
I will make a request Alexander to do it.
Thanks!

Comment 11

[alexander :surkov (:asurkov)]

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]

Attachment: 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]]

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]

Attachment: 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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc8044790a95

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fc8044790a95