Scroll element into view on screenshot of element

RESOLVED FIXED in Firefox 52

Status

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: ato, Assigned: ato)

Tracking

(Blocks 1 bug, {pi-marionette-server, pi-marionette-spec})

unspecified
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Assignee

Description

4 years ago
Marionette does not scroll the element into view when taking a screen capture of it.  The WebDriver specification mandates that it should.
Assignee

Updated

4 years ago
actually we need to clarification from MS about this. They wanted to an argument to scroll or not from previous discussions and the spec might need updating here.
Assignee

Comment 2

4 years ago
The clarification from TPAC 2015 in Sapporo was that this was still needed.  I will go ahead an make the necessary adjustments to the specifications.
Assignee

Comment 3

4 years ago
I see the `scroll` argument, which if undefined or false means “please scroll”, has already made it into spec: https://w3c.github.io/webdriver/webdriver-spec.html#take-element-screenshot
Hm, so "scroll=true" should not scroll? Looks like the values are reversed?
Summary: Scroll element into view on screen capture of element → Scroll element into view on screenshot of element
Assignee

Comment 5

3 years ago
Sorry, I meant the opposite.  If scroll is true or undefined, do scroll.
Assignee

Updated

3 years ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

3 years ago
mozreview-review
Comment on attachment 8820250 [details]
Bug 1213875 - Lint arguments in capture module;

https://reviewboard.mozilla.org/r/99790/#review100258
Attachment #8820250 - Flags: review?(hskupin) → review+

Comment 15

3 years ago
mozreview-review
Comment on attachment 8820251 [details]
Bug 1213875 - Add ability to not scroll into view element on screen capture;

https://reviewboard.mozilla.org/r/99792/#review100260

This patch seems to bust all wpt-reftests. Also I think it's not an ideal time to work on it based that my patch for chrome support has still not been landed and this patch causes a lot of merge conflicts.
Assignee

Comment 16

3 years ago
mozreview-review-reply
Comment on attachment 8820251 [details]
Bug 1213875 - Add ability to not scroll into view element on screen capture;

https://reviewboard.mozilla.org/r/99792/#review100260

Conflicts can be resolved; this change isn’t particularly invasive.  Work in the same area on something positively different shouldn’t block a simultanous review of this changeset.

I will look at the WPT test failures soon.

Comment 17

2 years ago
mozreview-review
Comment on attachment 8820251 [details]
Bug 1213875 - Add ability to not scroll into view element on screen capture;

https://reviewboard.mozilla.org/r/99792/#review101698

::: testing/marionette/driver.js:2398
(Diff revision 2)
>   *     List of web elements to highlight.
> - * @param {boolean} hash
> + * @param {boolean=} hash
>   *     True if the user requests a hash of the image data.
> + * @param {boolean=} scroll
> + *     Scroll to element if |id| is provided.  If undefined, it will
> + *     scroll to the element.

nit: extra space between provided and if.

::: testing/marionette/driver.js:2453
(Diff revision 2)
>        break;
>  
>      case Context.CONTENT:
> -      if (hash) {
> -        return this.listener.getScreenshotHash(id, full, highlights);
> -      } else {
> +      return this.listener.takeScreenshot(
> +          hash ? capture.Format.Hash : capture.Format.Base64,
> +          cmd.parameters);

Lets figure out which type of data we want to have before we call takeScreenshot. As best do it at the top because for chrome we need the same distinction now.

::: testing/marionette/listener.js:1575
(Diff revision 2)
>   *
> - * @return {string}
> - *     Base64 encoded string of an image/png type.
> - */
> -function takeScreenshot(id, full=true, highlights=[]) {
> -  let canvas = screenshot(id, full, highlights);
> + *     @param {UUID=} id
> + *         Optional web element reference of an element to take a screenshot
> + *         of.
> + *     @param {boolean=} full
> + *         True to take a screenshot of the entire document element.  Is not

Extra space again between the end of a sentence and the next one. Please fix them all.

::: testing/marionette/listener.js:1622
(Diff revision 2)
>      }
>  
> -    canvas = capture.element(node, highlightEls);
> +    canvas = capture.element(el, highlightEls);
>    }
>  
> -  return canvas;
> +  dump("canvas=" + canvas + "\n");

