expose placeholder as description if wasn't used as name

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
7 years ago
2 years ago

People

(Reporter: surkov, Assigned: taken.spc, Mentored)

Tracking

(Blocks: 3 bugs, {access})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug], URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

7 years ago
spun off bug 545817
(Reporter)

Updated

7 years ago
Blocks: 389237
(Reporter)

Updated

6 years ago
Duplicate of this bug: 824882

Comment 2

4 years ago
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

4 years ago
Created attachment 8434148 [details] [diff] [review]
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.
Assignee: nobody → marco.zehe
Status: NEW → ASSIGNED
Attachment #8434148 - Flags: review?(surkov.alexander)

Comment 4

4 years ago
Created attachment 8434209 [details] [diff] [review]
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.
Attachment #8434148 - Attachment is obsolete: true
Attachment #8434148 - Flags: review?(surkov.alexander)
Attachment #8434209 - Flags: review?(surkov.alexander)

Comment 5

4 years ago
Created attachment 8434247 [details] [diff] [review]
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.
Attachment #8434209 - Attachment is obsolete: true
Attachment #8434209 - Flags: review?(surkov.alexander)
Attachment #8434247 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 6

4 years ago
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

4 years ago
Created attachment 8434965 [details] [diff] [review]
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.
Attachment #8434247 - Attachment is obsolete: true
Attachment #8434247 - Flags: feedback?(surkov.alexander)
Attachment #8434965 - Flags: review?(surkov.alexander)
(Reporter)

Comment 8

4 years ago
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

4 years ago
(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.
Created attachment 8437675 [details] [diff] [review]
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.
Attachment #8434965 - Attachment is obsolete: true
Attachment #8434965 - Flags: review?(surkov.alexander)
Attachment #8437675 - Flags: review?(surkov.alexander)
(Reporter)

Comment 11

4 years ago
(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
(Reporter)

Comment 12

4 years ago
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

4 years ago
(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.
(Reporter)

Comment 14

4 years ago
(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

4 years ago
(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.
(Reporter)

Comment 16

4 years ago
why?
(Reporter)

Comment 17

4 years ago
(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.
(Reporter)

Comment 18

4 years ago
btw, I field bug against ARIA https://www.w3.org/Bugs/Public/show_bug.cgi?id=26085
(Reporter)

Comment 19

4 years ago
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
Attachment #8437675 - Flags: review?(surkov.alexander)
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. :(
(Reporter)

Comment 21

4 years ago
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

Updated

4 years ago
Depends on: 1031184

Updated

4 years ago
Depends on: 1031188
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.
Assignee: marco.zehe → nobody
Mentor: marco.zehe
Status: ASSIGNED → NEW
Whiteboard: [good first bug]

Updated

2 years ago
Blocks: 1273884
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → taken.spc
(Reporter)

Comment 25

2 years ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 27

2 years ago
mozreview-review
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?
(Reporter)

Comment 28

2 years ago
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
(Reporter)

Updated

2 years ago
Attachment #8789979 - Flags: review?(surkov.alexander)
(Reporter)

Comment 29

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

I filed a new bug 1303429 for placeholder object attribute.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.