Bug 121990

Summary: AX: Regression: media controls are no longer accessible
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, eric.carlson, glenn, jcraig, jdiggs, mario, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 120895    
Bug Blocks: 123749    
Attachments:
Description Flags
partail patch (still needs loc)
none
partial patch (need to discuss loc approach)
none
patch for review/commit
darin: review+, jcraig: commit-queue-
patch with review feedback none

Description Jer Noble 2013-09-26 15:57:01 PDT
Media controls are currently explicitly localized by AccessibilityMediaControls.  Now that they are standard DOM objects, use ARIA roles to make the controls accessible.
Comment 1 Radar WebKit Bug Importer 2013-10-30 02:44:03 PDT
<rdar://problem/15349899>
Comment 2 James Craig 2013-10-31 01:36:51 PDT
Created attachment 215629 [details]
partail patch (still needs loc)
Comment 3 Jer Noble 2013-10-31 10:29:54 PDT
Looks good so far.
Comment 4 James Craig 2013-10-31 13:28:39 PDT
Related to bug 120956 (Localization)
Comment 5 James Craig 2013-10-31 19:11:45 PDT
Created attachment 215712 [details]
partial patch (need to discuss loc approach)
Comment 6 James Craig 2013-10-31 19:17:20 PDT
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?
Comment 7 Jer Noble 2013-11-01 08:32:03 PDT
(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.
Comment 8 James Craig 2013-11-01 10:25:21 PDT
Created attachment 215736 [details]
patch for review/commit
Comment 9 Darin Adler 2013-11-01 11:32:39 PDT
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”.
Comment 10 James Craig 2013-11-01 12:38:04 PDT
(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.
Comment 11 James Craig 2013-11-01 13:20:12 PDT
Created attachment 215752 [details]
patch with review feedback
Comment 12 Jer Noble 2013-11-01 13:54:34 PDT
Comment on attachment 215752 [details]
patch with review feedback

r=me.
Comment 13 WebKit Commit Bot 2013-11-01 14:22:05 PDT
Comment on attachment 215752 [details]
patch with review feedback

Clearing flags on attachment: 215752

Committed r158455: <http://trac.webkit.org/changeset/158455>
Comment 14 WebKit Commit Bot 2013-11-01 14:22:07 PDT
All reviewed patches have been landed.  Closing bug.