Frédéric Wang (:fredw) Assignee
	Description • 9 years ago

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

	alexander :surkov (:asurkov)
	Comment 3 • 9 years ago

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

	Frédéric Wang (:fredw) Assignee
	Comment 4 • 9 years ago

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.

	Frédéric Wang (:fredw) Assignee
	Comment 5 • 9 years ago

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...

	Frédéric Wang (:fredw) Assignee
	Comment 6 • 9 years ago

updating V3 (fix build & whitespace errors)

	Frédéric Wang (:fredw) Assignee
	Comment 7 • 9 years ago

@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/fred.wang@free.fr-6bcfee452e28/try-macosx64/firefox-44.0a1.en-US.mac.dmg

	Marco Zehe (:MarcoZ)
	Comment 8 • 9 years ago

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.

	alexander :surkov (:asurkov)
	Comment 9 • 9 years ago

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

	Frédéric Wang (:fredw) Assignee
	Comment 10 • 9 years ago

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

	Pulsebot
	Comment 11 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 9 years ago

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