Don't consider player gone when it only got temporarily added by us
ClosedPublic

Authored by broulik on Apr 9 2020, 4:17 PM.

Details

Summary

We temporarily add players created by JS to the DOM so we can access them from the content-script.
When the player is removed again - at least on Firefox - the MutationObserver notices that and we signal the player being gone.
This causes media controls for HTML5 Audio to be immediately rescinded when created but not played immediately.

Test Plan

Only Firefox seems to do this:

Diff Detail

Repository
R856 Plasma Browser Integration
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik created this revision.Apr 9 2020, 4:17 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 9 2020, 4:17 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Apr 9 2020, 4:17 PM
fvogt added a comment.Apr 9 2020, 5:05 PM

So was the case handled by line 805 completely broken?

It worked on Chrome, and the properties can be accessed from the same context.
It's just that the mutation.removedNodes loop above which is in content script cannot access those JS properties, so I had to change it, so it can.

fvogt added a comment.Apr 9 2020, 5:42 PM

It worked on Chrome, and the properties can be accessed from the same context.
It's just that the mutation.removedNodes loop above which is in content script cannot access those JS properties, so I had to change it, so it can.

Before this patch it didn't attempt to read the property on Chrome either though, so I don't see how the player.parentNode.removeChild(player); didn't immediately result in the mutation observer to call playerGone.

The change looks good, but I'm wondering how it ever worked previously...

Yeah, wondering the same... maybe it didn't. Anyway this also fixes Spotify's previous/next buttons not working since when the player is gone we clear media session actions (not sure if that is someting we should do though)

fvogt accepted this revision.Apr 10 2020, 10:23 AM

Yeah, wondering the same... maybe it didn't. Anyway this also fixes Spotify's previous/next buttons not working since when the player is gone we clear media session actions (not sure if that is someting we should do though)

Some logging reveals that with Vivaldi on Google translate there's a sequence of added, removed, added. After commenting out the player.parentNode.removeChild(player);, only an added event remains. So the removal triggers an add event?

This revision is now accepted and ready to land.Apr 10 2020, 10:23 AM
This revision was automatically updated to reflect the committed changes.