Screenshot configurations should supply a relative CSS selector

RESOLVED FIXED in Firefox 58

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jaws, Assigned: rndmustafa, Mentored)

Tracking

(Blocks 1 bug)

Version 3
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment)

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 hidden (mozreview-request)
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.
Attachment #8915691 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
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.
Attachment #8915691 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
Comment on attachment 8915691 [details]
Bug 1403680 - Screenshot configurations should supply a relative CSS selector.

https://reviewboard.mozilla.org/r/186894/#review192454
Attachment #8915691 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)

Comment 8

2 years ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/398ef4e2ae5d
Screenshot configurations should supply a relative CSS selector. r=jaws
https://hg.mozilla.org/mozilla-central/rev/398ef4e2ae5d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.