New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make screenshot of SelenideElement/WebElement which is inside iframe #705

Merged
merged 5 commits into from Apr 18, 2018

Conversation

Projects
None yet
6 participants
@andrejska
Contributor

andrejska commented Mar 22, 2018

Proposed changes

New feature: added possibility to make a screenshot of SelenideElement or WebElement which is inside iframe

Checklist

  • Checkstyle and unit tests pass locally with my changes by running gradle check chrome htmlunit command
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
Andrejs Kalnačs
@rosolko

This comment has been minimized.

Collaborator

rosolko commented Mar 22, 2018

@andrejska For first why don't you cover this feature with tests?
For second wht don't you reuse exist method for takeScreenshotAsImage(WebElement element) with minimal extend for frame switch?
Also isn't it work for you just take screenshot of frame?
What behavior do you expect if iframe or element inside iframe doesn't exist?

@rosolko rosolko added the feature label Mar 22, 2018

@coveralls

This comment has been minimized.

coveralls commented Mar 22, 2018

Coverage Status

Coverage increased (+1.3%) to 66.812% when pulling f4e91e4 on andrejska:master into 1c98a7a on codeborne:master.

@codecov-io

This comment has been minimized.

codecov-io commented Mar 22, 2018

Codecov Report

Merging #705 into master will increase coverage by 1.67%.
The diff coverage is 51.57%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #705      +/-   ##
============================================
+ Coverage     61.81%   63.48%   +1.67%     
- Complexity      827      862      +35     
============================================
  Files           154      154              
  Lines          2891     2977      +86     
  Branches        281      288       +7     
============================================
+ Hits           1787     1890     +103     
+ Misses          996      988       -8     
+ Partials        108       99       -9
Impacted Files Coverage Δ Complexity Δ
.../main/java/com/codeborne/selenide/Screenshots.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../codeborne/selenide/impl/ScreenShotLaboratory.java 59.09% <52.68%> (+3.02%) 42 <15> (+8) ⬆️
...e/selenide/impl/WebDriverThreadLocalContainer.java 80.88% <0%> (-0.74%) 30% <0%> (ø)
...ava/com/codeborne/selenide/commands/GetParent.java 100% <0%> (ø) 3% <0%> (+1%) ⬆️
...a/com/codeborne/selenide/commands/SelectRadio.java 100% <0%> (ø) 6% <0%> (+1%) ⬆️
...deborne/selenide/commands/SelectOptionByValue.java 100% <0%> (ø) 5% <0%> (+1%) ⬆️
...e/selenide/commands/SelectOptionByTextOrIndex.java 100% <0%> (ø) 8% <0%> (+1%) ⬆️
.../java/com/codeborne/selenide/commands/GetText.java 100% <0%> (ø) 4% <0%> (+1%) ⬆️
...a/com/codeborne/selenide/commands/SetSelected.java 95.65% <0%> (+0.91%) 10% <0%> (+1%) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c98a7a...f4e91e4. Read the comment docs.

@rosolko rosolko added the question label Mar 23, 2018

@rosolko

This comment has been minimized.

Collaborator

rosolko commented Mar 23, 2018

@andrejska ping!

@rosolko rosolko requested review from asolntsev and removed request for asolntsev Apr 1, 2018

Andrejs Kalnačs
Changes according to MR:
Added tests
Added same behavior as for existing screenshoting functionality in case if element doesn't exist inside iframe
Now taking into account vertical scroll bar for partially visible elements
@andrejska

This comment has been minimized.

Contributor

andrejska commented Apr 5, 2018

@rosolko

For first why don't you cover this feature with tests?

Tests added

For second wht don't you reuse exist method for takeScreenshotAsImage(WebElement element) with minimal extend for frame switch?

Difference between taking screenshot of element inside iframe and regular element screenshot is big, so reusability of this method will just add more complexity

Also isn't it work for you just take screenshot of frame?

For my specific use case I need a screenshot of webelement inside iframe

What behavior do you expect if iframe or element inside iframe doesn't exist?

Behavior implemented, warning message and returning null, similar approach as in takeScreenshotAsImage

@andrejska

This comment has been minimized.

Contributor

andrejska commented Apr 9, 2018

@rosolko can you please check?

@rosolko

This comment has been minimized.

Collaborator

rosolko commented Apr 11, 2018

@andrejska Hey, looks better now, but unfortunately you didn't add any tests for new command takeScreenShot that return File, that why coverage still failing.

Also I want to clarify why method signature takes WebElement element for first and then WebElement iframe, from my point of view - more semantic flow is iframe for first and element for second, because you look up element inside iframe and before all you switch to iframe, and work with it first.

Also I'm not sure about such complex method takeScreenshotAsImage that return BufferedImage, I'll take a look at them soon.
But anyway, thanks for PR.

Andrejs Kalnačs
@andrejska

This comment has been minimized.

Contributor

andrejska commented Apr 12, 2018

@rosolko did changes according to your latest comment, added missing tests & semantic change in order of parameters, please check

@rosolko

This comment has been minimized.

Collaborator

rosolko commented Apr 12, 2018

@andrejska LGTM.
@asolntsev @BorisOsipov Any comments?

@BorisOsipov

This comment has been minimized.

Member

BorisOsipov commented Apr 12, 2018

Also I'm not sure about such complex method takeScreenshotAsImage that return BufferedImage, I'll take a look at them soon.

+1 It looks nasty.

No other objections.

@BorisOsipov BorisOsipov self-requested a review Apr 12, 2018

@andrejska

This comment has been minimized.

Contributor

andrejska commented Apr 18, 2018

@rosolko, @BorisOsipop what are next steps :)?

@rosolko

This comment has been minimized.

Collaborator

rosolko commented Apr 18, 2018

@andrejska Sorry for long response. I'll take a look at this PR today and if I can't decrease complexity of method fastly - I'll merge it.

Don't use star import.
Style code.

@rosolko rosolko merged commit 536e41d into selenide:master Apr 18, 2018

3 of 4 checks passed

codecov/patch 51.57% of diff hit (target 61.81%)
Details
codecov/project 63.48% (+1.67%) compared to 1c98a7a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.3%) to 66.812%
Details

@asolntsev asolntsev added this to the 4.11.2 milestone Apr 19, 2018

@asolntsev asolntsev self-assigned this Apr 19, 2018

@asolntsev

This comment has been minimized.

Contributor

asolntsev commented Apr 19, 2018

@andrejska @rosolko Thanks for the PR! Merged to Selenide 4.11.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment