[Bug]: Duplicate <footer> on AMO version-history page, with its white text mostly-invisible but superimposed with the actual page content

[dholbert]

What happened?

If you visit https://addons.mozilla.org/en-US/firefox/addon/bitwarden-password-manager/versions with a small window (e.g. 950px by 950px) and scroll down a little bit, you can sort of see there's some white text from a footer that's mysteriously present in the left sidebar that seems to overshoot onto the main page content.

The overshooting text is mostly white-on-white so it's mostly not visible, but it overlaps the actual content and causes artifacts.

If you look at the page DOM in devtools, you can see that this is actually a redundant <footer> element which is a next-sibling of the #react-view element. (There's another <footer> that's correctly placed, which is a descendant of the #react-view element.)

What did you expect to happen?

No such redundant footer overlapping content.

Is there an existing issue for this?

• [X] I have searched the existing issues

┆Issue is synchronized with this Jira Task

[dholbert]

Screenshots showing this out-of-place footer, clearly visible in the part overlapping the left sidebar, and somewhat-visible when overlapping content in the main area (creating what look like visual artifacts over the words "and Safari only" in this case): (note here^, the "Creative Commons" text is part of the text in the footer -- you can't see its surrounding text because it's white-text-on-white-background) (note here^, I've manually changed the footer text to have a red color instead of a white color so that you can see it all quite clearly)

[dholbert]

screenshot of devtools showing that we have two <footer> elements (the second one being the unwanted one that's causing trouble here):

[dholbert]

This doesn't reproduce on all version-history pages. E.g. this one is fine: https://addons.mozilla.org/en-US/firefox/addon/languagetool/versions/ ...but this one is reliably broken (for me at least): https://addons.mozilla.org/en-US/firefox/addon/bitwarden-password-manager/versions/

[diox]

The problem is also slightly different when JS is off. It looks like something in the same vein of https://github.com/mozilla/addons/issues/14200, where the HTML is bad (unclosed element, element inside another element it's not supposed to be in, etc) and that React tries to reconcile its DOM poorly.

The fact that it only happens for this specific add-on is weird... maybe has to something to do with the content of their release notes.

[diox]

Bitwarden version 2024.4.2 has the following release notes:

<li>Extension requires additional browser permissions to better manage content script injection.</li>
<li>Small bug fixes and improvements</li>

We do allow developers to use <li> in there, but they should be enclosed in <ul>... On that page, each version is already in a <li> of its own, and I'm guessing that's what's causing the issue here.

[diox]

Can be reproduced at https://addons-dev.allizom.org/en-US/firefox/addon/ratopuse-weasp-vulturkal/versions/

You can see the content jump around if you reload that page, and the footer content being duplicated and displayed on top of the normal footer (or, depending on your window size, the main body content)

[diox]

Rough potential fix:

  _purify.addHook(
    'uponSanitizeElement',
    function (node) {
      if (node.tagName === 'LI' && !['OL', 'UL'].includes(node.parentNode.tagName)) {
          const oldParent = node.parentNode;
          const newParent = node.ownerDocument.createElement('ul');
          newParent.appendChild(node);
          oldParent.appendChild(newParent);
      }
    }
  );

But we need to do some refactoring to only do that in sanitizeUserHTML instead of the more global sanitizeHTML. The former doesn't have access to DOMPurify directly, hence the need for refactoring.

Any fix will have to be checked with and without client-side JavaScript: it should work when loading the page server-side with or without JS enabled in the browser, and when navigating from the add-on page.

[diox]

We should have another issue to look into alternatives to HTML and/or how to address similar issues with other elements we currently allow. I think it's only <li> but not 100% sure.

[diox]

bleach ensures tags are closed properly, so at least we don't have to worry about unbalanced tags.

We allow:

• a
• abbr
• acronym
• b
• blockquote
• code
• em
• i
• li
• ol
• strong
• ul

I've done various tests and only the <li> outside of a <ul> or <ol> is problematic. Technically, blockquote, ul or ol are not allowed inside the inline ones but in practice doesn't cause problems. We also accept any content inside <ul> and <ol> but again, that doesn't cause any issues.

So the only thing that really cause issues at this point are <li> without a <ul> or <ol> parent, which my approach would solve.