Please remove this left-over debug comment.

::: testing/marionette/listener.js:1626
(Diff revision 2)
>  
> -  return canvas;
> +  dump("canvas=" + canvas + "\n");
> +
> +  switch (format) {
> +    case capture.Format.Base64:
> +      return capture.toBase64(canvas);

Why don't we do this switch in driver.js so both content and chrome can benefit from?
Attachment #8820251 - Flags: review?(hskupin) → review-

Comment 18

2 years ago
mozreview-review
Comment on attachment 8820252 [details]
Bug 1213875 - Add scroll argument to Marionette client;

https://reviewboard.mozilla.org/r/99794/#review101702

::: testing/marionette/client/marionette_driver/marionette.py:2059
(Diff revision 2)
>  
>          body = {"id": element,
>                  "highlights": lights,
>                  "full": full,
> -                "hash": False}
> +                "hash": False,
> +                "scroll": scroll}

I would suggest you move the closing bracket to the next line, and also add a final comma. It will make the diff smaller in the future if other parameters have to be added.

Regarding backwards compatibility I assume older releases of Firefox will simply ignore this extra argument. So there is nothing we would have to do.
Attachment #8820252 - Flags: review?(hskupin) → review+

Comment 19

2 years ago
mozreview-review
Comment on attachment 8820253 [details]
Bug 1213875 - Test scroll override behaviour;

https://reviewboard.mozilla.org/r/99796/#review101706

::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:214
(Diff revision 2)
> +    def test_scroll_default(self):
> +        self.marionette.navigate(long)
> +        before = self.page_y_offset
> +        el = self.marionette.find_element(By.TAG_NAME, "p")
> +        self.marionette.screenshot(element=el)
> +        self.assertNotEqual(before, self.page_y_offset)

The long data url will cause a long execution time for Fennec. I wonder if we could add this check to a generic method which checks all the defaults at once.

::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:216
(Diff revision 2)
> +        before = self.page_y_offset
> +        el = self.marionette.find_element(By.TAG_NAME, "p")
> +        self.marionette.screenshot(element=el)
> +        self.assertNotEqual(before, self.page_y_offset)
> +
> +    def test_scroll(self):

My screenshot patch for chrome added a placeholder method for that. Please move the code into it.
Attachment #8820253 - Flags: review?(hskupin) → review-
Assignee

Comment 20

2 years ago
mozreview-review-reply
Comment on attachment 8820251 [details]
Bug 1213875 - Add ability to not scroll into view element on screen capture;

https://reviewboard.mozilla.org/r/99792/#review101698

> Why don't we do this switch in driver.js so both content and chrome can benefit from?

We have to return something that is JSON-serialisable, and a `<canvas>` element isn’t.
Assignee

Comment 21

2 years ago
mozreview-review-reply
Comment on attachment 8820252 [details]
Bug 1213875 - Add scroll argument to Marionette client;

https://reviewboard.mozilla.org/r/99794/#review101702

> I would suggest you move the closing bracket to the next line, and also add a final comma. It will make the diff smaller in the future if other parameters have to be added.
> 
> Regarding backwards compatibility I assume older releases of Firefox will simply ignore this extra argument. So there is nothing we would have to do.

As much as I dislike Python’s syntax, this is fairly common code formatting and incidentally what autopep8 produces.
Assignee

Comment 22

2 years ago
mozreview-review-reply
Comment on attachment 8820253 [details]
Bug 1213875 - Test scroll override behaviour;

https://reviewboard.mozilla.org/r/99796/#review101706

