Closed Bug 1213875 Opened 9 years ago Closed 7 years ago

Scroll element into view on screenshot of element

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

(Keywords: pi-marionette-server, pi-marionette-spec)

Attachments

(4 files, 1 obsolete file)

Bug 1213875 - Lint arguments in capture module;
58 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1213875 - Add ability to not scroll into view element on screen capture;
58 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1213875 - Add scroll argument to Marionette client;
58 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1213875 - Test scroll override behaviour;
58 bytes, text/x-review-board-request
whimboo
: review+
Details 
Marionette does not scroll the element into view when taking a screen capture of it.  The WebDriver specification mandates that it should.
Blocks: webdriver
Keywords: ateam-marionette-server
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.
Keywords: ateam-marionette-spec
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.
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
Sorry, I meant the opposite.  If scroll is true or undefined, do scroll.
Assignee: nobody → ato
Status: NEW → ASSIGNED
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 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.
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 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 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 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-
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.
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.
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?
Attachment #8822460 - Attachment is obsolete: true
Attachment #8822460 - Flags: review?(hskupin)
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 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 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.
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 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 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 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)
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.
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.
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 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 on attachment 8820253 [details]
Bug 1213875 - Test scroll override behaviour;

https://reviewboard.mozilla.org/r/99796/#review102810
Attachment #8820253 - Flags: review?(hskupin) → review+
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
Sheriffs: Please uplift this to Aurora as test-only.
Whiteboard: [checkin-needed-aurora]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: