1403680 - Screenshot configurations should supply a relative CSS selector

Status: RESOLVED FIXED

(Testing :: mozscreenshots, enhancement)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Each screenshot configuration should supply a relative CSS selector that defines the region the screenshots should focus on.

The selector should live in each sub-configuration.

Using Tabs.jsm as an example:

this.Tabs = {
  init(libDir) {},

  configurations: {
    fiveTabs: {
      selector: "#tabbrowser-tabs",
      async applyConfig() {
        fiveTabsHelper();
        let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
        hoverTab(browserWindow.gBrowser.tabs[3]);
        await new Promise((resolve, reject) => {
          setTimeout(resolve, 3000);
        });
      },
    },

    fourPinned: {
      selector: "#tabbrowser-tabs",
      async applyConfig() {
        ...
      }
    }
  }
}

Comment 1

[rndmustafa]

Attachment: Bug 1403680 - Screenshot configurations should supply a relative CSS selector.

Each configuration now has the selectors property, an array of strings containing it's css selectors.
These selectors will be used to implement the automated cropping feature.

Review commit: https://reviewboard.mozilla.org/r/186894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186894/

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8915691 [details]
Bug 1403680 - Screenshot configurations should supply a relative CSS selector.

https://reviewboard.mozilla.org/r/186894/#review192012

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/AppMenu.jsm:18
(Diff revision 1)
>  this.AppMenu = {
>  
>    init(libDir) {},
>  
>    configurations: {
> +    selectors: ["#appMenu-mainView"],

Change this to #appMenu-popup so it include the whole popup and not one of the inner boxes.

For panels, like this, we should select the whole panel so that we will have the arrow, border, and shadow included in the screenshot.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/AppMenu.jsm:27
(Diff revision 1)
>          browserWindow.PanelUI.hide();
>        },
>      },
>  
>      appMenuMainView: {
> +      selectors: ["#appMenu-mainView"],

Ditto.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/AppMenu.jsm:37
(Diff revision 1)
>          return promise;
>        },
>      },
>  
>      appMenuHistorySubview: {
> +      selectors: ["#PanelUI-history"],

Because this is inside of the #appMenu-popup, we can just use #appMenu-popup here too for the same reason as outlined in the above comment.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/AppMenu.jsm:55
(Diff revision 1)
>  
>        verifyConfig: verifyConfigHelper,
>      },
>  
>      appMenuHelpSubview: {
> +      selectors: ["#PanelUI-helpView"],

Ditto.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Buttons.jsm:29
(Diff revision 1)
>          CustomizableUI.addWidgetToArea("screenshot-widget", CustomizableUI.AREA_NAVBAR);
>        },
>      },
>  
>      tabsToolbarButtons: {
> +      selectors: ["#nav-bar"],

The configuration below adds the button to the tabstrip, so this selector won't include the button.

It should be #TabsToolbar.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Buttons.jsm:31
(Diff revision 1)
>      },
>  
>      tabsToolbarButtons: {
> +      selectors: ["#nav-bar"],
>        applyConfig: async () => {
>          CustomizableUI.addWidgetToArea("screenshot-widget", CustomizableUI.AREA_TABSTRIP);

CustomizableUI.AREA_TABSTRIP

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Buttons.jsm:36
(Diff revision 1)
>          CustomizableUI.addWidgetToArea("screenshot-widget", CustomizableUI.AREA_TABSTRIP);
>        },
>      },
>  
>      menuPanelButtons: {
> +      selectors: ["#appMenu-mainView"],

This needs to be updated to point at the overflow panel.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Buttons.jsm:38
(Diff revision 1)
>      },
>  
>      menuPanelButtons: {
> +      selectors: ["#appMenu-mainView"],
>        applyConfig: async () => {
>          CustomizableUI.addWidgetToArea("screenshot-widget", CustomizableUI.AREA_PANEL);

CustomizableUI.AREA_FIXED_OVERFLOW_PANEL (AREA_PANEL doesn't exist anymore)

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Buttons.jsm:51
(Diff revision 1)
>          return Promise.resolve("menuPanelButtons.verifyConfig");
>        },
>      },
>  
>      custPaletteButtons: {
> +      selectors: [],  // Unsure what this is looking for

This is looking at the "Customize" mode tool palette. You can see this by clicking on the Firefox menu and choosing "Customize...". You can also access this by right-clicking the navigation toolbar and choosing "Customize...".

This test is checking that the layout of Customize mode doesn't change unexpectedly.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/CustomizeMode.jsm:42
(Diff revision 1)
>          });
>        },
>      },
>  
>      customizing: {
> +      selectors: ["#navigator-toolbox"],

This should include the palette area too.

["#navigator-toolbox", "#customization-container"]

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/DevTools.jsm:55
(Diff revision 1)
>          await gDevTools.showToolbox(getTargetForSelectedTab(), "inspector", "side");
>          await new Promise(resolve => setTimeout(resolve, 500));
>        },
>      },
>      undockedToolbox: {
> +      selectors: ["#toolbox-container"],  // This opens a new window, can it still be selected like this?

You should add another property of a configuration that is for the window type. If the `windowType` property doesn't exist, then TestRunner.jsm can just use `"navigator:browser"` when it calls the `Services.wm.getMostRecentWindow` function.

In this case, your `windowType` property would return `"devtools:toolbox"`

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Preferences.jsm:38
(Diff revision 1)
>        let configName = primary.replace(/^pane/, "prefs");
>        if (customFn) {
>          configName += "-" + customFn.name;
>        }
>        this.configurations[configName] = {};
> +      this.configurations[configName].selectors = [];

Do you plan on adding selectors here? Maybe you forgot?

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/TabsInTitlebar.jsm:32
(Diff revision 1)
>          return undefined;
>        },
>      },
>  
>      tabsOutsideTitlebar: {
> +      selectors: ["#navigator-toolbox"], // Is it possible to select the title bar as well?

#titlebar should select the titlebar, though if the title bar is drawn natively then we won't have a selector for it.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/WindowSize.jsm:23
(Diff revision 1)
>      Services.prefs.setBoolPref("browser.fullscreen.autohide", false);
>    },
>  
>    configurations: {
>      maximized: {
> +      selectors: [],

You can set these ones to `:root` since we'll want to screenshot the whole window.

Comment 3

[rndmustafa]

Comment on attachment 8915691 [details]
Bug 1403680 - Screenshot configurations should supply a relative CSS selector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186894/diff/1-2/

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8915691 [details]
Bug 1403680 - Screenshot configurations should supply a relative CSS selector.

https://reviewboard.mozilla.org/r/186894/#review192396

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Buttons.jsm:31
(Diff revision 2)
>      },
>  
>      tabsToolbarButtons: {
> +      selectors: ["#TabsToolbar"],
>        applyConfig: async () => {
> -        CustomizableUI.addWidgetToArea("screenshot-widget", CustomizableUI.AREA_TABSTRIP);
> +        CustomizableUI.addWidgetToArea("screenshot-widget", CustomizableUI.TABSTRIP);

Sorry, this line should be reverted to use AREA_TABSTRIP.

Yesterday when I was reviewing your patch I was going to propose a different change to the line. Before finalizing my review I removed my proposal, and my note about the second argument was left in but it wasn't necessary (it was just repeating what was already there, not asking to change it).

Today when you asked what I meant I think our lines of communication got crossed. When I said "it should just be TABSTRIP" I had meant AREA_TABSTRIP like it was before, but I can see how easily it would have been misinterpreted.

Are you running the test after you make these changes? CustomizableUI.TABSTRIP doesn't exist and would fail. You should be running these tests as you are making these changes.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/ControlCenter.jsm:34
(Diff revision 2)
>  this.ControlCenter = {
>    init(libDir) { },
>  
>    configurations: {
>      about: {
> +      selectors: ["#browser"],

This one should be #identity-popup too.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/ControlCenter.jsm:42
(Diff revision 2)
>          await openIdentityPopup();
>        },
>      },
>  
>      localFile: {
> +      selectors: ["#browser"],

#identity-popup

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/DevTools.jsm:55
(Diff revision 2)
>          await gDevTools.showToolbox(getTargetForSelectedTab(), "inspector", "side");
>          await new Promise(resolve => setTimeout(resolve, 500));
>        },
>      },
>      undockedToolbox: {
> +      selectors: [],

In the Browser Toolbox, there is a button that lets you change which window the toolbox is focused on. It's in the top right of the window with the tooltip "Select an iframe as the currently targeted document". You can use this to change focus to the toolbox window and then inspect it there to find the selector.

Comment 5

[rndmustafa]

Comment on attachment 8915691 [details]
Bug 1403680 - Screenshot configurations should supply a relative CSS selector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186894/diff/2-3/

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8915691 [details]
Bug 1403680 - Screenshot configurations should supply a relative CSS selector.

https://reviewboard.mozilla.org/r/186894/#review192454

Comment 7

[rndmustafa]

Comment on attachment 8915691 [details]
Bug 1403680 - Screenshot configurations should supply a relative CSS selector.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186894/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/398ef4e2ae5d
Screenshot configurations should supply a relative CSS selector. r=jaws

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/398ef4e2ae5d