Bug 121990 – AX: Regression: media controls are no longer accessible

• Component: Accessibility
• Status: RESOLVED FIXED

[Jer Noble]

Media controls are currently explicitly localized by AccessibilityMediaControls.  Now that they are standard DOM objects, use ARIA roles to make the controls accessible.

[Radar WebKit Bug Importer]

<rdar://problem/15349899>

[James Craig]

Created attachment 215629 [details]
partail patch (still needs loc)

[Jer Noble]

Looks good so far.

[James Craig]

Related to bug 120956 (Localization)

[James Craig]

Created attachment 215712 [details]
partial patch (need to discuss loc approach)

[James Craig]

This is ready to go but doesn't yet fully address Loc. I added an accessor method for error avoidance, and brought all the string values into one spot, but they are still hardcoded in English.

Jer, do you still want to do Loc as part of bug 120956? If so, I'll update my Changelogs and resubmit this for review and commit queue. Otherwise, we should discuss the Loc approach. Is there already precedent for pulling localized strings into JS files like this?

[Jer Noble]

(In reply to comment #6)
> This is ready to go but doesn't yet fully address Loc. I added an accessor method for error avoidance, and brought all the string values into one spot, but they are still hardcoded in English.
> 
> Jer, do you still want to do Loc as part of bug 120956?

Sure.

> If so, I'll update my Changelogs and resubmit this for review and commit queue. Otherwise, we should discuss the Loc approach. Is there already precedent for pulling localized strings into JS files like this?

There is; the inspector has it's own localization technique, and we can either borrow it from them or come up with a common approach.

[James Craig]

Created attachment 215736 [details]
patch for review/commit

[Darin Adler]

Comment on attachment 215736 [details]
patch for review/commit

View in context: https://bugs.webkit.org/attachment.cgi?id=215736&action=review

Looks great.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:80
> +    /* Localized strings */

We normally use // comments everywhere, not /* */ comments.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:81
> +    loc: function(s){

Is this a standard name we use for this function elsewhere? Could we use a word rather than a little abbreviation? I understand that brevity is nice, but I at least find words easier to read.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:85
> +            return s; // TODO: log something if string not localized.

We normally use FIXME for all things like this, never TODO.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:91
> +        'Display Fullscreen': 'Display Fullscreen',

We use two words for this in Apple user interface and marketing materials, “Full Screen”, not one word “Fullscreen”.

[James Craig]

(In reply to comment #9)

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:81
> > +    loc: function(s){
> 
> Is this a standard name we use for this function elsewhere? Could we use a word rather than a little abbreviation? I understand that brevity is nice, but I at least find words easier to read.

WebInspector uses .UIString('foo'). I'll use that for now, since Jer indicate he may borrow the localization pattern from there.

[James Craig]

Created attachment 215752 [details]
patch with review feedback

[Jer Noble]

Comment on attachment 215752 [details]
patch with review feedback

r=me.

[WebKit Commit Bot]

Comment on attachment 215752 [details]
patch with review feedback

Clearing flags on attachment: 215752

Committed r158455: <http://trac.webkit.org/changeset/158455>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.