| • Niedzielski | |
| May 4 2017, 6:32 PM |
| F7913741: screenshot-2017-05-04-12-31-10-811637865.png | |
| May 4 2017, 6:32 PM |
| F7913746: screenshot-2017-05-04-12-30-31-133133359.png | |
| May 4 2017, 6:32 PM |
Video playback starts
Video does not play and is offcenter
App version: 983829650
Android OS versions: API 25
Device model: Pixel XL
Device language: English
Change 352189 had a related patch set uploaded (by BearND; owner: BearND):
[mediawiki/services/mobileapps@master] Wrap video elements in anchor elements
Change 352686 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/extensions/MobileApp@master] Center <video> elements in Parsoid HTML
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:
Maybe we should go straight for an app-side fix and put it in the next release.
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
Change 352686 merged by Niedzielski:
[mediawiki/extensions/MobileApp@master] Center <video> elements in Parsoid HTML
Change 352726 merged by jenkins-bot:
[apps/android/wikipedia@master] Center <video> elements in Parsoid HTML
Change 352189 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Wrap video elements in anchor elements
@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
Change 357750 had a related patch set uploaded (by BearND; owner: BearND):
[mediawiki/services/mobileapps@master] Limit video transform to videos
Change 357749 abandoned by BearND:
Limit video transform to videos
Reason:
lost Change-Id for next PS
Change 357750 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Limit video transform to videos