> The long data url will cause a long execution time for Fennec. I wonder if we could add this check to a generic method which checks all the defaults at once.

What performance penalty are we talking about?  I suspect it’s negliable.  I would rather not make premature performance optimisations to this unless there is a real need for it.

> My screenshot patch for chrome added a placeholder method for that. Please move the code into it.

Can you explain what ‘that’ is and how to use it?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8822460 - Attachment is obsolete: true
Attachment #8822460 - Flags: review?(hskupin)

Comment 31

2 years ago
mozreview-review-reply
Comment on attachment 8820251 [details]
Bug 1213875 - Add ability to not scroll into view element on screen capture;

https://reviewboard.mozilla.org/r/99792/#review101698

> We have to return something that is JSON-serialisable, and a `<canvas>` element isn’t.

Please see the new code in driver.js for taking a screenshot. Your patch should cover both contextes now.

Comment 32

2 years ago
mozreview-review-reply
Comment on attachment 8820252 [details]
Bug 1213875 - Add scroll argument to Marionette client;

https://reviewboard.mozilla.org/r/99794/#review101702

> As much as I dislike Python’s syntax, this is fairly common code formatting and incidentally what autopep8 produces.

But also adds extra lines to each diff because an addition to the dict needs a comma in the former line. It's just bah.

Comment 33

2 years ago
mozreview-review-reply
Comment on attachment 8820253 [details]
Bug 1213875 - Test scroll override behaviour;

https://reviewboard.mozilla.org/r/99796/#review101706

> What performance penalty are we talking about?  I suspect it’s negliable.  I would rather not make premature performance optimisations to this unless there is a real need for it.

