jpsantosbh commented 1 month ago

Description

Adds and maintain HTML div for each participant in the call using a well known id patterns. This allow developers to add participant specifics UI elements on top of the participant video.

It's also making the local video overlay more reliable, reducing the e2e tests flakiness.

Type of change

[ ] Internal refactoring
[x] Bug fix (bugfix - non-breaking)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Code snippets

<div id="rootElement" style="width: 100%; height: 100%;">
    <div class="mcuContent"
        style="position: relative; width: 100%; height: 100%; margin: 0px auto; display: flex; align-items: center; justify-content: center;">
        <div class="paddingWrapper" style="position: relative; width: 100%; padding-bottom: 56.25%;">
            <div style="position: absolute; inset: 0px;"><video autoplay="" playsinline=""
                    style="width: 100%; max-height: 100%;"></video></div>
            <div class="mcuLayers" style="display: block;">
                <div id="sw-sdk-60c0df92-d622-4aee-90f2-ac5eb1adf6cc"
                    style="position: absolute; z-index: 10; top: 50%; left: 50%; width: 50%; height: 50%;"><video
                        autoplay="" playsinline="" disablepictureinpicture=""
                        style="width: 100%; height: 100%; pointer-events: none; object-fit: cover; transform: scale(1, 1);"></video>
                </div>
                <div id=""
                    style="position: absolute; overflow: hidden; top: 0%; left: 0%; width: 100%; height: 100%; z-index: 20;">
                </div>
                <div id="sw-56c0eff9-d0d3-48ff-bb98-72d195e675fe-overlay"
                    style="position: absolute; overflow: hidden; top: 50%; left: 50%; width: 50%; height: 50%; z-index: 20;">
                </div>
                <div id="sw-40f809dc-949c-449f-a123-7621aab8b12e-overlay"
                    style="position: absolute; overflow: hidden; top: 50%; left: 0%; width: 50%; height: 50%; z-index: 20;">
                </div>
                <div id="sw-0cb6bb7b-e547-4fc0-bba0-dd4ff64ba95d-overlay"
                    style="position: absolute; overflow: hidden; top: 0%; left: 50%; width: 50%; height: 50%; z-index: 20;">
                </div>
                <div id="sw-f9c559e9-3ec8-46be-853d-6ec428fb2495-overlay"
                    style="position: absolute; overflow: hidden; top: 0%; left: 0%; width: 50%; height: 50%; z-index: 20;">
                </div>
            </div>
        </div>
    </div>
</div>

In case of new feature or breaking changes, please include code snippets.

changeset-bot[bot] commented 1 month ago

⚠️ No Changeset found

Latest commit: d0b1de5c45492be2423abcdf03a89e0a97e2e578

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

iAmmar7 commented 1 month ago

I was highlighting this issue in the meeting, the self-member layer is overflowing from the bottom.

--DELETED IMAGE--

Also, from the DOM, it seems we are adding two (one on top of another) layers for the self member.

jpsantosbh commented 1 month ago

I was highlighting this issue in the meeting, the self-member layer is overflowing from the bottom. Also, from the DOM, it seems we are adding two (one on top of another) layers for the self member.

The 2 layers are originally intended...

<div  id="sw-2bd1327a-9ec0-459b-b32b-caed8d21aeac-overlay" style="position: absolute; overflow: hidden; top: 50%; left: 50%; width: 50%; height: 50%;">
               <div id="sw-sdk-0e6f4ff8-e982-460a-890f-5912420b6e1d">
                    <video autoplay="" playsinline="" style="width: 100%; height: 100%; pointer-events: none; object-fit: cover; transform: scale(1, 1);"></video>
                </div>
</div>

We have the sw-ID-overlay wich defines the participant area even for self, and inside that when appropriated the SDK also adds the sw-sdk-ID with the local video overlay.

I'll check the overflow issue... thanks

iAmmar7 commented 1 month ago

We have the sw-ID-overlay wich defines the participant area even for self, and inside that when appropriated the SDK also adds the sw-sdk-ID with the local video overlay.

I like the idea of having two layers on top of the self member since both layers have separate requirements. However, I think we should not be nesting these because the developer will access the layer using the sw-ID-overlay to update the UI. If there is something nested inside already, it would mess up the DOM.

