Too many warnings in console for accessibility!

RESOLVED WORKSFORME

Status

()

RESOLVED WORKSFORME
12 years ago
7 years ago

People

(Reporter: aaronlev, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

Trunk
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [BK1])

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
When I run with Inspect or another accessibility tool on Windows, I get a lot of useless error spew.
(Reporter)

Comment 1

12 years ago
Created attachment 261824 [details] [diff] [review]
No doubt there is more to clean up, but this is a start
Attachment #261824 - Flags: review?(surkov.alexander)
(Assignee)

Updated

12 years ago
Attachment #261824 - Flags: review?(surkov.alexander) → review+
(Reporter)

Comment 2

12 years ago
Lets leave this open and go through the remaing warnings before we ship Firefox 3.

We should also check gcc warnings.
Blocks: 342901
(Reporter)

Comment 3

12 years ago
Surkov, nsAccessible::GetAttributes() spews a lot of messages here:
  NS_ENSURE_TRUE(attributes, NS_ERROR_FAILURE);
Assignee: aaronleventhal → surkov.alexander

Comment 4

12 years ago
gcc warnings of a11y module should be gone after checkin of bug 382296, bug 382293.

For debug console warnings,
There're a lot of NS_ENSURE_TRUE(accWrap, nsnull) should be changed to
if (!accWrap) return nsnull;
because accWrap could be null if it's shutdown

Comment 5

12 years ago
Created attachment 267133 [details] [diff] [review]
patch

1) For MAI callbacks,
change NS_ENSURE_TRUE(accWrap, XXX) to if (!accWrap) return XXX;
We don't need these warnings

2) Merge GetAccessibleWrap and CheckMaiAtkObject
kill console warnings

3) Implement static method GetAtkObject(nsIAccessible *) as surkov's suggest, see bug 382567 comment 3

4) nsAccessibleWrap::FireAtkPropChangedEvent doesn't look correct to me.
I removed some code from it. Anyway, this method will never be called.

5) GetRowDescription is not implemented, so remove the code of getRowDescriptionCB
kill console warnings

6) small logic changes in getNameCB, getDescriptionCB, we don't care empty string here.
Attachment #267133 - Flags: review?(surkov.alexander)

Updated

12 years ago
OS: Windows XP → All

Updated

12 years ago
Attachment #267133 - Flags: review?(aaronleventhal)
(Reporter)

Comment 6

12 years ago
Comment on attachment 267133 [details] [diff] [review]
patch

Having Surkov review should be enough.
Attachment #267133 - Flags: review?(aaronleventhal)
(Assignee)

Comment 7

12 years ago
Comment on attachment 267133 [details] [diff] [review]
patch

Sorry for delay in review. I have few comments.

> AtkRole
> getRoleCB(AtkObject *aAtkObj)
>+        NS_ASSERTION(nsAccessible::IsTextInterfaceSupportCorrect(accWrap), "Does not support nsIAccessibleText when it should");

nit: is there offset here?

> AtkObject *
> getParentCB(AtkObject *aAtkObj)
>+    if (!aAtkObj->accessible_parent) {

Is it possible accessible parent has been changed but aAtkObj->accessible_parent holds old value?

nsAccessibleWrap::FireAtkPropChangedEvent(nsIAccessibleEvent *aEvent,
>                                           AtkObject *aObject)

>+    //Perhaps need more cases in the future

I would suggest to do not remove the code. This is needed for ATK and IA2 awaits too something similar. We need to file foollowing up bug to support them in gecko.

>Index: nsMaiInterfaceTable.cpp
> const gchar*
> getRowDescriptionCB(AtkTable *aTable, gint aRow)
> {

I would suggest to do not remove this code because I guess it's ok to fail if method is not implementd. We should rather to implement this instead to remove ATK mapping of the method.
Attachment #267133 - Flags: review?(surkov.alexander) → review+

Comment 8

12 years ago
Patch committed.

> Is it possible accessible parent has been changed but
> aAtkObj->accessible_parent holds old value?

I don't think accessible parent could be changed.
I didn't change code logic here.

I leave FireAtkPropChangedEvent and getRowDescriptionCB alone.
Keep in mind, the code in FireAtkPropChangedEvent is probably wrong.
And we don't need this function for general properties, e.g. "accessible-name"
atk_object_set_name already does it for us.
(Reporter)

Updated

11 years ago
Blocks: 389800
No longer blocks: 342901

Comment 9

7 years ago
Closing WFM since we got rid of warnings as the module was developed further.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WORKSFORME
Whiteboard: [BK1]
(Assignee)

Comment 10

7 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #9)
> Closing WFM since we got rid of warnings as the module was developed further.

we did that but I'm not sure we don't have them a lot anymore. Marco did you have a change to check the console?
You need to log in before you can comment on or make changes to this bug.