This page simply produce a very long viewport and taking the screenshot already takes about 5s on my device. On the emulator we could multiply it with maybe 3 (I don't have numbers because I cannot run tests on the emulator).

> Can you explain what ‘that’ is and how to use it?

That in this case means test method. See the file for the todo item after you rebased against latest mozilla-central.
Assignee

Comment 34

2 years ago
mozreview-review-reply
Comment on attachment 8820253 [details]
Bug 1213875 - Test scroll override behaviour;

https://reviewboard.mozilla.org/r/99796/#review101706

> This page simply produce a very long viewport and taking the screenshot already takes about 5s on my device. On the emulator we could multiply it with maybe 3 (I don't have numbers because I cannot run tests on the emulator).

That is a lot more expensive than I thought.  If is the length of the data that is causing this, would returning a hash be acceptable?  For this test we don’t care about the returned data; just that calling the function causes/does not cause a scroll.

> That in this case means test method. See the file for the todo item after you rebased against latest mozilla-central.

I don’t think I understand this fully, but I’ve now removed `test_capture_scroll_element_into_view` that I found in `TestScreenCaptureChrome` since scrolling to an element in chrome context doesn’t make any sense.
Comment hidden (mozreview-request)

Comment 36

2 years ago
mozreview-review-reply
Comment on attachment 8820253 [details]
Bug 1213875 - Test scroll override behaviour;

https://reviewboard.mozilla.org/r/99796/#review101706

> That is a lot more expensive than I thought.  If is the length of the data that is causing this, would returning a hash be acceptable?  For this test we don’t care about the returned data; just that calling the function causes/does not cause a scroll.

Sounds totally fine with me.

Comment 37

2 years ago
mozreview-review
Comment on attachment 8820251 [details]
Bug 1213875 - Add ability to not scroll into view element on screen capture;

https://reviewboard.mozilla.org/r/99792/#review102224

::: testing/marionette/driver.js:2416
(Diff revision 4)
>   *     string.
>   */
>  GeckoDriver.prototype.takeScreenshot = function (cmd, resp) {
> -  let {id, highlights, full, hash} = cmd.parameters;
> +  let {id, highlights, full, hash, scroll} = cmd.parameters;
>    highlights = highlights || [];
> +  hash = hash ? capture.Format.Hash : capture.Format.Base64;

The assignment is confusing. I would call the variable `format` instead.

::: testing/marionette/listener.js:1635
(Diff revision 4)
> -  let canvas = screenshot(id, full, highlights);
> -  return capture.toHash(canvas);
> -}
> +  let id = opts.id;
> +  let full = !!opts.full;
> +  let highlights = opts.highlights || [];
> +  let scroll = !!opts.scroll;
>  
> -/**
> +  let highlightEls = highlights.map(ref => seenEls.get(ref, curContainer));

Please make the same change to the code in driver.js

::: testing/marionette/listener.js:1653
(Diff revision 4)
>      } else {
> -      node = curContainer.frame.document.documentElement;
> +      el = curContainer.frame.document.documentElement;
>      }
>  
> -    canvas = capture.element(node, highlightEls);
> +    if (scroll) {
> +      element.scrollIntoView(el);

The spec doesn't say that we should scroll into view if no element is given. You might want to update the if condition here.
Attachment #8820251 - Flags: review?(hskupin) → review-

Comment 38

2 years ago
mozreview-review
Comment on attachment 8820253 [details]
Bug 1213875 - Test scroll override behaviour;

https://reviewboard.mozilla.org/r/99796/#review102230

::: testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py:413
(Diff revision 5)
> +    def test_scroll_off(self):
> +        self.marionette.navigate(long)
> +        el = self.marionette.find_element(By.TAG_NAME, "p")
> +        before = self.page_y_offset
> +        self.marionette.screenshot(element=el, format="hash", scroll=False)
> +        self.assertEqual(before, self.page_y_offset)

Can you add another test for the case when no element has been specified?
Attachment #8820253 - Flags: review?(hskupin)
Assignee

Comment 39

2 years ago
mozreview-review-reply
Comment on attachment 8820251 [details]
Bug 1213875 - Add ability to not scroll into view element on screen capture;

https://reviewboard.mozilla.org/r/99792/#review102224

> The spec doesn't say that we should scroll into view if no element is given. You might want to update the if condition here.

Good catch.
Assignee

Comment 40

2 years ago
mozreview-review-reply
Comment on attachment 8820253 [details]
Bug 1213875 - Test scroll override behaviour;

https://reviewboard.mozilla.org/r/99796/#review101706

> Sounds totally fine with me.

Smashing.  Addressed this in the last revision.

> I don’t think I understand this fully, but I’ve now removed `test_capture_scroll_element_into_view` that I found in `TestScreenCaptureChrome` since scrolling to an element in chrome context doesn’t make any sense.

I think this is fixed in the rebase.
Assignee

Comment 41

2 years ago
mozreview-review-reply
Comment on attachment 8820253 [details]
Bug 1213875 - Test scroll override behaviour;

https://reviewboard.mozilla.org/r/99796/#review102230

> Can you add another test for the case when no element has been specified?

Yes, this is a good test case to include.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 46

2 years ago
mozreview-review
Comment on attachment 8820251 [details]
Bug 1213875 - Add ability to not scroll into view element on screen capture;

https://reviewboard.mozilla.org/r/99792/#review102808
Attachment #8820251 - Flags: review?(hskupin) → review+

Comment 47

2 years ago
mozreview-review
Comment on attachment 8820253 [details]
Bug 1213875 - Test scroll override behaviour;

https://reviewboard.mozilla.org/r/99796/#review102810
Attachment #8820253 - Flags: review?(hskupin) → review+

Comment 48

2 years ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f183b8b27c5
Lint arguments in capture module; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/a1295d5f6569
Add ability to not scroll into view element on screen capture; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/cbcb68e2fba7
Add scroll argument to Marionette client; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/8867228e0796
Test scroll override behaviour; r=whimboo
Assignee

Comment 50

2 years ago
Sheriffs: Please uplift this to Aurora as test-only.
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.