1926125 - Sidebar button lingering after test despite sidebar.revamp = false

Status: NEW

(Firefox :: Sidebar, task)

Description

[Stephen Thompson [:sthompson]]

It looks like there may be a test that adds the sidebar button to the UI but fails to reset the UI after the test completes.

I added a new test for keyboard control of tab groups in https://phabricator.services.mozilla.com/D226154. As part of the test setup, I had the test focus on the URL bar and then Shift + Tab back to get the active tab in focus. This worked locally.

On a try run, the test timed out. On a screenshot from one of those runs https://firefoxci.taskcluster-artifacts.net/LbW-hIu6Qf2eaEUovy98PQ/0/public/test_info/mozilla-test-fail-screenshot_wuon6k4w.png you can see that the sidebar button is present and received focus instead of the new tab button, like I expected.

I added some code to expect to focus on #sidebar-button if sidebar.revamp=true, or else expect the new tab button. On a try run, the test found sidebar.revamp to be false, yet the screenshot from that try run showed a sidebar button with focus in the UI.

I think the only way that scenario would happen is if a test added it and didn't clean up. It looks to me like there are a number of tests that add the sidebar button, but it also looks like they mostly/all have cleanup logic. Perhaps there is a hole in one of them, but I wasn't able to identify where.

Comment 1

[Sarah Clements [:sclements]]

Thanks for the bug. Can you share what test failed that you grabbed the screenshot from?

Comment 2

[Stephen Thompson [:sthompson]]

It's a test that hasn't landed yet. The relevant code from the test was:

const sidebarRevampEnabled = Services.prefs.getBoolPref(
  "sidebar.revamp",
  false
);

// ...

info("Move focus to location bar using the keyboard");
await synthesizeKeyAndWaitForFocus(gURLBar, "l", { accelKey: true });
is(document.activeElement, gURLBar.inputField, "urlbar should be focused");

if (sidebarRevampEnabled) {
  info("Move focus to sidebar button using the keyboard");
  const sidebarButton = document.getElementById("sidebar-button");
  await synthesizeKeyAndWaitForFocus(
    sidebarButton,
    "VK_TAB",
    { shiftKey: true }
  );
}

info("Move focus to new tab button using the keyboard");
await synthesizeKeyAndWaitForFocus(
  document.getElementById("tabs-newtab-button"),
  "VK_TAB",
  { shiftKey: true }
);

// ...

The test outputted Move focus to location bar using the keyboard, reported a test pass for urlbar should be focused, outputted Move focus to new tab button using the keyboard, then failed on timeout waiting for the new tab button to receive focus.

Since the screenshot showed the sidebar button being in focus at the time of the test failure, that suggested to me that 1) sidebar.revamp was false and so the test did not try to focus on it, and 2) the sidebar button was nevertheless present in the UI.

Comment 3

[Nikki Sharpley (:nikkis) (she/her)]

Stephen, is this still causing an issue with your tests? Its possible a prior test involving the sidebar revamp requires CustomizableUI.reset() to be added to the clean up function.

Comment 4

[Stephen Thompson [:sthompson]]

Hi, Nikki. I'm not blocked. In the end, I changed my approach for test setup. Instead of pressing Shift + Tab a variable number of times, I decided to just assign document.activeElement to the active tab.