fix(ads): height property was not taking effect on client side ads #3284

[mariocynicys]

Description

controls were covering the SkipAd button.
this commit lifts up the shaka-client-side-ad-container.
also the shaka-server-side-ad-container was overflowing outside shaka-controls-container, now its shrinkable.

refer to #3284

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
• This change requires a documentation update

Checklist:

• I have signed the Google CLA https://cla.developers.google.com
• My code follows the style guidelines of this project
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• I have verified my change on multiple browsers on different platforms
• I have run ./build/all.py and the build passes
• I have run ./build/test.py and all tests pass

[ismena]

You need to string this with the server-side-container, so it's applied to both of them.
I think, it's done like this, but please double check with less.js documentation, I haven't worked on less for a while:

.shaka-server-side-ad-container .shaka-client-side-ad-container {
  iframe {/* iframe settings here */}
}

[mariocynicys]

like this

.shaka-server-side-ad-container, .shaka-client-side-ad-container {
  iframe {/* iframe settings here */}
}

however ,i found that the server-side-ad-container is a flex item inside shaka-controls so it never interfere with the shaka-bottom-controls, so no need to lift it up.
am i missing something?

[mariocynicys]

A demonstration of what iam talking about.

i though it should be lifted up like the client-side-ad, but turns out it already flexes with bottom-controls-container

[ismena]

Man, I hate UI work((
Okay. I went and checked. We don't have an example of a skippable server-side ad, but looking at the way the IMA SDK adds their UI on the client side, they align it directly over the video element using position: absolute. So, essentially, they don't care whether the container is lifted, and we still have to lift the i-frame.

I think it's a safe assumption that they do this with server-side ads as well, so please lift the iframe on the server-side container too.

[mariocynicys]

sensible 👍

[ismena]

Looks good. Running the build bot now.

[shaka-bot]

Test Failure:

Generating Closure dependencies...
Linting JavaScript...
Linting CSS...

ui/less/ad_controls.less
33:34  ✖  Expected selector ".shaka-server-side-ad-container" to come before selector ".shaka-server-side-ad-container:not([ad-active="true"])"   no-descending-specificity

END-BUILD: FAILURE
Build step 'Execute shell' marked build as failure

[ismena]

Re: lint failure - try moving your change above the first server-side-ad-container block, then run build/all.py to see if that resolves the problem. If not, you might have to turn the iframe settings into a mixin and apply it to both server-side and client-side containers.

[shaka-bot]

All tests passed!

[ismena]

Merging now. Thanks for contributing!