Proxy handling for mozHTMLAccessible

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lsocks, Assigned: lsocks)

Tracking

unspecified
mozilla42
x86_64
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8639101 [details] [diff] [review]
handle proxies in mozHTMLAccessible

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)
(Assignee)

Comment 3

3 years ago
Created attachment 8643931 [details] [diff] [review]
handle proxies in mozHTMLAccessible with fixes
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
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.