WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#24183 closed defect (bug) (fixed)

MediaElement.js i18n issues

Reported by: SergeyBiryukov Owned by: nacin
Milestone: 3.6 Priority: high
Severity: normal Version: 3.6
Component: I18N Keywords: has-patch commit
Focuses: Cc:

Description

As reported by dimadin in #23282:

MedaElement.js has very poor i18n support that isn't compatible with one we use in WordPress since it relies on browser's language to determine locale, and strings are hardcoded in it (see pull that introduced this).

I looked how to improve this and have it backward compatible and attached patch contains my proposal. Note that this shouldn't go straight to WordPress since it contains changes to MedaElement.js too. I wanted to hear thoughts from others before submitting pull to MedaElement.js. When they add compatible support, we can patch WordPress.

Attachments (11)

24183.patch (4.4 KB) - added by SergeyBiryukov 2 years ago.
Milan's patch from #23282
24183.me.patch (3.2 KB) - added by SergeyBiryukov 2 years ago.
24183.2.patch (1.7 KB) - added by SergeyBiryukov 2 years ago.
flashmediaelement.swf (28.0 KB) - added by ocean90 2 years ago.
Flash file of 2.12.0
24183.me.2.12.0.patch (146.5 KB) - added by ocean90 2 years ago.
CSS + JS of 2.12.0
24183.3.patch (1.5 KB) - added by ocean90 2 years ago.
Refreshed and added missing strings
24183.diff (1.5 KB) - added by nacin 2 years ago.
24183.4.patch (1.5 KB) - added by SergeyBiryukov 2 years ago.
24183.me.2.patch (1.6 KB) - added by SergeyBiryukov 2 years ago.
24183.4.alt.patch (141.5 KB) - added by SergeyBiryukov 2 years ago.
24183.2.diff (4.8 KB) - added by nacin 2 years ago.

Download all attachments as: .zip

Change History (41)

@SergeyBiryukov2 years ago

Milan's patch from #23282

comment:1 @SergeyBiryukov2 years ago

While we're at it, could we get http://mediaelementjs.com/ to spell WordPress correctly? :)

comment:2 @wonderboymusic2 years ago

Can you make a patch for MEjs upstream so we can report here: https://github.com/johndyer/mediaelement/issues/new ?

@SergeyBiryukov2 years ago

@SergeyBiryukov2 years ago

comment:3 @SergeyBiryukov2 years ago

24183.me.patch is a patch for MediaElement.js master branch.

24183.2.patch is a refreshed patch for script-loader.php.

@ocean902 years ago

Flash file of 2.12.0

@ocean902 years ago

CSS + JS of 2.12.0

comment:6 @ocean902 years ago

MediaElement.js 2.12.0 was released.

comment:7 @ocean902 years ago

In 24410:

Update MediaElement.js to 2.12.0. see #24183.

@ocean902 years ago

Refreshed and added missing strings

comment:8 @ocean902 years ago

  • Keywords needs-patch added; has-patch removed

Through a change in ME, see https://github.com/johndyer/mediaelement/commit/68ce179cbd, the patch doesn't work.

@nacin2 years ago

comment:9 follow-up: @nacin2 years ago

Seems like we could just nest it one level further, maybe?

I do wonder, if mediaelement is already enqueued, how much 24183.diff could break the whole mejs object.

comment:10 in reply to: ↑ 9 @ocean902 years ago

Replying to nacin:

Seems like we could just nest it one level further, maybe?

No, ME.js just overrides it. See https://github.com/johndyer/mediaelement/blob/master/build/mediaelement-and-player.js#L1676

@SergeyBiryukov2 years ago

@SergeyBiryukov2 years ago

comment:11 @SergeyBiryukov2 years ago

So, the problem is that when MEjs tries to use mejs.i18n.locale.strings, the mejs.i18n object is already overwritten above with the one that doesn't contain the strings we're trying to add. It also doesn't detect the language we're trying to pass.

24183.me.2.patch is a patch for upstream that makes the most sense to me. It consults a separate mejsL10n object, which follows the pattern we have in script-loader.php.

24183.4.patch complements it on our side.

comment:12 follow-up: @azaozz2 years ago

If we have to use it as-is, looks like will have to split mediaelement-and-player.js in two files. It has two parts: MediaElement.js at the top, MediaElementPlayer at the bottom. The translation needs to be between them to initialize properly.

Last edited 2 years ago by azaozz (previous) (diff)

comment:13 @nacin2 years ago

With mediaelement-and-player.js being an official upstream build, I think we have a good chance of getting a pull request accepted.

(At worst, let's plan on monkey-patching until a PR is accepted.)

comment:15 in reply to: ↑ 12 @SergeyBiryukov2 years ago

  • Keywords has-patch added; needs-patch removed

Replying to azaozz:

If we have to use it as-is, looks like will have to split mediaelement-and-player.js in two files.

24183.4.alt.patch implements that, just in case. Both mediaelement.min.js and mediaelementplayer.min.js are also official upstream builds.

comment:16 @SergeyBiryukov2 years ago

Pull request was accepted. Once 2.12.1 is out, 24183.4.patch should be good to go.

comment:17 @nacin2 years ago

  • Keywords commit added

Let's just sit tight for upstream then. By the end of the week, if there is no 2.12.1, we should merge in the patch ourselves.

@nacin2 years ago

comment:18 @nacin2 years ago

In 24755:

Update MediaElement.js to include i18n fixes that were accepted upstream. Add strings to script-loader.

props SergeyBiryukov.
see #24183.

comment:19 @nacin2 years ago

Leaving this open while we wait for 2.12.1.

comment:20 @nacin2 years ago

In 24786:

Update MediaElement.js to a new 2.12.1 (untagged) build. see #24183.

comment:21 @markjaquith2 years ago

Is there anything outstanding to do on this ticket?

comment:22 @nacin2 years ago

  • Resolution set to fixed
  • Status changed from new to closed

comment:23 @ocean902 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

It seems like [24786] breaks some things. See http://wordpress.org/support/topic/audio-player-controls-disappear-in-rc2.

Currently I only can reproduce it in Opera with a MP3 and MP4 file. Reverting to 24785 shows the controls again.

Last edited 2 years ago by ocean90 (previous) (diff)

comment:24 @ocean902 years ago

Replacing flashmediaelement.swf with the one from https://github.com/johndyer/mediaelement/commit/c52d79f0853567190d5a1607d82af0da6eacd66e restores the controls for me.

comment:25 @nacin2 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from reopened to closed

In 24861:

Update MediaElement.js SWF file from upstream. Fixes issues with controls. fixes #24183.

comment:26 @nacin2 years ago

In 24862:

Update MediaElement.js SWF file from upstream. Fixes issues with controls. fixes #24183.

Merges [24861] to the 3.6 branch.

comment:27 @nacin2 years ago

In 24910:

New build of MediaElement.js SWF. see #24183.

comment:28 @nacin2 years ago

In 24911:

New build of MediaElement.js SWF. see #24183. For the 3.6 branch.

comment:29 @nacin2 years ago

In 24946:

Update MediaElement.js to 2.13.0 build, which fixes issues WordPress reported. see #24183.

comment:30 @nacin2 years ago

In 24947:

Update MediaElement.js to 2.13.0 build, which fixes issues WordPress reported. see #24183.

Merges [24946] to the 3.6 branch.

Note: See TracTickets for help on using tickets.