Bug 173858 - [Modern Media Controls] Accessibility labels should be formatted using NSDateComponentsFormatter
Summary: [Modern Media Controls] Accessibility labels should be formatted using NSDate...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-26 20:20 PDT by Ryosuke Niwa
Modified: 2017-06-28 09:10 PDT (History)
10 users (show)

See Also:


Attachments
Make media control use stringsdict (6.01 KB, patch)
2017-06-26 20:21 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (10.92 KB, patch)
2017-06-27 08:17 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (981.85 KB, application/zip)
2017-06-27 09:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.13 MB, application/zip)
2017-06-27 09:26 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.75 MB, application/zip)
2017-06-27 09:42 PDT, Build Bot
no flags Details
Patch (14.85 KB, patch)
2017-06-27 09:47 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (15.04 KB, patch)
2017-06-27 10:24 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (15.37 KB, patch)
2017-06-27 14:20 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2017-06-26 20:20:52 PDT
Media control accessibility text should use stringsdict
Comment 1 Ryosuke Niwa 2017-06-26 20:21:34 PDT
Created attachment 313889 [details]
Make media control use stringsdict
Comment 2 Ryosuke Niwa 2017-06-26 22:51:30 PDT
Comment on attachment 313889 [details]
Make media control use stringsdict

Actually, this doesn't work. The modern media controls doesn't use the localized strings at all :(
Comment 3 Ryosuke Niwa 2017-06-26 22:53:08 PDT
<rdar://problem/32643171>
Comment 4 Antoine Quint 2017-06-27 08:12:32 PDT
I think the right fix is to use NSDateComponentsFormatter, so retitling this bug to cover this.
Comment 5 Antoine Quint 2017-06-27 08:17:59 PDT
Created attachment 313913 [details]
Patch
Comment 6 Build Bot 2017-06-27 09:16:50 PDT
Comment on attachment 313913 [details]
Patch

Attachment 313913 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4006504

New failing tests:
media/modern-media-controls/scrubber/scrubber-has-correct-ax-label.html
Comment 7 Build Bot 2017-06-27 09:16:51 PDT
Created attachment 313917 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2017-06-27 09:26:35 PDT
Comment on attachment 313913 [details]
Patch

Attachment 313913 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4006514

New failing tests:
media/modern-media-controls/scrubber/scrubber-has-correct-ax-label.html
Comment 9 Build Bot 2017-06-27 09:26:36 PDT
Created attachment 313918 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 10 Build Bot 2017-06-27 09:42:54 PDT
Comment on attachment 313913 [details]
Patch

Attachment 313913 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4006538

New failing tests:
media/modern-media-controls/scrubber/scrubber-has-correct-ax-label.html
Comment 11 Build Bot 2017-06-27 09:42:56 PDT
Created attachment 313920 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Antoine Quint 2017-06-27 09:47:53 PDT
Created attachment 313921 [details]
Patch
Comment 13 Antoine Quint 2017-06-27 10:24:05 PDT
Created attachment 313924 [details]
Patch
Comment 14 Dean Jackson 2017-06-27 13:46:13 PDT
Comment on attachment 313924 [details]
Patch

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

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:292
> +String MediaControlsHost::formattedDuration(const double durationInSeconds) const

I'm not sure about this name. Maybe formattedStringForDuration? or just stringForDuration?

> Source/WebCore/rendering/RenderTheme.h:97
> +    virtual String mediaControlsFormattedDuration(const double) { return String(); }

Ditto here.

> Source/WebCore/rendering/RenderThemeMac.mm:309
> +String RenderThemeMac::mediaControlsFormattedDuration(const double durationInSeconds)

We don't have a RenderThemeCocoa?
Comment 15 Tim Horton 2017-06-27 14:00:23 PDT
(In reply to Dean Jackson from comment #14)
> Comment on attachment 313924 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313924&action=review
> 
> > Source/WebCore/rendering/RenderThemeMac.mm:309
> > +String RenderThemeMac::mediaControlsFormattedDuration(const double durationInSeconds)
> 
> We don't have a RenderThemeCocoa?

We do!
Comment 16 Tim Horton 2017-06-27 14:01:52 PDT
Comment on attachment 313924 [details]
Patch

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

>> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:292
>> +String MediaControlsHost::formattedDuration(const double durationInSeconds) const
> 
> I'm not sure about this name. Maybe formattedStringForDuration? or just stringForDuration?

userVisibleDuration :P
Comment 17 Antoine Quint 2017-06-27 14:20:14 PDT
Created attachment 313943 [details]
Patch for landing
Comment 18 WebKit Commit Bot 2017-06-27 16:48:26 PDT
Comment on attachment 313943 [details]
Patch for landing

Clearing flags on attachment: 313943

Committed r218853: <http://trac.webkit.org/changeset/218853>
Comment 19 WebKit Commit Bot 2017-06-27 16:48:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Darin Adler 2017-06-27 17:30:18 PDT
Comment on attachment 313943 [details]
Patch for landing

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

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:292
> +String MediaControlsHost::formattedStringForDuration(const double durationInSeconds) const

The const here is basically meaningless and not needed.

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.h:91
> +    String formattedStringForDuration(const double) const;

Ditto.
Comment 21 Antoine Quint 2017-06-28 09:10:03 PDT
(In reply to Darin Adler from comment #20)
> Comment on attachment 313943 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313943&action=review
> 
> > Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:292
> > +String MediaControlsHost::formattedStringForDuration(const double durationInSeconds) const
> 
> The const here is basically meaningless and not needed.
> 
> > Source/WebCore/Modules/mediacontrols/MediaControlsHost.h:91
> > +    String formattedStringForDuration(const double) const;
> 
> Ditto.

Raised https://bugs.webkit.org/show_bug.cgi?id=173925 for the followup.