670083 - expose placeholder as description if wasn't used as name

Status: RESOLVED WONTFIX

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

spun off bug 545817

Comment 2

[Marco Zehe (:MarcoZ)]

This was again brought up today by Paul J Adam from Deque Systems in this example:
http://pauljadam.com/demos/mobileforma11y.html

It seems to be a reoccurring pattern that both a label and a placeholder are being used to hint at expected formats (see the e-mail and phone number examples in the above form). So we might consider actually getting moving on this, or alternatively, make a final decision not to do it at all?

Thoughts?

Comment 3

[Marco Zehe (:MarcoZ)]

Attachment: Patch

1. If placeholder is present in addition to the label and is not identical, expose as accDescription.
2. If title is also present, and it differs from both label and placeholder, append it separated by a space.

Comment 4

[Marco Zehe (:MarcoZ)]

Attachment: Simpler patch

Got rid of the concatenation as discussed on IRC. This patch now behaves similarly to the instance for the name, e. g. only if no other description mechanism is successful, and placeholder is not alreay used as the label, it will be used as the description. This satisfies both the HTML5 Accessibility and Paul's examples.

Comment 5

[Marco Zehe (:MarcoZ)]

Attachment: Refactor to NativeDescription, untested

This is the approach I'm taking. I haven't tested if this compiles, just wanted to get your feedback on the general approach. Will continue to work on this tomorrow, so if you have feedback until then, I can incorporate that.

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 8434247 [details] [diff] [review]
Refactor to NativeDescription, untested

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

