As per the UX, the standalone display should be showing both of the remote screen and video when screen sharing is active.

I'm going to take a stab at this.

This first part renames the existing classes from local-stream and remote-stream to focus-stream and inset-stream. I'm still not quite sure about the focus stream name. The elements still have .local and .remote on them as names. I'd prefer not to have things such as local-inset-stream and remote-inset-stream as that feels like duplicating classnames for no particular reason with our current UI. Still working on this and part 2, so not requesting review yet, but feedback welcome.

Draft of part 2 adding the "sub-inset" stream. It doesn't work properly yet as I need to do some more adjustments to the calculations and decide how we're going to cope with the sdk's minimum stream size values. However I think the structure is right now, so requesting feedback on that before I finish it off.

Comment on attachment 8563392 [details] [diff] [review] Part 1. Redefine the classnames for remote and local streams to focus and inset to reflect the real display better. Ok, I'm currently discussing a different method with Sevaan due to the local video being extremely small in the designed set-up. Hence cancelling feedback for now.

New version that works for the wider screens only (wider than 640px). To do smaller is going to be harder as it involves a lot more calculations as the DOM could do with being set up differently. I'm going to split that out to a separate bug. The remote and local video are stacked here - as per my discussions with Sevaan; doing a sub-inset makes it a bit too small to be useable/visable. I'll finish up the tests on Monday.

Updated with unit tests and to fix a couple of minor issues.

Comment on attachment 8564570 [details] [diff] [review] Loop Standalone should display both of the remote video and screen when screen sharing is active. Review of attachment 8564570 [details] [diff] [review]: ----------------------------------------------------------------- Nice one Mark! It seems to work fine when I tested it, I didn't see any incorrect behavior. You mentioned that you'll be making the inset-view smaller in a follow-up, right? With that bug filed and comments addressed, r=me. ::: browser/components/loop/content/shared/js/mixins.js @@ +276,5 @@ > * Note: Once we support multiple remote video streams, this function will > * need to be updated. > + * > + * @param {string} videoType The video type according to the sdk, e.g. "camera" or > + * "stream". ITYM 'screen' @@ +329,5 @@ > remoteVideoDimensions.streamHeight = leadingAxis === "width" ? > remoteVideoDimensions.height: leadingAxisSize; > } > } > + }; please remove this semicolon. ::: browser/components/loop/test/shared/mixins_test.js @@ +267,5 @@ > > it("should fetch the correct stream sizes for leading axis width and full", > function() { > remoteVideoDimensions = { > + stream: { ITYM 'screen' throughout this file... ::: browser/components/loop/test/standalone/standaloneRoomViews_test.js @@ +258,5 @@ > + style: {} > + }; > + remoteElement = { > + style: {}, > + removeAttribute: sinon.spy() I think it'd be nice to check if this function gets called indeed, for extra coverage.

(In reply to Mike de Boer [:mikedeboer] from comment #7) > You mentioned that you'll be making the inset-view smaller in a follow-up, > right? With that bug filed and comments addressed, r=me. I just filed bug 1133532 and bug 1133534 as follow-ups.

Verified as fixed using Firefox 38 beta 8 (after enabling the feature) on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit.