Media.net Adapter Improvements

[vedantseta]

Type of change

• Other

Description of change

This PR adds functionality adds slot visibility in request to bid optimally.

[snapwich]

I think you have a logical error in determining ad positioning for before or after the fold. You're using getBoundingClientRect which changes relative to the scroll position; this means your logic would have to take into account scroll position somewhere to accurately determine if you are before or after the fold.

From: https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect

The amount of scrolling that has been done of the viewport area (or any other scrollable element) is taken into account when computing the bounding rectangle. This means that the rectangle's boundary edges (top, left, bottom, and right) change their values every time the scrolling position changes (because their values are relative to the viewport and not absolute). If you need the bounding rectangle relative to the top-left corner of the document, just add the current scrolling position to the top and left properties (these can be obtained using window.scrollX and window.scrollY) to get a bounding rectangle which is independent from the current scrolling position.

It's possible the discrepancy wouldn't be terrible depending on how fast the library loaded how long it took before this viewability code ran (if it ran before the user had done any scrolling). Let me know if you want to update, otherwise I can merge.

[ruturajv]

Hey @snapwich ,

As you mentioned there are following cases

Slot is above/below the fold (the div hasn't taken its size in DOM)

Code currently assigns smallest size (by area) to the container, and figures out if its gonna be in view or not.

Slot is above/below the fold (user hasn't scrolled yet, and DIV/container loaded in DOM)

Correct visibility would be determined.

The user has already scrolled to some slot (making that slot in view)

Yes current code would make it "look" ABOVE_THE_FOLD, but then from the buyer's perspective, that impression really WAS visible, I'm sure as you said earlier it'll be a tiny fraction.

... on how fast the library loaded how long it took before this viewability code ran ...

I'm not quite sure I understood this point, wouldn't the entire lib load as one monolithic code and execute ? The execution of the slot visibility code is inside buildRequests call. Perhaps you could help us understand this part.

I would've loved if prebid's core lib could've given us slot visibility info. Happy to give a PR for that!

[snapwich]

Hi @ruturajv

So I'm guessing the intent is to determine if an element is above the fold considering the use of ABOVE_THE_FOLD and BELOW_THE_FOLD as variable names. In digital display above the fold usually refers to the viewable content available without scrolling. However, using getBoundingClientRect will give you an X an Y position to the top left of the screen, not the document. That means the way getCoordinates is used within getSlotVisibility in this code currently will determine whether or not the given element is within the current viewport, not whether it was above the fold; that may or may not be the same result (depending on whether the user has scrolled).

If you want these functions to always tell you whether the element was placed above the fold then getBoundingClientRect has to be normalized with the current scroll position, in the code getCoordinates would change to:

    coordinates.top_left = {
      y: rect.top + window.pageYOffset,
      x: rect.left + window.pageXOffset,
    };
    coordinates.bottom_right = {
      y: rect.bottom + window.pageYOffset,
      x: rect.right + window.pageXOffset,
    };

[snapwich]

To get the distance from the top left corner of the document (as opposed to viewport) this code should be

    coordinates.top_left = {
      y: rect.top + window.pageYOffset,
      x: rect.left + window.pageXOffset,
    };
    coordinates.bottom_right = {
      y: rect.bottom + window.pageYOffset,
      x: rect.right + window.pageXOffset,
    };

[snapwich]

A mock does not modify the given object.

Does not change the object, but returns a mock object to set expectations on the object’s methods.

This means that setting these read-only values on window will most likely throw errors (in Safari I believe) and also not be reset after your test suite runs. To solve this you can either use a helper function to get the screen dimensions and stub that to get the results you want or you can make your tests more tolerant of various screen sizes (as the browserstack tests will run across multiple devices).

[snapwich]

Needs to be updated as well not to modify read-only window properties.

[ruturajv]

Hey @snapwich ,

Thanks for review and clarification on sinon. Will get your changes integrated.

Ruturaj

[vedantseta]

@snapwich , We have updated the code as requested. Can you please have a look ?

Also we have exposed getWindowSize for required sinon changes , can you please suggest some better alternative if any?

[snapwich]

You should just put the sandbox creation in the outermost describe block's beforeEach and the restore in its afterEach. Doing this multiple times in different nested-levels of describe make it difficult to see if the sandbox is being properly cleaned up.

edit - just edited to say that they should be in a beforeEach and afterEach as well, as opposed to before and after which only run once per describe block.

[snapwich]

Since the documentStub was created with the sandbox it should be cleaned up by the sandbox automatically by the sandbox.restore() in the afterEach. Doing documentStub.restore() might double restore, which can do weird things.

[snapwich]

@vedantseta Exposing getWindowSize is fine. However, there are some other changes that need to be fixed in the tests.

[vedantseta]

@snapwich , We have made changes as requested. Can you please have a look?
Thanks