1503804 - Since Serde changes "Take Element Screenshot" screenshots the viewport and not the element only

Status: RESOLVED FIXED

(Testing :: geckodriver, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Originally reported as: https://github.com/mozilla/geckodriver/issues/1413

Since my Serde changes on bug 1396821 we no longer only screenshot the element but the viewport.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Now that we have better screenshot helpers for wdspec tests, which should get at least one more test added.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Here trace logs from before and after the Serde conversion:

Before:
> 1541067402261	webdriver::server	DEBUG	-> POST /session/767b55bb-aba8-2b4a-98fe-2f36b271ccca/element {"using": "css selector", "value": "[id=\"nothing2\"]"}
> 1541067402263	Marionette	TRACE	0 -> [0,3,"WebDriver:FindElement",{"using":"css selector","value":"[id=\"nothing2\"]"}]
> 1541067402280	Marionette	TRACE	0 <- [1,3,null,{"value":{"element-6066-11e4-a52e-4f735466cecf":"12b11b17-af5a-974b-87d4-8d5d4b563c74","ELEMENT":"12b11b17-af5a-974b-87d4-8d5d4b563c74"}}]
> 1541067402281	webdriver::server	DEBUG	<- 200 OK {"value":{"element-6066-11e4-a52e-4f735466cecf":"12b11b17-af5a-974b-87d4-8d5d4b563c74"}}
> 1541067402282	webdriver::server	DEBUG	-> GET /session/767b55bb-aba8-2b4a-98fe-2f36b271ccca/element/12b11b17-af5a-974b-87d4-8d5d4b563c74/screenshot 
> 1541067402284	Marionette	TRACE	0 -> [0,4,"WebDriver:TakeScreenshot",{"full":false,"highlights":[],"id":"12b11b17-af5a-974b-87d4-8d5d4b563c74"}]

After:
> 1541070433340	webdriver::server	DEBUG	-> POST /session/0c2895f5-8f84-934f-980e-43b7a3540654/element {"using": "css selector", "value": "[id=\"nothing2\"]"}
> 1541070433349	Marionette	TRACE	0 -> [0,3,"WebDriver:FindElement",{"using":"css selector","value":"[id=\"nothing2\"]"}]
> 1541070433355	Marionette	TRACE	0 <- [1,3,null,{"value":{"element-6066-11e4-a52e-4f735466cecf":"20519704-b136-1e4e-a4a2-e69b4296451e"}}]
> 1541070433357	webdriver::server	DEBUG	<- 200 OK {"value":{"element-6066-11e4-a52e-4f735466cecf":"20519704-b136-1e4e-a4a2-e69b4296451e"}}
> 1541070433358	webdriver::server	DEBUG	-> GET /session/0c2895f5-8f84-934f-980e-43b7a3540654/element/20519704-b136-1e4e-a4a2-e69b4296451e/screenshot 
> 1541070433361	Marionette	TRACE	0 -> [0,4,"WebDriver:TakeScreenshot",{"element":{"element-6066-11e4-a52e-4f735466cecf":"20519704-b136-1e4e-a4a2-e69b4296451e"},"full":false,"highlights":[]}]

So the problem is that we now pass the element via `element` but not `id`. As such it is not recognized.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ouch:

https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/testing/geckodriver/src/marionette.rs#921-922

>  data.insert("element".to_string(), serde_json::to_value(e)?);
>  // data.insert("id".to_string(), e.id.to_json());

Looks like that I missed to uncomment this line when finalizing the patch. :/

Further we can also remove the implementation of `to_marionette()` for `TakeScreenshotParameters` because it's not used anywhere.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

First try job including Rust tests, where Wd1 failed for all platforms because I didn't check our Mozilla specific wdspec tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5935ee5dbdf4c329432edb96891f24679ed8c5c&selectedJob=209168101

Most recent try job with lesser changes to helpers, which will fix the Wd1 failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d285c58039686686d529e11992e7edadc9d4ec52

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1503804 - [geckodriver] Fix serialization of "Take Element Screenshot" parameters for Marionette. r?ato

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1503804 - [wdspec] Use shared assert_png() method in screenshot tests. r?ato

Depends on D10626

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1503804 - [wdspec] Assert for expected screenshot dimensions. r?ato

Depends on D10627

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb03520d8991
[geckodriver] Fix serialization of "Take Element Screenshot" parameters for Marionette. r=ato
https://hg.mozilla.org/integration/autoland/rev/8ee95012eaf0
[wdspec] Use shared assert_png() method in screenshot tests. r=ato
https://hg.mozilla.org/integration/autoland/rev/076d32d1343f
[wdspec] Assert for expected screenshot dimensions. r=ato

Comment 9

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13929 for changes under testing/web-platform/tests

Comment 10

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13929
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/451005144?utm_source=github_status&utm_medium=notification)
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/LIrnFNp0T-qTtN05hRn8Ow)

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/fb03520d8991
https://hg.mozilla.org/mozilla-central/rev/8ee95012eaf0
https://hg.mozilla.org/mozilla-central/rev/076d32d1343f

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://github.com/web-platform-tests/wpt/pull/13929 got merged about 30min ago. Did the sync bot got stuck somewhere so it didn't notice that?

Comment 13

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 14

[James Graham [:jgraham]]

I poked it a bit.