Page MenuHomePhabricator

Videos do not play and do not appear correctly
Closed, ResolvedPublic1 Story Points

Description

Steps to reproduce

  1. Go to the Barack Obama article
  2. Go to the economic policy section
  3. Play the weekly address video

Expected results

Video playback starts

Actual results

Video does not play and is offcenter

Environments observed

App version: 983829650
Android OS versions: API 25
Device model: Pixel XL
Device language: English

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 352189 had a related patch set uploaded (by BearND; owner: BearND):
[mediawiki/services/mobileapps@master] Wrap video elements in anchor elements

https://gerrit.wikimedia.org/r/352189

bearND claimed this task.May 5 2017, 8:48 PM
bearND triaged this task as High priority.

Change 352686 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/extensions/MobileApp@master] Center <video> elements in Parsoid HTML

https://gerrit.wikimedia.org/r/352686

I'm not exactly opposed to this patch (since it'll be behind the "legacy" flag) but I'm feeling kind of on the fence about it, and here's why:

  • It adds a new hack for legacy clients when in general our focus is on removing client-specific hacks.
  • This isn't a recent regression; videos have never played correctly in MCS up until now.
  • It seems better to encourage users to update the app rather than add new service-side legacy fixes.

Maybe we should go straight for an app-side fix and put it in the next release.

bearND added a comment.May 8 2017, 9:27 PM

Hmm, I thought video playback did work in the app via MCS as well before Parsoid switched to 1.4. The MCS patch is just to make it work for older versions of the app before the app can be updated to work directly with the <video> element.

I just tried loading from local MCS with the Parsoid spec version reverted to 1.3 and got the same result.

The other thing, which I forgot to mention before, is that I opened up the current iOS app to see what it did here, and isn't able to play this video either. It might be out of scope here, though since they're still loading mobileview HTML and must have a completely separate bug. Still, I'd imagine there will be some consolidation around video loading at some point not too far away.

Change 352726 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[apps/android/wikipedia@master] Center <video> elements in Parsoid HTML

https://gerrit.wikimedia.org/r/352726

Change 352686 merged by Niedzielski:
[mediawiki/extensions/MobileApp@master] Center <video> elements in Parsoid HTML

https://gerrit.wikimedia.org/r/352686

Change 352726 merged by jenkins-bot:
[apps/android/wikipedia@master] Center <video> elements in Parsoid HTML

https://gerrit.wikimedia.org/r/352726

Change 352189 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Wrap video elements in anchor elements

https://gerrit.wikimedia.org/r/352189

@Mholloway, @bearND, thanks for fixing this! I had taken a note to make a ticket against the page library for this change specifically but it looks like Michael has updated the app and Parsoid's CSS (although I think we might need to re-sync with Parsoid). I believe the work remaining for page library is simply to pull in fixVideoAnchor() from Bernd's patch so that the app can use the latest Content Service revision. Is this correct? :]

My patch was just to fix the centering piece. I think you're right that the updated transform should be pulled in to pagelib—what we want is for it to appear correctly on the client when (legacy === false) in the service.

@Niedzielski I think the app should change some Java and JS code to handle <video> tags in addition to <a> tags (to launch the Gallery feature) . Then the fixVideoAnchor() from my patch is not needed anymore.

@bearND, @Mholloway, thank you for the detail. I've filed T165071 to capture this work. Since the JavaScript just reports the link clicked, I believe we could make the fix entirely on the JS side. If I'm mistaken, please update the ticket.

Change 357749 had a related patch set uploaded (by BearND; owner: BearND):
[mediawiki/services/mobileapps@master] Limit video transform to videos

https://gerrit.wikimedia.org/r/357749

Change 357750 had a related patch set uploaded (by BearND; owner: BearND):
[mediawiki/services/mobileapps@master] Limit video transform to videos

https://gerrit.wikimedia.org/r/357750

Change 357749 abandoned by BearND:
Limit video transform to videos

Reason:
lost Change-Id for next PS

https://gerrit.wikimedia.org/r/357749

Change 357750 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Limit video transform to videos

https://gerrit.wikimedia.org/r/357750

bearND closed this task as Resolved.Jun 22 2017, 5:48 PM