1178272 - Move table semantics to a separate mozTableAccessible.mm file

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Frédéric Wang (:fredw)]

Follow-up of bug 744790:

> it'd be good to move this code into separate class, that seems goes with current architecture, and you can skip IsTable() checks

Comment 1

[Frédéric Wang (:fredw)]

Attachment: Patch V1

Comment 2

[Frédéric Wang (:fredw)]

Attachment: Patch V2

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 8668556 [details] [diff] [review]
Patch V2

Review of attachment 8668556 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/mac/AccessibleWrap.mm
@@ +289,5 @@
>      case roles::LINK:
>        return [mozLinkAccessible class];
>  
> +    case roles::TABLE:
> +    case roles::MATHML_TABLE:

grid, treetable?

@@ +297,5 @@
> +    case roles::MATHML_TABLE_ROW:
> +      return [mozTableRowAccessible class];
> +
> +    case roles::CELL:
> +    case roles::MATHML_CELL:

gridcell, column/rowheader

can we use IsTableCell() instead?

it makes sense to check IsLayout stuff at this point

@@ +299,5 @@
> +
> +    case roles::CELL:
> +    case roles::MATHML_CELL:
> +      return [mozTableCellAccessible class];
> +      

nit: whitespaces

::: accessible/mac/mozTableAccessible.mm
@@ +8,5 @@
> +#import "mozTableAccessible.h"
> +#import "nsCocoaUtils.h"
> +
> +@implementation mozTablePartAccessible
> +- (BOOL)isLayoutTablePart;

maybe isForLayout or isLayout

Comment 4

[Frédéric Wang (:fredw)]

Attachment: Patch V3

This version uses the IsTable* functions instead of testing roles, so the class creation should be consistent with the current behavior.

However, the comment in https://dxr.mozilla.org/mozilla-central/source/accessible/mac/Platform.mm?offset=0#40 seems to suggest that IPC can not be used at this point:

1) ProxyAccessible::IsTable* functions won't work correctly if bug 1210477 is fixed by making these functions use IPC calls.

2) The ProxyAccessible::TableIsProbablyForLayout can not be used if we want to restrict the creation of tabular classes to only "real" tables.

Comment 5

[Frédéric Wang (:fredw)]

Attachment: Patch V4

Version 4 tries to move the "layout table" check at the creation for normal Accessibles, but this is not for Proxy Accessibles. I'm not sure putting the verification into different places makes the patch better than version 3...

Comment 6

[Frédéric Wang (:fredw)]

Attachment: Patch V3

updating V3 (fix build & whitespace errors)

Comment 7

[Frédéric Wang (:fredw)]

@Marco: can you test some tables with this build and tells whether anything is broken:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6bcfee452e28/try-macosx64/firefox-44.0a1.en-US.mac.dmg

Comment 8

[Marco Zehe (:MarcoZ)]

This feels like it still works as expected. I tried both a Bugzilla bug which has some genuine tables, and also the test case from bug 1177640 where I have a data table and a layout table. No crashes, and all exposure is as I would have expected.

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 8668721 [details] [diff] [review]
Patch V3

Review of attachment 8668721 [details] [diff] [review]:
-----------------------------------------------------------------

i cannot think of anything nicer, so rs=me

Comment 10

[Frédéric Wang (:fredw)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39335b8bc2b8

Comment 11

[Pulsebot]

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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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