Closed Bug 799909 Opened 7 years ago Closed 7 years ago

decomify Accessible::GetNameInternal

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access)

Attachments

(1 file)

Attached patch patchSplinter Review
No description provided.
Attachment #669925 - Flags: review?(trev.saunders)
Comment on attachment 669925 [details] [diff] [review]
patch

>-  nsresult rv = GetNameInternal(aName);
>+  ENameValueFlag nameFlag = NativeName(aName);
>   if (!aName.IsEmpty())
>     return eNameOK;
> 
>   // In the end get the name from tooltip.
>   if (mContent->IsHTML()) {
>     if (mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::title, aName)) {
>       aName.CompressWhitespace();
>       return eNameFromTooltip;
>@@ -287,17 +287,17 @@ Accessible::Name(nsString& aName)
>     if (mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::tooltiptext, aName)) {
>       aName.CompressWhitespace();
>       return eNameFromTooltip;
>     }
>   } else {
>     return eNameOK;
>   }
> 
>-  if (rv != NS_OK_EMPTY_NAME)
>+  if (nameFlag != eNoNameOnPurpose)
>     aName.SetIsVoid(true);

hm, is there a reason we cann't move that to before the tooltip stuff and return early?  It seems like it would be a bit less confusing / strange

>   Accessible* labelAcc = nullptr;
>   HTMLLabelIterator iter(Document(), this);
>   while ((labelAcc = iter.Next())) {
>     nsresult rv = nsTextEquivUtils::
>-      AppendTextEquivFromContent(this, labelAcc->GetContent(), &label);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    label.CompressWhitespace();
>+      AppendTextEquivFromContent(this, labelAcc->GetContent(), &aLabel);
>+    //NS_ENSURE_SUCCESS(rv, rv);

why not just remove it and rv?

> 
>-  return nsTextEquivUtils::GetNameFromSubtree(this, aLabel);
>+  nsTextEquivUtils::GetNameFromSubtree(this, aName);

I'm a little worried that changing to ignoring the return of all these nsTextEquivUtils functions will change behavior, but I don't see a terribly clear way it actually does.  It seems like a safer way to do this might be to make the nsTextEquivUtils stuff infallable / return the nameFlag first, and then change NativeName().

>+ /**
>+  * Name was left empty by the author on purpose:
>+  * name.IsEmpty() && !name.IsVoid().
>+  */
>+ eNoNameOnPurpose,

its kind of anoying we have this and .Isvoid() instead of just .IsEmpty() meaning the same thing is there a reason for it?

> protected:
>+  // Accessible
>+  virtual mozilla::a11y::ENameValueFlag NativeName(nsString& aName);

MOZ_OVERRIDE here and elsewhere?
Attachment #669925 - Flags: review?(trev.saunders) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #1)

> >-  if (rv != NS_OK_EMPTY_NAME)
> >+  if (nameFlag != eNoNameOnPurpose)
> >     aName.SetIsVoid(true);
> 
> hm, is there a reason we cann't move that to before the tooltip stuff and
> return early?  It seems like it would be a bit less confusing / strange

maybe constant name is confusing. That noname works if no name at all was provided (including tooltip stuff).

> >-  return nsTextEquivUtils::GetNameFromSubtree(this, aLabel);
> >+  nsTextEquivUtils::GetNameFromSubtree(this, aName);
> 
> I'm a little worried that changing to ignoring the return of all these
> nsTextEquivUtils functions will change behavior, but I don't see a terribly
> clear way it actually does.  It seems like a safer way to do this might be
> to make the nsTextEquivUtils stuff infallable / return the nameFlag first,
> and then change NativeName().

we should be ok since:
1) if failure then something bad happened but it shouldn't mean we should stop name computation at all
2) noname_clause_handled is internal nsTextEquivUtils thing

> >+ /**
> >+  * Name was left empty by the author on purpose:
> >+  * name.IsEmpty() && !name.IsVoid().
> >+  */
> >+ eNoNameOnPurpose,
> 
> its kind of anoying we have this and .Isvoid() instead of just .IsEmpty()
> meaning the same thing is there a reason for it?

absolutely, but we use flags internally since they are more readable, .IsVoid/IsEmpty stuff for consumer.

> MOZ_OVERRIDE here and elsewhere?

ok
(In reply to alexander :surkov from comment #2)
> (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> 
> > >-  if (rv != NS_OK_EMPTY_NAME)
> > >+  if (nameFlag != eNoNameOnPurpose)
> > >     aName.SetIsVoid(true);
> > 
> > hm, is there a reason we cann't move that to before the tooltip stuff and
> > return early?  It seems like it would be a bit less confusing / strange
> 
> maybe constant name is confusing. That noname works if no name at all was
> provided (including tooltip stuff).

I think what's actually confusing there is that we do tuff that if successful returns before looking at the early return value.

> > >-  return nsTextEquivUtils::GetNameFromSubtree(this, aLabel);
> > >+  nsTextEquivUtils::GetNameFromSubtree(this, aName);
> > 
> > I'm a little worried that changing to ignoring the return of all these
> > nsTextEquivUtils functions will change behavior, but I don't see a terribly
> > clear way it actually does.  It seems like a safer way to do this might be
> > to make the nsTextEquivUtils stuff infallable / return the nameFlag first,
> > and then change NativeName().
> 
> we should be ok since:
> 1) if failure then something bad happened but it shouldn't mean we should
> stop name computation at all
> 2) noname_clause_handled is internal nsTextEquivUtils thing

true

> > >+ /**
> > >+  * Name was left empty by the author on purpose:
> > >+  * name.IsEmpty() && !name.IsVoid().
> > >+  */
> > >+ eNoNameOnPurpose,
> > 
> > its kind of anoying we have this and .Isvoid() instead of just .IsEmpty()
> > meaning the same thing is there a reason for it?
> 
> absolutely, but we use flags internally since they are more readable,
> .IsVoid/IsEmpty stuff for consumer.

ok, does it make sense to comment that somewhere?
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> (In reply to alexander :surkov from comment #2)

> I think what's actually confusing there is that we do tuff that if
> successful returns before looking at the early return value.

I agree there's a problem but I don't really have ideas how to improve it.

> > absolutely, but we use flags internally since they are more readable,
> > .IsVoid/IsEmpty stuff for consumer.
> 
> ok, does it make sense to comment that somewhere?

ok, I will add a comment to Accessible::Name()
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e070c062f0
Assignee: nobody → surkov.alexander
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/d9e070c062f0
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.