The default bug view has changed. See this FAQ.

Status

()

Core
Disability Access APIs
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Aaron Leventhal, Assigned: Nian Liu(n/a in a long time))

Tracking

({access})

Trunk
x86
Linux
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: testcase-needed)

Attachments

(3 attachments, 9 obsolete attachments)

(Reporter)

Description

11 years ago
This is the Firefox portion of this Gnome bug:
http://bugzilla.gnome.org/show_bug.cgi?id=344284

There is a new interfaces to help us deal with the fact that links are not interfaces in ATK, but objects. In Firefox we just treat it like another interface.

Take a look at this include file:
#include <atk/atkhyperlinkimpl.h>

Updated

11 years ago
Status: NEW → ASSIGNED

Updated

11 years ago
Assignee: ginn.chen → nian.liu
Status: ASSIGNED → NEW
(Assignee)

Comment 1

11 years ago
Created attachment 230092 [details] [diff] [review]
patch

patch without new added files
Attachment #230092 - Flags: review?(aaronleventhal)
(Assignee)

Comment 2

11 years ago
Created attachment 230093 [details] [diff] [review]
.h file
Attachment #230093 - Flags: review?(aaronleventhal)
(Assignee)

Comment 3

11 years ago
Created attachment 230094 [details] [diff] [review]
.cpp file
Attachment #230094 - Flags: review?(aaronleventhal)
(Reporter)

Updated

11 years ago
Attachment #230092 - Flags: review?(aaronleventhal) → review?(ginn.chen)
(Reporter)

Updated

11 years ago
Attachment #230093 - Flags: review?(aaronleventhal) → review?(ginn.chen)
(Reporter)

Comment 4

11 years ago
Comment on attachment 230094 [details] [diff] [review]
.cpp file

I'm traveling and won't be available to review for a few days. Anyway, Ginn understands that ATK/AT-SPI architecture better than I.
Attachment #230094 - Flags: review?(aaronleventhal) → review?(ginn.chen)

Comment 5

11 years ago
Comment on attachment 230092 [details] [diff] [review]
patch

+    if (accessInterfaceHyperlink) {
+       interfacesBits |= 1 << MAI_INTERFACE_HYPERLINK;
+    }
     //nsIAccessibleTable

Nit: add a blank line after }

   nsMaiInterfaceTable.cpp \
+  nsMaiInterfaceHyperlinkImpl.cpp \
   $(NULL)
we need ATK version check here.
And we need to make sure it can work well with low version ATK.
Attachment #230092 - Flags: review?(ginn.chen) → review-

Comment 6

11 years ago
Comment on attachment 230094 [details] [diff] [review]
.cpp file

    nsresult rv;

not used.

Neo, do you have a method or python script to verify the patch?
Attachment #230094 - Flags: review?(ginn.chen) → review-
(Assignee)

Comment 7

11 years ago
Created attachment 230398 [details] [diff] [review]
patchv2

addressing ginn's comment
Attachment #230092 - Attachment is obsolete: true
Attachment #230398 - Flags: review?(ginn.chen)
(Assignee)

Comment 8

11 years ago
> Neo, do you have a method or python script to verify the patch?
> 

nope:(

it's not called from firefox side. add bill.haneman@sun.com to see if he can make one

Comment 9

11 years ago
Comment on attachment 230398 [details] [diff] [review]
patchv2

1) it should be ATK 1.12.1
2) it won't compile with ATK below 1.12.1
because you uses ATK_TYPE_HYPERLINK_IMPL in nsAccessibleWrap.cpp
Attachment #230398 - Flags: review?(ginn.chen) → review-
(Assignee)

Comment 10

11 years ago
Created attachment 230402 [details] [diff] [review]
patch

oops, strong coffee needed
Attachment #230398 - Attachment is obsolete: true
Attachment #230402 - Flags: review?(ginn.chen)

Updated

11 years ago
Whiteboard: testcase-needed
(Reporter)

Comment 11

11 years ago
Where are we at with this patch? It's probably the most important thing we need right now.

Comment 12

