Skip to content
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

Using frames messes up the blur effect. #50

Closed
bioneyez opened this issue Feb 14, 2020 · 22 comments
Closed

Using frames messes up the blur effect. #50

bioneyez opened this issue Feb 14, 2020 · 22 comments

Comments

@bioneyez
Copy link
Contributor

Bug
When I try to blur an element out and the page contains frame elements then the blur is mislocated on the screenshot. For example If i have a 100px frame on the left side (menu) then on the main frame the blur effect is 100px to the left compared to the element I want to blur out.

To Reproduce
Steps to reproduce the behavior:
Try to blur out elements on a page containing frames and framesets.

Expected behavior
I expect that the specified element is blurred precisely.

Desktop (please complete the following information):

  • OS: Windows 10
  • Browser Chrome
  • Version 74
@jesse019
Copy link

@bioneyez Could you provide some more information for better understanding like a screenshot of the webpage with the element that you were trying to blur and the actual blur?

@bioneyez
Copy link
Contributor Author

bioneyez commented Feb 14, 2020

Capture

On the left side you can see a house icon (thats the menu but now its collapsed). I wanted to blur out the application number text input. As you can see the blur is misaligned. I tried it without collapsing the menu and in that case the blur is even more misaligned since then the menu frame takes more space from the page.

@jesse019
I found a possible solution for the matter. The problem is that the getBoundingClientRect method does not take the offset caused by the frames into account.
The script below calculates the offset that is caused by the other frames (It is just a POC, so please review it. Also since we're getting the frames by the frame tag, iframes could cause another issue).

function currentFrameAbsolutePosition() {
  let currentWindow = window;
  let currentParentWindow;
  let positions = [];
  let rect;
  while (currentWindow !== window.top) {
    currentParentWindow = currentWindow.parent;
    for (let idx = 0; idx < currentParentWindow.frames.length; idx++) {
      if (currentParentWindow.frames[idx] === currentWindow) {
        for (let frameElement of currentParentWindow.document.getElementsByTagName('frame')) {
          if (frameElement.contentWindow === currentWindow) {
            rect = frameElement.getBoundingClientRect();
            positions.push({x: rect.x, y: rect.y});
            console.log("x: " + rect.x + " y: " + rect.y)
          }
        }
        currentWindow = currentParentWindow;
        break;
      }
    }
  }

  return positions.reduce((accumulator, currentValue) => {
    return {
      x: accumulator.x + currentValue.x,
      y: accumulator.y + currentValue.y
    };
  }, { x: 0, y: 0 });
};

I think calculating the offset and adding it to the coordinates before blurring will resolve the issue.

@jessezach
Copy link
Owner

@bioneyez Thanks for posting a probable solution. Based on your investigation if I may ask, could you try modifying the source code to fetch coordinates using selenium driver? Currently the coordinates are fetched using javascript. Method _get_coordinates_from_driver(self, element) fetches coordinates from the driver. Im hoping that if the offset issue could be solved using selenium then there would not be any need for additional code to calculate offset based on frames in the page.

I would need a couples of days to look into this issue and get back to you. However feel free to investigate further and raise pull request if you figure out a proper solution

@bioneyez
Copy link
Contributor Author

@jz-jess I tried modifying the source code as you suggested, but the result is the same. Selenium driver returns the coordinates relative to the current frame, not the whole page. https://sqa.stackexchange.com/questions/32617/how-to-get-coordinates-of-element-inside-iframe

@jessezach
Copy link
Owner

@bioneyez Understood. I'd prefer that we fix the issue using python similar to the code in the link above.
I'm assuming now that this would be breaking even the capture element keyword.
I'm thinking we could add an argument in the capture keywords to take frame locators in a list and then modify the blur method to get location of frames using selenium and adding it to the elements location.
Since Im struggling to find time to work on fixing some current issues. It would be great if you would be able to to raise a pull request with a fix. I can fix it myself after a few days if it isn't very urgent.

@bioneyez
Copy link
Contributor Author

bioneyez commented Feb 17, 2020

@jz-jess I'm not sure we need to pass the frames as a list, since we can retrieve all the parent frames automatically. I would prefer python as well, but I think it's a struggle to get all the frame positions using selenium driver. Right now my only idea is to iterate through all the parent frames using switchTo and accumulate the offset, but then we need to switch back to the original frame. If you have a better idea please tell me. It's not urgent since I solved it locally using JS, and it works for now.

If I have the time I'll try to solve it in Python as well and make a pull a request.

@jessezach
Copy link
Owner

@bioneyez If you have implemented the JS code in the library and called it using selenium execute script, then I would like to take a look at it. You can raise a pull request

@bioneyez
Copy link
Contributor Author

@jz-jess Another issue came into my mind. What if we want to blur elements in different frames. Right now I think it's only possible to blur elements in the active frame.

@jessezach
Copy link
Owner

@bioneyez What if we call select frame keyword first and then call the capture element keyword?
If this doesnt work then support would have to be added :)

