Skip to content

Instantly share code, notes, and snippets.

@JorikSchellekens
Last active December 18, 2018 06:39
Show Gist options
  • Save JorikSchellekens/03bdcab7a7bccb714ba23da59c0fe67c to your computer and use it in GitHub Desktop.
Save JorikSchellekens/03bdcab7a7bccb714ba23da59c0fe67c to your computer and use it in GitHub Desktop.

Lyricist lyric injection bugfix

The current extension cycle is triggered by an event listener on onUpdate over all the tabs. The event is then broadcast to the extension with the TAB_CHANGED id. If a lyrics view is currently present it is removed and then the metadata of the song is fetched, the lyrics fetched and then inserted into the #related div.

As follows:

+----------+
|          |
| onUpdate |
|          |
+----+-----+
     |
     |
+----v--------+
|             |
| clearLyrics |
|             |
+----+--------+
     |
     |
+----v--------+
|             |
| getMetadata |
|             |
+----+--------+
     |
     |
+----v------+
|           |
| getLyrics |
|           |
+----+------+
     |
     |
+----v-------------+
|                  |
| createLyricsView |
|                  |
+------------------+

The bug

The onUpdate event was regularly fired and acted on before the DOM was updated which resulted in either in the lyrics of the previous song title to be loaded. Or the lyrics injected into the #related container and then being overwritten by the update.

In other cases the onUpdate could be fired multiple times without the YouTube single page changing in any meaningful way. In this case there would be multiple copies of the lyrics displayed. Multiple copies could be avoided by history tracking or by inspecting for an already present lyrics view however the proposed bugfix for the previous case of bugs will handle this in a far more elegant way.

The fix

Instead of setting an event listener on onUpdate I would propose to set a mutation listener on the title of the page. Doing so would only update the lyrics once the page has changed in a meaningful way (that is: a new video has loaded via autoplay; or the user has navigated to another page).

Resulting in a flow such as:

+----------------------------+
|                            |
| MutationObserver on title  |
|                            |
+----+-----------------------+
     |
     |
+----v--------+
|             |
| clearLyrics |
|             |
+----+--------+
     |
     |
+----v--------+
|             |
| getMetadata |
|             |
+----+--------+
     |
     |
+----v------+
|           |
| getLyrics |
|           |
+----+------+
     |
     |
+----v-------------+
|                  |
| createLyricsView |
|                  |
+------------------+

Repercussions of the fix

At this suggestion, we realised that the document title would itself contain the information we need to get the song's metadata. Which was taken up here.

So, the title parser will now work off of the webpage's title using document.title

Introduced edge cases

Using the MutationObserver on the document title introduces an edge case when the page is first loaded. We've decided to cache the events and ignore repeats which occur under 10ms from the next.

Implementation is handled as follows:

The MutationObserver takes a callback which must accept a mutationsList and an observer. We've added a caching system in js/utils/mutation_handlers/ which makes a record of the last event witnessed and check whether we should load new lyrics based on any of the mutations in the mutationslist. The criteria we set are:

  • The last title mutation has a different title to the current.
  • If the last title mutation had the same title it happened more than 10ms ago.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment