Bug 121990 - AX: Regression: media controls are no longer accessible
Summary: AX: Regression: media controls are no longer accessible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 120895
Blocks: 123749
  Show dependency treegraph
 
Reported: 2013-09-26 15:57 PDT by Jer Noble
Modified: 2013-11-04 18:24 PST (History)
12 users (show)

See Also:


Attachments
partail patch (still needs loc) (7.83 KB, patch)
2013-10-31 01:36 PDT, James Craig
no flags Details | Formatted Diff | Diff
partial patch (need to discuss loc approach) (15.36 KB, patch)
2013-10-31 19:11 PDT, James Craig
no flags Details | Formatted Diff | Diff
patch for review/commit (16.67 KB, patch)
2013-11-01 10:25 PDT, James Craig
darin: review+
jcraig: commit-queue-
Details | Formatted Diff | Diff
patch with review feedback (16.92 KB, patch)
2013-11-01 13:20 PDT, James Craig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.