@bioneyez
Copy link
Contributor Author

bioneyez commented Feb 17, 2020

@jz-jess My problem with the above mentioned suggestion is that I think it should be possible to capture the full screen and just pass a list of elements that should be blurred regardless of which frame contains which element. For example by specifying some kind of pattern ex. "frameName:class=tail". I think its cumbersome to specify a list of elements for each frame the page contains and then capturing them individually. Let me know what you think about this.

@jessezach
Copy link
Owner

@bioneyez Let me try to put it another way.
The library is essentially just an extension to selenium. Now consider an example where you are writing a functional test.
To interact with an element within a frame, you would ideally switch to frame using selenium and then interact with the required element.
This is the behavior that this library would also ideally mimic.
The above step is only when you want to capture an element within a single frame.

However like you suggested when you would want to blur elements in the whole page, you would want the capture screen keyword itself to take care of all the frames present in the page and calculate offsets accordingly. This would require proper testing to ensure all cases are covered

@bioneyez
Copy link
Contributor Author

bioneyez commented Feb 17, 2020

Thats exactly how I imagined it.

@jessezach
Copy link
Owner

@bioneyez So we're on the same page then :)
Feel free to update your pull request with your fixes. Will release it as part of the upcoming release post review and testing

@bioneyez
Copy link
Contributor Author

bioneyez commented Feb 19, 2020

@jz-jess Can you help me out implementing the capture full screen functionality the way we agreed upon earlier? I tried it and its not that straightforward as I thought.

My concept was:

  1. Get the current frame
    initialFrame = self.driver.execute_script("return self.name")
  2. Switch back to the root content and get all the frames in the page
self.driver.switch_to_default_content()
frames = self.driver.find_elements_by_tag_name("frame")
  1. Iterate through all the frames and call the blur_regions on each of them.
for index, frame in enumerate(frames):
            self.driver.switch_to.frame(frame)
            currentFrame = self.driver.execute_script("return self.name")
            logger.console("Blurring in: " + str(currentFrame))
            self.blur_regions(blur, radius, path)
  1. Switch back to the initial frame
    self.driver.switch_to_frame(initialFrame)

Unfortunately it's not working, I get StaleElementReferenceException
Can you give me some advice how to move forward?

Concerns

  • frameset handling
  • nested frames

@jessezach
Copy link
Owner

jessezach commented Feb 19, 2020

@bioneyez StaleElementReferenceException commonly occurs when iterating over a list of loctors. In this case maybe frames. I think the state of DOM is changing in between the iterations which is why the error is thrown.

Before we get into the issue, in your current approach how do we get to know that the region we're trying to blur is within a particular frame?

@bioneyez
Copy link
Contributor Author

@jz-jess Right now I don't care about that. I'm trying to blur all the specified elements in all the frames. (later I plan to modify that but for now I think it's okay).
So for example if id=element is in both frame1 and frame2, then it should blur out that element in both of those frames.

@jessezach
Copy link
Owner

jessezach commented Feb 19, 2020

@bioneyez Looks like we are trying to find a non existent element within a frame.
https://stackoverflow.com/questions/53246587/slate-element-reference-exception-when-handling-frames

Would this be the reason?

@jessezach
Copy link
Owner

I think we need to switch to default_content within the loop after blurring, so we can switch to the next frame.

@bioneyez
Copy link
Contributor Author

Good point. Now it looks like its working as expected.
What do you think how should it work in case of capture full screen keyword?
Should we care about which frame contains the specified element or just blur out the specified element in every frame? If yes, how should it work? Adding some kind of identifier logic for example:
frame=frameName&class=elementClass (I'm open for suggestions) and if no frame is specified then the blur out in all the frame would be the default.

+1 TODO: make it work for iframes as well

@jessezach
Copy link
Owner

I think we could try blurring an element in every frame. We should catch NoSuchElement exception and continue incase element isn't present.
Keep it simple for the user.
In most cases there will not be frames present I assume. And above logic should take care of it if frames are present. This would require quite a bit of testing though :)

@bioneyez
Copy link
Contributor Author

I pushed the solution to the existing pull request,
Extensive testing will be required since I've only managed to do a little bit. I hope everything is ok though.

@jessezach
Copy link
Owner

@bioneyez This has been released in v1.3.2. There were a couple of bugs in your pull request. However they were minor and I was able to fix them. I was able to test it against demo pages with iframes.
Thanks for the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants