Closed Bug 1503804 Opened 2 years ago Closed 2 years ago

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


(Testing :: geckodriver, defect, P1)



(firefox-esr60 unaffected, firefox63 wontfix, firefox64 wontfix, firefox65 fixed)

Tracking Status
firefox-esr60 --- unaffected
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- fixed


(Reporter: whimboo, Assigned: whimboo)


(Blocks 1 open bug, )


(Keywords: regression)


(3 files)

Originally reported as:

Since my Serde changes on bug 1396821 we no longer only screenshot the element but the viewport.
Now that we have better screenshot helpers for wdspec tests, which should get at least one more test added.
Here trace logs from before and after the Serde conversion:

> 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"}]

> 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.

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

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.
First try job including Rust tests, where Wd1 failed for all platforms because I didn't check our Mozilla specific wdspec tests:

Most recent try job with lesser changes to helpers, which will fix the Wd1 failures:
Pushed by
[geckodriver] Fix serialization of "Take Element Screenshot" parameters for Marionette. r=ato
[wdspec] Use shared assert_png() method in screenshot tests. r=ato
[wdspec] Assert for expected screenshot dimensions. r=ato
Created web-platform-tests PR for changes under testing/web-platform/tests
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65 got merged about 30min ago. Did the sync bot got stuck somewhere so it didn't notice that?
Flags: needinfo?(james)
I poked it a bit.
Flags: needinfo?(james)
You need to log in before you can comment on or make changes to this bug.