Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Title attribute value in closed-caption menu is broken #3699

Closed
stsvilik opened this issue Oct 21, 2016 · 18 comments · Fixed by #4019
Closed

Title attribute value in closed-caption menu is broken #3699

stsvilik opened this issue Oct 21, 2016 · 18 comments · Fixed by #4019

Comments

@stsvilik
Copy link

stsvilik commented Oct 21, 2016

Description

When CC menu items are hovered by mouse, they display empty or invalid text.

Steps to reproduce

Hover mouse over CC menu item to see title text appear.

In Chrome

  1. Put player on pause
  2. Inspect CC button and set :hover state to checked
  3. Inspect menu item which appears on-hover (caption settings is good example)
  4. Pay attention to "title" attribute of li tag

Results

Expected

Title text to match values in the items.

Actual

Empty or invalid text appears.

Error output

No.

This issue can be observed on Video.js homepage in "Funny or Die" demo.

versions

videojs

5.11.8?

browsers

All

OSes

All

@gkatsev
Copy link
Member

gkatsev commented Oct 21, 2016

Can you please elaborate on what you mean by invalid text?
I believe right now the title attribute for the captions menu items is set to only tell whether the item is selected or not.
I'm not sure it would make sense to have the title attribute be the text itself, seems a bit redundant. What do you think, @OwenEdwards?

@stsvilik
Copy link
Author

@gkatsev I would expect a title for say "english" track to say at least "english" instead of " " which looks like a small square blocking the content. As for "caption settings" item, title is ", opens captions settings dialog" do you find it correct? Where is comma coming from? To be honest I don't think title is necessary at all - who would ever wait long enough to see it show up?

@gkatsev
Copy link
Member

gkatsev commented Oct 21, 2016

Yeah, we should potentially just remove it altogether. I think the comma is there for screenreaders, but not positive.

@OwenEdwards
Copy link
Member

OwenEdwards commented Oct 21, 2016

This is the same issue that @chemoish mentioned in his comment on #3447.

When title attributes were added, they just pulled in the controlText value. This makes sense for controls which have an icon and no visible text since the controlText is both what is presented by a screen reader and what is shown on mouse-hover as a tooltip.

For controls which have visible text (e.g. MenuItems), to support screen reader users the controlText was set to additional information about the button which is required for accessibility. Unfortunately, this doesn't work with the way that the title text was implemented. It doesn't make sense to have any title attribute on a control with a text label - if the control isn't clear from the text itself, then it shouldn't rely on the title tooltip because that isn't shown for sighted keyboard-only users (EDIT: some frameworks implement a custom tooltip which doesn't rely on the title attribute and is shown to both keyboard-only and mouse users; AblePlayer does this too)..

So either the setting of the title text needs to be changed, or controlText needs to be more clearly defined and potentially the components need a separate feature for screen reader specific text.

What do you think, @gkatsev and @chemoish?

@hartman
Copy link
Contributor

hartman commented Oct 22, 2016

This just shows another case, of where the current 'controlText' structure breaks down in my opinion. Even if you look at the html, it doesn't make sense:

<li class="vjs-menu-item vjs-selected" tabindex="-1" role="menuitemcheckbox" aria-live="polite" aria-checked="true" title=", selected">
    captions off<span class="vjs-control-text">, selected</span>
</li>

I mean, what is marked as control-text is not the actual control's text.. And then that get's reused for a tooltip, even though this thing doesn't need a tooltip. It should probably be something like:

<li class="vjs-menu-item vjs-selected vjs-textual" tabindex="-1" role="menuitemcheckbox" aria-live="polite" aria-checked="true">
    <span class="vjs-control-text">captions off</span><span class="vjs-offscreen-text">, selected</span>
</li>

@OwenEdwards
Copy link
Member

@hartman sure, but there are two main types of controls; ones which have an icon, in which case the controlText is essentially the alt attribute for that icon and should be hidden, and controls which are labeled with text. For controls which are labeled with text, the vjs-control-text that you propose needs to be visible, and there's no reason to have a title attribute.

@hartman
Copy link
Contributor

hartman commented Oct 24, 2016

@OwenEdwards but that's just a matter of defining the right css. who says that vis-control-text always needs to be invisible, just because it is now ?

@OwenEdwards
Copy link
Member

@hartman then would the controlText always be used as the title (visible on hover), or would the CSS add the title attribute if the controlText isn't visible?

@dgirgenti
Copy link

This is still an issue for 5.13 in IE11

@OwenEdwards
Copy link
Member

Looking over this again, it seems like @hartman is starting from the assumption that controlText is the title of the control, whereas I was starting from the basis that it is text associated with the control that is not displayed visually. Look again at the example:

<li class="vjs-menu-item vjs-selected" tabindex="-1" role="menuitemcheckbox" aria-live="polite" aria-checked="true" title=", selected">
    captions off<span class="vjs-control-text">, selected</span>
</li>

The captions off isn't the controlText for this control. Any change to this would be a DOM change, so it would need to happen in V6 rather than V5.x, right @gkatsev ?

Fundamentally we need these separate pieces of information for every control:

  1. Title
  2. Icon (which is currently exposed to screen readers, but ought to be aria-hidden)
  3. Text, which may or may not be displayed, but will act as a textual alternative for the icon

The title may match the text, but it may be absent instead (I think it needs to be absent, rather than having title="", but I could be wrong about that). I foresee that there will be cases where people will want the title to be present but not match the hidden text. And this is where it gets tricky, because of the accessible name computation.

I was trying to add a fourth piece of information, which was "state or description," for screen reader users. This has clearly caused problems when the title attribute was added, so maybe it should also be a separate piece of information within the Component, and we can decide whether to add it as off-screen text or some other way. @chemoish, thoughts?

@gkatsev
Copy link
Member

gkatsev commented Jan 25, 2017

This sounds like something we should figure out and change for 6.0. Might be too tricky to fix for 5.x. I'll add this to the 6.0 project.

@chemoish
Copy link
Member

@OwenEdwards sorry apparently I missed your previous message.

Its been a long while since I have put much thought into this (could totally be off the mark with context—everything is off cuff), but I think the organization structure of components's label, title, controlText, and icon are currently structured in a very confusing way (leading to misusage issues).

  1. Title—should only be used for icon's? (I don't think title is a good alternative for a tooltip, but is fine for now?)
  2. Icon—icon's should manage themselves and not be constrained by a component's ::before
  3. Text—text or textual alternative—combining these makes things more confusing
  4. controlText is a very confusing name to represent visually hidden text

Instead,

  • I think icon's should be stand alone DOM elements that should control their own title and textual alternatives.
  • I think controlText should be replaced with visuallyHiddenText or something that more appropriately represents this concept
  • controlText can stay the same (or be labeled differently since its too similar to label), but should only control the text (for sure not the title)

I don't remember all of the reasons to and how label, title, controlText, and icon should be used. It might be a good exercise to lay out the use cases prior to making any changes.

I also don't know if you need to add , selected to MenuItem's or if aria-checked=true is enough. However, separating the controlText from the title will help with extraneously floating boxes.

What audience is title attribute used for?

Hopefully, a combination of all of these cases do not need to be handled…But instead define specific use cases where we can provide a less confusing interface.

@gkatsev
Copy link
Member

gkatsev commented Jan 25, 2017

  • controlText was created to provide description for screen readers when using font-icons
  • title shows up on hover for buttons/components. controlText of buttons like playtoggle works great for this.
  • menu items have a body but also extra hints to read inside of controlText.

Thinking about the above like this, it sounds like we just want to manage the title attribute of menu items manually in the menu item? Rather than forcing the title attribute to be the control text for those, we'd want to use the actual text of the menu item.
I guess we'd want to update to not force components to have the title attribute be the control text.

@chemoish
Copy link
Member

Sounds like we are on a similar page.

  • Definitely need a way to manage accessible text for both icon buttons and menu items
  • Titles are helpful for icon buttons (less useful for text only menu items)

My suggestion above might be a bit too bold, but I am down to help simplify this whenever it gets slotted in.

@OwenEdwards
Copy link
Member

I agree with @gkatsev and @chemoish. @hartman's suggestion that the hidden text was something that might be made visible by a CSS/skin change would make things much more complicated because then you'd want to remove the title attribute to prevent there being a duplicative tooltip.

So yes, in the short-term, I think just not adding a title for MenuItem components would fix this issue.

Then these two are separate issues:

  1. hiding the icon font from screen readers
  2. allowing menu items to include both an icon and a text label (I'm thinking of an example we discussed previously, where Captions and Subtitles are available on a single menu, but distinguished in that menu with the appropriate icon)

I'll open them as distinct issues.

@gkatsev
Copy link
Member

gkatsev commented Jan 25, 2017

Stopping using control-text for title on MenuItem is probably something that we can release as a patch or minor update as well.

@dgirgenti
Copy link

Can the icon font just be hidden from screen readers with aria-hidden="true"?

@OwenEdwards
Copy link
Member

@dgirgenti the question of hiding the icon font from screen readers has been moved to a separate issue, #3982

OwenEdwards added a commit to OwenEdwards/video.js that referenced this issue Feb 1, 2017
OwenEdwards added a commit to OwenEdwards/video.js that referenced this issue Feb 1, 2017
gkatsev pushed a commit that referenced this issue Feb 2, 2017
Prevents a title attribute from being applied to MenuItems.
MenuItem indicates to ClickableComponent that the control is not just an icon, so it shouldn't have a title attribute.

Fixes #3699
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants