The default bug view has changed. See this FAQ.

getTextAttributes doesn't work with magic offsets

RESOLVED FIXED in mozilla21

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: surkov, Assigned: gui.iscaro)

Tracking

(Blocks: 1 bug)

unspecified
mozilla21
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
steps to fix:
1) use ConvertMagicOffset in HyperTextAccessible::GetTextAttributes
2) add a test under attributes/test_text.html (-1 end of test offset works)
(Reporter)

Comment 1

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

Comment 2

5 years ago
(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?
(Reporter)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

4 years ago
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?
(Reporter)

Comment 5

4 years ago
(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.
(Reporter)

Updated

4 years ago
Assignee: nobody → cabelitostos
(Assignee)

Comment 6

4 years ago
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?
(Reporter)

Comment 7

4 years ago
(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
(Assignee)

Comment 8

4 years ago
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!
(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
(Assignee)

Comment 10

4 years ago
Hey David,
I will make a request Alexander to do it.
Thanks!
(Assignee)

Updated

4 years ago
Attachment #698700 - Flags: review?(surkov.alexander)
(Reporter)

Comment 11

4 years ago
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+
(Assignee)

Comment 12

4 years ago
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!
Attachment #699673 - Flags: review?(surkov.alexander)
(Reporter)

Updated

4 years ago
Attachment #699673 - Flags: review?(surkov.alexander) → review+
(Assignee)

Updated

4 years ago
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
(Assignee)

Comment 14

4 years ago
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!
Attachment #701454 - Flags: checkin?(dbolter)
Attachment #699673 - Attachment is obsolete: true
Attachment #701454 - Flags: checkin?(dbolter)
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc8044790a95
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fc8044790a95
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.