jpsantosbh commented 1 month ago

We have the sw-ID-overlay wich defines the participant area even for self, and inside that when appropriated the SDK also adds the sw-sdk-ID with the local video overlay.

I like the idea of having two layers on top of the self member since both layers have separate requirements. However, I think we should not be nesting these because the developer will access the layer using the sw-ID-overlay to update the UI. If there is something nested inside already, it would mess up the DOM.

I was thinking about that too... Yes, I agree we should keep the overlay

empty. I'm going to change that

iAmmar7 commented 1 month ago

I tried to simplify the whole logic and came up with something like this:

// ...the videoElement.ts file code till line 177
      myLayer.style.opacity = hasVideo ? '1' : '0'
      _updateLayer({ location, element: myLayer })

      // Make overlay for all members (including a self member)
      if (applyMemberOverlay && mcuLayers) {
        const layerWithMember = layers.filter(
          (location) => !!location.member_id
        )
        layerWithMember.forEach((location) => {
          const memberLayerId = `sw-overlay-${location.member_id}`

          let memberLayer = document.getElementById(memberLayerId)

          // If the layer already exists, modify its styles
          if (memberLayer) {
            _updateLayer({ location, element: memberLayer })
          } else {
            // If the layer doesn't exist, create a new one
            memberLayer = _buildLayer({ location })
            memberLayer.id = memberLayerId
            memberLayer.style.zIndex = '20'
            mcuLayers.appendChild(memberLayer)
          }
        })
      }

Points I considered;

Minimum updates
Changes are based on the flag applyMemberOverlay so that it is a non-breaking change.
Handle the overlay for local video and for design separately.

Something more I would like to achieve is;

The user should be able to get the member overlay even if he does not want the local video overlay. Currently, if he passes the applyLocalVideoOverlay set to false, the SDK won't render anything.
Set each layer not just the local video overlay inside the LayerMap.
Expose it through the CallFabricRoomSession (if possible).

jpsantosbh commented 1 month ago

I tried to simplify the whole logic and came up with something like this:
// ...the videoElement.ts file code till line 177
      myLayer.style.opacity = hasVideo ? '1' : '0'
      _updateLayer({ location, element: myLayer })

      // Make overlay for all members (including a self member)
      if (applyMemberOverlay && mcuLayers) {
        const layerWithMember = layers.filter(
          (location) => !!location.member_id
        )
        layerWithMember.forEach((location) => {
          const memberLayerId = `sw-overlay-${location.member_id}`

          let memberLayer = document.getElementById(memberLayerId)

          // If the layer already exists, modify its styles
          if (memberLayer) {
            _updateLayer({ location, element: memberLayer })
          } else {
            // If the layer doesn't exist, create a new one
            memberLayer = _buildLayer({ location })
            memberLayer.id = memberLayerId
            memberLayer.style.zIndex = '20'
            mcuLayers.appendChild(memberLayer)
          }
        })
      }
Points I considered;

Minimum updates

Changes are based on the flag applyMemberOverlay so that it is a non-breaking change.

Handle the overlay for local video and for design separately.

Something more I would like to achieve is;

The user should be able to get the member overlay even if he does not want the local video overlay. Currently, if he passes the applyLocalVideoOverlay set to false, the SDK won't render anything.

Set each layer not just the local video overlay inside the LayerMap.

Expose it through the CallFabricRoomSession (if possible).

Is this snippet on top of my branch or the main branch?

In general, I agree with the points you talked about. I am struggling to fit the proposed snippet in either branch.

iAmmar7 commented 1 month ago

In general, I agree with the points you talked about. I am struggling to fit the proposed snippet in either branch.

It was just an idea on how can we implement this without any breaking change. Let me share the diff with you. The changes are in the videoElement.ts file on the main branch. Let me know if that works for you or if you see any issues with that.

jpsantosbh commented 1 month ago

I'm closing this one in favor of https://github.com/signalwire/signalwire-js/pull/1133

signalwire / signalwire-js

Add member overlay area #1126

Description

Type of change

Code snippets

⚠️ No Changeset found