::: accessible/src/html/HTMLFormControlAccessible.cpp
@@ +351,5 @@
> +  if (!aDescription.IsEmpty()) {
> +    // Fetch the name and compare against description
> +    nsAutoString name;
> +    ENameValueFlag nameFlag = Name(name);
> +    if (aDescription.Equals(name))

it's correct to check nameFlag rather value. I agree it doesn't really make sense to expose description in case 

<input aria-label="hello" title="hello">

but that's what we currently do

anyway nameFlag is unused variable

::: accessible/src/html/HTMLSelectAccessible.cpp
@@ -453,5 @@
> -  // First check to see if combo box itself has a description, perhaps through
> -  // tooltip (title attribute) or via aria-describedby
> -  Accessible::Description(aDescription);
> -  if (!aDescription.IsEmpty())
> -    return;

you change the logic here (option description is preferred over @title), it's not evident there's something wrong with existing logic but either way it's separate issue. So I would not change all these overridden Description() methods.

Comment 7

[Marco Zehe (:MarcoZ)]

Attachment: Patch

1. Removed those instances of method renames/reworks that change logic, and only kept those who were simple renames/code moves into the new NativeDescription methods.
2. Moved the logic to check for name/description duplication into the generic Description method. The one for HTMLTextFieldAccessible is now very tiny.

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 8434965 [details] [diff] [review]
Patch

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

::: accessible/src/generic/Accessible.cpp
@@ +276,5 @@
>      GetTextEquivFromIDRefs(this, nsGkAtoms::aria_describedby,
>                             aDescription);
>  
>    if (aDescription.IsEmpty()) {
> +    NativeDescription(aDescription);

probably nicer would be to have if (!aDescription.IsEmpty()) and return early

@@ +304,5 @@
> +      // Don't use tooltip or placeholder for a description if it was used for a name.
> +      // Also, if obtained description duplicates the name, discard it.
> +      if (nameFlag == eNameFromTooltip ||
> +          nameFlag == eNameFromPlaceholder ||
> +          aDescription.Equals(name))

if you're going to compare strings then you don't really need to check nameFlag, you just make sure that description never equals name. If it sounds reasonable then it's the way to go I'd say.

::: accessible/src/html/HTMLFormControlAccessible.h
@@ +142,5 @@
>  
>  protected:
>    // Accessible
>    virtual ENameValueFlag NativeName(nsString& aName) MOZ_OVERRIDE;
> +  virtual void NativeDescription(nsString& aDescription);

use MOZ_OVERRIDE

::: accessible/src/xul/XULMenuAccessible.cpp
@@ +140,5 @@
>    return eNameOK;
>  }
>  
>  void
> +XULMenuitemAccessible::NativeDescription(nsString& aDescription)

all these unrelated changes, we may benefit from doing that but I'd say it should be covered by mochitest then

Comment 9

[Marco Zehe (:MarcoZ)]

(In reply to alexander :surkov from comment #8)
> Comment on attachment 8434965 [details] [diff] [review]
> Patch
> 
> Review of attachment 8434965 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/generic/Accessible.cpp
> @@ +276,5 @@
> >      GetTextEquivFromIDRefs(this, nsGkAtoms::aria_describedby,
> >                             aDescription);
> >  
> >    if (aDescription.IsEmpty()) {
> > +    NativeDescription(aDescription);
> 
> probably nicer would be to have if (!aDescription.IsEmpty()) and return early

That would lose the call to aDescription.CompressWhitespace() at the end of the method, which means I'd have to duplicate it.

> @@ +304,5 @@
> > +      // Don't use tooltip or placeholder for a description if it was used for a name.
> > +      // Also, if obtained description duplicates the name, discard it.
> > +      if (nameFlag == eNameFromTooltip ||
> > +          nameFlag == eNameFromPlaceholder ||
> > +          aDescription.Equals(name))
> 
> if you're going to compare strings then you don't really need to check
> nameFlag, you just make sure that description never equals name. If it
> sounds reasonable then it's the way to go I'd say.

Yeah I'm not sure about the string comparison any more. I think the safer way for this patch would be to just do the additional compare to the new flag, and not do a string comparison. Web authors might expect aria-describedby still be duplicated in the future, and our previous behavior didn't prevent that. So I think I'll remove the string comparison instead and just make sure we don't duplicate the placeholder in addition to what we already have.

> ::: accessible/src/html/HTMLFormControlAccessible.h
> @@ +142,5 @@
> >  
> >  protected:
> >    // Accessible
> >    virtual ENameValueFlag NativeName(nsString& aName) MOZ_OVERRIDE;
> > +  virtual void NativeDescription(nsString& aDescription);
> 
> use MOZ_OVERRIDE

Oh? Hadn't seen that in other places. What does this do?

> ::: accessible/src/xul/XULMenuAccessible.cpp
> @@ +140,5 @@
> >    return eNameOK;
> >  }
> >  
> >  void
> > +XULMenuitemAccessible::NativeDescription(nsString& aDescription)
> 
> all these unrelated changes, we may benefit from doing that but I'd say it
> should be covered by mochitest then

I think I'll just remove these for now. We can deal with those in a separate bug.

Comment 10

[Marco Zehe (:MarcoZ)]

Attachment: Patch, reuced to the absolute necessary

OK, stripped this patch down to the bare minimum and made these changes:

1. Placeholder is being obtained in NativeDescription, and compared to whatever the label is for the HTMLTextFieldAccessible.
2. If it is identical, the description is truncated.
3. In Accessible::Description, only the XUL part is transferred to a NativeDescription method (like in previous version of patch), and I again got rid of the eNameFromPlaceholder flag, since all placeholder processing is now done within the HTMLTextField::NAtiveDescription method.
4. Stripped out all unrelated renames.
5. Added some more description tests to where placeholder is tested, to make sure label/description duplication is caught.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #9)

> > if you're going to compare strings then you don't really need to check
> > nameFlag, you just make sure that description never equals name. If it
> > sounds reasonable then it's the way to go I'd say.
> 
> Yeah I'm not sure about the string comparison any more. I think the safer
> way for this patch would be to just do the additional compare to the new
> flag, and not do a string comparison. Web authors might expect
> aria-describedby still be duplicated in the future, and our previous
> behavior didn't prevent that. So I think I'll remove the string comparison
> instead and just make sure we don't duplicate the placeholder in addition to
> what we already have.

fine with me. however if having description never equal name makes sense then things would be much simpler. it's separate issue granted

> > use MOZ_OVERRIDE
> 
> Oh? Hadn't seen that in other places. What does this do?

it will assert you if this virtual method does not override method from base class

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 8437675 [details] [diff] [review]
Patch, reuced to the absolute necessary

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

::: accessible/src/html/HTMLFormControlAccessible.cpp
@@ +352,5 @@
> +  // If yes, discard it.
> +  nsAutoString name;
> +  Name(name);
> +  if (aDescription.Equals(name))
> +    aDescription.Truncate();

you still compare strings, you said in comment #9 you don't plan to do that. Btw, you can call Name() twice for single description computation at least until you have IsEmpty() check.

Comment 13

[steve faulkner]

(In reply to alexander :surkov from comment #11)
> (In reply to Marco Zehe (:MarcoZ) from comment #9)
> 
> > > if you're going to compare strings then you don't really need to check
> > > nameFlag, you just make sure that description never equals name. If it
> > > sounds reasonable then it's the way to go I'd say.
> > 
> > Yeah I'm not sure about the string comparison any more. I think the safer
> > way for this patch would be to just do the additional compare to the new
> > flag, and not do a string comparison. Web authors might expect
> > aria-describedby still be duplicated in the future, and our previous
> > behavior didn't prevent that. So I think I'll remove the string comparison
> > instead and just make sure we don't duplicate the placeholder in addition to
> > what we already have.
> 
> fine with me. however if having description never equal name makes sense
> then things would be much simpler. it's separate issue granted
> 
> > > use MOZ_OVERRIDE
> > 
> > Oh? Hadn't seen that in other places. What does this do?
> 
> it will assert you if this virtual method does not override method from base
> class

if you are suggesting what I think you are it would be a backwards incompatible and incompatibel with the ARIA name calc algorithm. For example: when an img only has a title it is used for the name, when the img has an and alt and a title the alt is used for name and the title is used for description.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to steve faulkner from comment #13)
> if you are suggesting what I think you are it would be a backwards
> incompatible and incompatibel with the ARIA name calc algorithm. For
> example: when an img only has a title it is used for the name, when the img
> has an and alt and a title the alt is used for name and the title is used
> for description.

right, on the another hand we (including spec) should be use case driven, if what spec says doesn't make sense then it should be changed.

Comment 15

[steve faulkner]

(In reply to alexander :surkov from comment #14)
> (In reply to steve faulkner from comment #13)
> > if you are suggesting what I think you are it would be a backwards
> > incompatible and incompatibel with the ARIA name calc algorithm. For
> > example: when an img only has a title it is used for the name, when the img
> > has an and alt and a title the alt is used for name and the title is used
> > for description.
> 
> right, on the another hand we (including spec) should be use case driven, if
> what spec says doesn't make sense then it should be changed.

I think it makes sense to use the acc description in such cases.

Comment 16

[alexander :surkov (:asurkov)]

why?

Comment 17

[alexander :surkov (:asurkov)]

(for the record) I think I misread Steve's comment #13 but we agreed on irc that description should never equals as a string to name, for example, <img alt="hi" title="hi"> the image accessible has 'hi' name and has empty description.

Comment 18

[alexander :surkov (:asurkov)]

btw, I field bug against ARIA https://www.w3.org/Bugs/Public/show_bug.cgi?id=26085

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 8437675 [details] [diff] [review]
Patch, reuced to the absolute necessary

Marco, are you going to update the patch?

If we are going to take an approach that description never equals name then it makes sense to handle that separately in another bug

Comment 20

[Marco Zehe (:MarcoZ)]

If I update the patch and remove all checks of duplication, then <input type="text" placeholder="My placeholder" /> will result in name and description both being present and duplicated. And that is just ugly. :(

Comment 21

[alexander :surkov (:asurkov)]

nah, I didn't mean that, I meant this scenario
1) fix the bug description doesn't dupe name which may look like
// calculate descr
if (descr.Equals(name))
  descr.Truncate();
2) fix this bug

Comment 22

[Marco Zehe (:MarcoZ)]

Decided to not work on this myself, but offer it as a good first bug and mentor it. I also followed dependent bug 1031184 for the general introduction of NativeDescription, and bug 1031188 for the work on name/description duplication.

Comment 23

[Dominic Mazzoni]

Here's the relevant part of the spec:

http://w3c.github.io/aria/html-aam/html-aam.html#accessible-name-and-description-calculation

Comment 24

[Takeshi Kurosawa :takenspc]

Attachment: Bug 670083 - expose placeholder as description if wasn't used as name

- Pass nameFlag to NativeDescription to prepare further implementation of HTML Accessibility API Mappings
- Use placeholder as description if it wasn't used as name

Review commit: https://reviewboard.mozilla.org/r/73802/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73802/

Comment 25

[alexander :surkov (:asurkov)]

Comment on attachment 8789979 [details]
Bug 670083 - expose placeholder as description if wasn't used as name

https://reviewboard.mozilla.org/r/73800/#review77054

::: accessible/generic/Accessible.cpp:195
(Diff revision 1)
>      GetTextEquivFromIDRefs(this, nsGkAtoms::aria_describedby,
>                             aDescription);
>  
> -  if (aDescription.IsEmpty()) {
> -    NativeDescription(aDescription);
> +  nsAutoString name;
> +  bool didNameInitialized = false;
>  

this is not trunk-based, right? can I see a complete diff?

Comment 26

[Takeshi Kurosawa :takenspc]

Comment on attachment 8789979 [details]
Bug 670083 - expose placeholder as description if wasn't used as name

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73802/diff/1-2/

Comment 27

[alexander :surkov (:asurkov)]

Comment on attachment 8789979 [details]
Bug 670083 - expose placeholder as description if wasn't used as name

https://reviewboard.mozilla.org/r/73802/#review77420

::: accessible/html/HTMLFormControlAccessible.cpp:353
(Diff revision 2)
> +
> +  // https://w3c.github.io/aria/html-aam/html-aam.html#input-type-text-input-type-password-input-type-search-input-type-tel-input-type-url-and-textarea-element
> +  // Use description from placeholder.
> +  if (nameFlag != eNameFromHTMLPlaceholder) {
> +    mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::placeholder, aDescription);
> +  }

not sure whether it's worth to have this flag, just to use it in this case only, especially if anyway we end up comparting a non empty description string with name. Would it be simpler to calculate native description for HTML input, and then compare strings in Accessible::Description?

Comment 28

[alexander :surkov (:asurkov)]

According to the latest discussion [1], IA2 participants tend to think HTML placeholder shouldn't be mapped into accessible description. The reasoning is placeholder is semantically different from both accessible name and description, and needs a special way to be exposed. 

Having said that, sometimes it may be used instead of a label in some UIs (see for example a search field at https://developer.mozilla.org/en-US/). In latter case, it makes sense to expose it as accessible name as a fallback, but otherwise it will be preferable if AT dealt with it as with placeholder.

Takeshi, if you are ok with it, then I suggest to close this bug. Otherwise please consider to join to IA2 list to object this.

[1] https://lists.linuxfoundation.org/pipermail/accessibility-ia2/2016-September/002165.html

Comment 29

[alexander :surkov (:asurkov)]

marking wontfix per comment #28, please reopen if you don't agree.

I filed a new bug 1303429 for placeholder object attribute.