Closed Bug 1187740 Opened 4 years ago Closed 4 years ago

Proxy handling for mozHTMLAccessible

Categories

(Core :: Disability Access APIs, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: lsocks, Assigned: lsocks)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Also removed some trailing whitespace.

GetLevelInternal function is modelled off the Accessible one.
Attachment #8639101 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8639101 [details] [diff] [review]
handle proxies in mozHTMLAccessible

>-    if (parent && parent->Role() == roles::TREE_TABLE) 
>+    if (parent && parent->Role() == roles::TREE_TABLE)

It'd be nice if you kept whitespace changes separate.

> bool
>+DocAccessibleChild::RecvIsHyperText(const uint64_t& aID, bool* aRetVal)

I'd rather not have this, and as far as I can tell you don't use it for anything.

> 
>+bool
>+ProxyAccessible::IsHyperText() const
>+{

same

> int32_t
>+ProxyAccessible::GetLevelInternal()
>+{

how are the over rides of GetLevleInternal supposed to work?  it seems to me it would be simplest to just add a message to get the level.

> - (id)value
> {
>+  uint32_t level = 0;
>   AccessibleWrap* accWrap = [self getGeckoAccessible];
>+  if (accWrap && accWrap->IsHyperText())
>+    level = accWrap->AsHyperText()->GetLevelInternal();

the AsHyperText() is useless, and I'm not sure what the point of the IsHyperText() call is either.

>-- (void)accessibilityPerformAction:(NSString*)action 
>+- (void)accessibilityPerformAction:(NSString*)action
> {
>-  AccessibleWrap* accWrap = [self getGeckoAccessible];
>+  if ([action isEqualToString:NSAccessibilityPressAction]) {
>+    if (AccessibleWrap* accWrap = [self getGeckoAccessible])
>+      accWrap->DoAction(0);
>+    else if (ProxyAccessible* proxy = [self getProxyAccessible])
>+      proxy->DoAction(0);
>+    else
>+      return;

seems like it might be simpler to make that unconditional, then you wouldn't need either else.

> - (NSURL*)url
> {
>-  if (![self getGeckoAccessible] || [self getGeckoAccessible]->IsDefunct())
>-    return nil;
>+  nsAutoString value;
>+  if (AccessibleWrap* accWrap = [self getGeckoAccessible]) {
>+    if (accWrap->IsDefunct())
>+      return nil;

it should never be defunct so that seems kind of silly.

> 
>-  nsAutoString value;
>-  [self getGeckoAccessible]->Value(value);
>+    accWrap->Value(value);
>+  } else if (ProxyAccessible* proxy = [self getProxyAccessible]) {
>+    if (proxy->State() & states::DEFUNCT)
>+      return nil;

same just drop it
Attachment #8639101 - Flags: review?(tbsaunde+mozbugs)
Attachment #8639101 - Attachment is obsolete: true
Attachment #8643931 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8643931 [details] [diff] [review]
handle proxies in mozHTMLAccessible with fixes

the random whitespace changes are kind of annoying, please put those in their own patch.
Attachment #8643931 - Flags: review?(tbsaunde+mozbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/b4b5caf0208e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.