11 years ago
(In reply to comment #11)
> Where are we at with this patch? It's probably the most important thing we need
> right now.
> 

I would like to have a unit test to make sure it really works before checking in.
(Reporter)

Comment 13

11 years ago
Ginn, would it help with testing to get this in? At least that way it won't bitrot. We can always fix it or totally remove it if it ends up being wrong in tests. It is trunk, and as long as we don't break anything too bad I think we should check it in if the code looks right.

Comment 14

11 years ago
at-spi doesn't have AtkHyperlinkImpl interface yet.
So these code can do nothing now.
(Reporter)

Comment 15

11 years ago
Now that we're building with recent ATK headers, we should have an easier time testing this, correct? It seems like we should get this in soon, because it is blocking more testing by screen reader developers.

The AT-SPI Hyperlink interface is returning incorrect info for startIndex, endIndex and crashing for getURI. We think this will probably fix that.
(Assignee)

Comment 16

11 years ago
Created attachment 235702 [details] [diff] [review]
patch

verified with at-poke at-spi atk link
Attachment #230093 - Attachment is obsolete: true
Attachment #230094 - Attachment is obsolete: true
Attachment #230402 - Attachment is obsolete: true
Attachment #235702 - Flags: review?(ginn.chen)
Attachment #230093 - Flags: review?(ginn.chen)
Attachment #230402 - Flags: review?(ginn.chen)
(Assignee)

Comment 17

11 years ago
Created attachment 235703 [details] [diff] [review]
.h file
Attachment #235703 - Flags: review?(ginn.chen)
(Assignee)

Comment 18

11 years ago
Created attachment 235705 [details] [diff] [review]
.cpp file
Attachment #235705 - Flags: review?(ginn.chen)

Updated

11 years ago
Attachment #235702 - Flags: review?(ginn.chen) → review+

Updated

11 years ago
Attachment #235703 - Flags: review?(ginn.chen) → review+

Updated

11 years ago
Attachment #235705 - Flags: review?(ginn.chen) → review+

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 19

11 years ago
patch backed out

It could not link on tinderbox machine.

undefined reference to `atk_hyperlink_impl_get_type'
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 20

11 years ago
Created attachment 235729 [details] [diff] [review]
patch to remove compile error
Attachment #235729 - Flags: review?(ginn.chen)
(Assignee)

Updated

11 years ago
Attachment #235729 - Attachment is obsolete: true
Attachment #235729 - Flags: review?(ginn.chen)
(Assignee)

Comment 21

11 years ago
Created attachment 235730 [details] [diff] [review]
patch to remove compile error
Attachment #235702 - Attachment is obsolete: true
Attachment #235730 - Flags: review?(ginn.chen)
(Assignee)

Comment 22

11 years ago
Created attachment 235853 [details] [diff] [review]
patch to remove compile error v2(variable name convension modify)
Attachment #235730 - Attachment is obsolete: true
Attachment #235853 - Flags: review?(ginn.chen)
Attachment #235730 - Flags: review?(ginn.chen)

Comment 23

11 years ago
Created attachment 236042 [details] [diff] [review]
patch to remove compile error v2

I changed a little bit.
Attachment #235853 - Attachment is obsolete: true
Attachment #236042 - Flags: review?(nian.liu)
Attachment #235853 - Flags: review?(ginn.chen)
(Assignee)

Comment 24

11 years ago
Comment on attachment 236042 [details] [diff] [review]
patch to remove compile error v2

fine for me
Attachment #236042 - Flags: review?(nian.liu) → review+

Updated

11 years ago
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 25

11 years ago
(In reply to comment #15)
> Now that we're building with recent ATK headers, we should have an easier time
> testing this, correct? It seems like we should get this in soon, because it is
> blocking more testing by screen reader developers.
> 
> The AT-SPI Hyperlink interface is returning incorrect info for startIndex,
> endIndex and crashing for getURI. We think this will probably fix that.
> 

Aaron, this patch does no help on above issue. 
(Reporter)

Comment 26

11 years ago
Okay, file a bug. I think Ginn Chen will need to look at it, unless you think you can debug it.
You need to log in before you can comment on or make changes to this bug.