Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutation Observer does not seems to work correctly #458

Closed
cbou opened this issue Oct 26, 2021 · 2 comments · Fixed by #460
Closed

Mutation Observer does not seems to work correctly #458

cbou opened this issue Oct 26, 2021 · 2 comments · Fixed by #460
Assignees
Labels

Comments

@cbou
Copy link

cbou commented Oct 26, 2021

On my app the roles that are set with JavaScript later after the page load are not detected by the plugin.

I look at the code and found out that you use a MutationObserver which should be used in this case.

Actually this does not seems to work.
Following is a HTML page which define a landmark after 2 seconds.

The landmark won't be detected by the plugin.
If you click on another tab and then back to this dummy page then the landmark will be find.

Is that the expected behaviour?

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>Hello World! Site Title</title>
  </head>
  <body>
    <div class="foo">
      <h1>Hello World!</h1>
    </div>
    <script>
      setTimeout(() => {
        document.querySelector('.foo').setAttribute("aria-label", "bar")
        document.querySelector('.foo').setAttribute("role", "banner")
        alert('Landmarks added')
      }, 2000)
    </script>
  </body>
</html>
@matatk
Copy link
Owner

matatk commented Oct 27, 2021

Thanks for your clear bug report and test page. I can reproduce the issue using Firefox (93) and Chrome (95). Interestingly, the "Mutation information" section in the DevTools > DOM Inspector > Landmarks pane shows that no mutations were detected. The reason why the landmark shows up when you move back to the page is that Landmarks will always run a scan shortly after a page becomes visible, in case the page changed whilst it was hidden. (The reason for the delay after the page becomes visible is that you might be switching tabs using the keyboard, to move through several tabs in quick succession.)

I tried making a debug build of the extension to get more info, and it seems that the MutationObserver itself s not firing—but I then tried creating one directly on the test page, and it did fire. I will need to investigate this more to find out the cause. It's likely to be a few days at least before I can do that; I will update this thread with any news.

@matatk matatk self-assigned this Oct 27, 2021
@matatk matatk added the bug label Oct 27, 2021
@matatk
Copy link
Owner

matatk commented Nov 29, 2021

I made a separate, minimal browser extension to try and track the problem down, and I was able to get it to work with your example code (i.e. the mutations were detected by the extension's content script).

Looks like this problem was introduced in a change to the content script that is causing mutations to only be observed after the page is hidden and then re-shown; quite a big mistake—oh dear! Thanks again for raising this, and providing a clear and minimal example.

I've often thought it would be good to have tests for at least some key behaviours of the extension (to go alongside the existing tests for the accuracy of scans), but the effort of mocking the whole extension environment would be very great, and I haven't figured out a way to test parts in a more isolated fashion. On the plus side, this code changes very rarely, and I put in a lot of logging; will aim to strengthen that and test it for longer in future.

There are a couple of details I'm still working out for this, but I will fix it as soon as possible.

matatk added a commit that referenced this issue Nov 30, 2021
…ges (#460)

PR #428 introduced a bug whereby the MutationObserver wouldn't be wired up until the page became visible (i.e. either the user moves to it for the first time, if it was loaded in a background tab, or the user moves away from and back to it, if it was loaded in the foreground tab). This was discovered and reported by @cbou in #458.

Fixes #458
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants