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

RESOLVED FIXED in Firefox 65

Status

defect
P1
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

(Blocks 1 bug, {regression})

Trunk
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

(URL)

Attachments

(3 attachments)

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

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.
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.
Blocks: 1495062
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 8

7 months ago
Pushed by hskupin@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13929 for changes under testing/web-platform/tests

Comment 11

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fb03520d8991
https://hg.mozilla.org/mozilla-central/rev/8ee95012eaf0
https://hg.mozilla.org/mozilla-central/rev/076d32d1343f
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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?
Flags: needinfo?(james)
Upstream PR merged
I poked it a bit.
Flags: needinfo?(james)
You need to log in before you can comment on or make changes to this bug.