396025 - Accessible object created for tbody/thead/tfoot even when ARIA role is on parent table

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

Those accessibles should obey role="presentation" on the parent table -- that is, they should not be created.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Aaron Leventhal]

Comment on attachment 280842 [details] [diff] [review]
patch

I should have said that any role on the table means don't create the tbody accessible.

Use nsAccessNode::HasRoleAttribute(tableContent)) {) for that -- in fact you should be able to combine with the previous if.

However, one other concern I have -- doesn't CreateHTMLAccessibleByMarkup() above create the accessible for the tbody? That used to come after these checks, I believe.

Comment 3

[alexander :surkov (:asurkov)]

I am blind because I didn't see my code won't ever work since HasRoleAttribute() is presented in the check above. So this bug is about we shouldn't create accessible for tbody/tfoot/thead in CreateHTMLAccessibleByMarkup() if table has role attribute, correct?

Comment 4

[Aaron Leventhal]

Right. I think we might want to move CreateHTMLAccessibleByMarkup() down just a little bit, but keep it before the frame->GetAccessible(). Because, it does the check for us already. We just need to use the info better, right?

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 6

[Aaron Leventhal]

Comment on attachment 281079 [details] [diff] [review]
patch2

We're now using frame->GetAccessible() when we're on a table frame and there is an ARIA role on the table. That means we'll be creating accessibles for table cells etc. via nsTableCellFrame::GetAccessible() when we shouldn't be.

Also, I think the name noTableRelatedAcc is confusing. How about allowTableCellFrame. And if it is false, don't call frame->GetAccessible() either.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch3

you're right, I should be more carefull

Comment 8

[Aaron Leventhal]

Comment on attachment 281083 [details] [diff] [review]
patch3

Code is fine.

Variable name: Hmm, I realize I asked you to change the name to allowTableRelatedAcc, but the |if (allowTableRelated)| { create acc based on tag or frame type)| is strange. We can't just return early, because we need to use the accessible for generic attributes block below. Maybe call it |PRBool tryTagNameOrFrame|.

Comment 9

[alexander :surkov (:asurkov)]

Agree. I think your new name is fine. I'll change before checkin.

Comment 10

[alexander :surkov (:asurkov)]

checked in with changed name