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

Jer Noble
Reported 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.
Attachments
partail patch (still needs loc) (7.83 KB, patch)
2013-10-31 01:36 PDT, James Craig 		no flags
DetailsFormatted DiffDiff
partial patch (need to discuss loc approach) (15.36 KB, patch)
2013-10-31 19:11 PDT, James Craig 		no flags
DetailsFormatted DiffDiff
patch for review/commit (16.67 KB, patch)
2013-11-01 10:25 PDT, James Craig 		darin: review+
jcraig: commit-queue-
DetailsFormatted DiffDiff
patch with review feedback (16.92 KB, patch)
2013-11-01 13:20 PDT, James Craig 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2013-10-30 02:44:03 PDT
<rdar://problem/15349899>
James Craig
Comment 2 2013-10-31 01:36:51 PDT
Created attachment 215629 [details] partail patch (still needs loc)
Jer Noble
Comment 3 2013-10-31 10:29:54 PDT
Looks good so far.
James Craig
Comment 4 2013-10-31 13:28:39 PDT
Related to bug 120956 (Localization)
James Craig
Comment 5 2013-10-31 19:11:45 PDT
Created attachment 215712 [details] partial patch (need to discuss loc approach)
James Craig
Comment 6 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?
Jer Noble
Comment 7 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.
James Craig
Comment 8 2013-11-01 10:25:21 PDT
Created attachment 215736 [details] patch for review/commit
Darin Adler
Comment 9 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”.
James Craig
Comment 10 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.
James Craig
Comment 11 2013-11-01 13:20:12 PDT
Created attachment 215752 [details] patch with review feedback
Jer Noble
Comment 12 2013-11-01 13:54:34 PDT
Comment on attachment 215752 [details] patch with review feedback r=me.
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2013-11-01 14:22:07 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.