FDC3 icon indicating copy to clipboard operation
FDC3 copied to clipboard

IFramePositioning

Open robmoffat opened this issue 1 year ago • 19 comments

In the WebConnectionProtocol3HandshakePayload I can see fields for the intent resolver and channel selector URLs. However, there doesn't appear to be a way for the desktop agent to tell the channel selector what position and size it should occupy on the screen at startup.

In the POC I used the following:

export interface CSSPositioning { [key: string]: string }

export const CSS_ELEMENTS = ["width",
    "height",
    "position",
    "zIndex",
    "left",
    "right",
    "top",
    "bottom",
    "transition",
    "maxHeight",
    "maxWidth"]

export type ChannelSelectorDetails = {
    uri?: string,
    expandedCss?: CSSPositioning,
    collapsedCss?: CSSPositioning
}

export type IntentResolverDetails = {
    uri?: string,
    css?: CSSPositioning
}

robmoffat avatar Jul 31 '24 15:07 robmoffat

I don't see anywhere that we're handling the location info coming from the DA at the moment. It probably needs to be added - although it could go into SessionSotrage instead...

Take a look at iFrameChannels (which is the message intended to send the data onto the channelSelector UI along with the current selection and channel metadata) for comparison: https://github.com/finos/FDC3/blob/69a4c08316077f4ffabf548b6e08ff8416143ecb/schemas/api/iFrameChannels.schema.json#L35-L57

Note that we're only sending the x and y coord at the moment (left and top), presumably the different selector implementations will have different sizes, so we kept things to the top-left corner only there... Its that or make the iframe set-up an exchange with the implementation sending back its sizing details? iFrameResize handles that on resize, so could also do so after iFrameChannels is handled: https://github.com/finos/FDC3/blob/69a4c08316077f4ffabf548b6e08ff8416143ecb/schemas/api/iFrameChannelResize.schema.json#L23-L57 But it would seem 'less chatty' to add that info to iFrameHandshake message..

So... I guess the message flow could be something like:

  1. Receive the URL in WCP3Handshake
  2. Setup the iframe with display: none in a default location and listen for it loading
  3. Handle WCP4ValidateAppIdentity and WCP5ValidateAppIdentityResponse (other conversation on that notwithstanding) - WCP5ValidateAppIdentityResponse could contain a saved location... OR Saved location could be in SessionStorage data...
  4. Send iFrameHello to iFrame
  5. Its sends iFrameHandshake back with its default sizing and positioning details (tba) and MessagePort (if we don't update iFrameHandshake, it sends and additional iFrameResize message to indicate its defaults)
  6. Send iFrameChannels to the to the iframe with Channels, selected channel and location data.
  7. Make the selector iframe visible

Also @julianna-ciq for feedback as you've implemented this.

kriswest avatar Jul 31 '24 16:07 kriswest

FYI I can make changes today or tomorrow before I disappear on vacation (sans laptop)... Hence, if anyone can have a think on this one and let me know ASAP I can try and get that done ;-)

kriswest avatar Aug 01 '24 14:08 kriswest

I didn't get a chance to bottom this out before your holiday, but here's how I think we should approach this.

  1. IFrameHello this gets sent from the iframe to the app, along with a message port. We need to do it in this direction otherwise there's a race condition.

  2. IFrameHandshake is therefore sent from the app back to the iframe via the messageport. This is just an indication from the app that it is ready to receive things through the message port.

  3. IFrameRestyle: sent from either iframe to the app, asking for css styles to be set on the iframe. We could use these ones:

export const CSS_ELEMENTS = ["width",
    "height",
    "position",
    "zIndex",
    "left",
    "right",
    "top",
    "bottom",
    "transition",
    "maxHeight",
    "maxWidth"]

... however I think we also need to add display since we're going to need to show/hide the resolver at times. It might be worth just going through the list of css styles you can apply to an iframe and seeing which ones might be relevant.

  1. This means we don't need drag/resize/position messages - all of this will be handled by the iframe if it wants to do this. I think actually implementing drag could be really really complicated when we have zoom / scaling / window sizing to consider so this is a way of leaving that complexity up to the implementor rather than doing something limiting in the standard.

  2. Because of (3) above, we don't need positioning details in IFrameChannels either.

I'm going to have a go at updating my implementation along these lines today

robmoffat avatar Aug 27 '24 12:08 robmoffat

I'm not convinced that you don't need a drag message. From inside the iframe you don't know your current location, but you do know how it needs to change due to a drag (i.e. the offset to apply). That offset then needs applying to calculate a new position and capping at the edge of the window etc. (drag continues, movement does not).

Were you to try and handle it like this you would also need messages from the DA about saved position (3 does not eliminate that) and from the client/app window about its dimensions, which the UI would need to do a load of reasoning about to calculate the styles to set. Thats far from ideal - whereas with the drag approach, you are sending offsets from the dragging while the client code works out the end position and can then ask the DA to persist that info, or it can persist it locally in SessionStorage. In the end you will need a combination of things I think:

  • Initial/default sizes and position from the UI
  • Saved size/position from the DA or SessionStorage
  • Drag and Resize messages for moving the UI and popping it open/shut/otherwise resizing, with handling in window to keep it within the viewport.
  • Persistence of revised position.

kriswest avatar Aug 28 '24 11:08 kriswest

We might need to come back to it. With a proper proof-of-concept. I think this could eat up several months of development time on its own when you consider different browsers, tabs vs. iframes, different css styling that could be applied on the main document, window zoom, accessibility... I don't think we can just put our finger in the air and come up with the right answer to this, unless it's already been done somewhere.

The things from my message above we definitely do need either way.

robmoffat avatar Aug 28 '24 14:08 robmoffat

The dragging came from a StackOverflow with a nice working example thats easy to test cross-browser and @julianna-ciq had a successful play with an example I believe. The examples are old and simple with no real cross-browser concerns:

  • https://blog.crimx.com/2017/04/06/position-and-drag-iframe-en/
  • https://gsap.com/community/forums/topic/11271-iframe-in-draggable/

Tabs vs. iframes also have no bearing as it's purely a relationship between one DOM (whereever it is) and a single iframe within it. Styling should be heavily namespaced to avoid conflicts. Accessibility is a concern for the individual UI implementation within the window.

I think we should try for a workable solution and then refine it.

kriswest avatar Aug 28 '24 14:08 kriswest

Ok I'll put it on the stack. In the meantime, are you good with the above changes?

robmoffat avatar Aug 28 '24 14:08 robmoffat

Not entirely, as I said I don't think its workable that way round for positioning, but otherwise yes. In particular, I'm fine with the iframe sending an outward message first and to include an initial position and styling to set (although we should probably apply some limits one day in the future), but then drag needs to stay as is based on offsets. The DA should update the styling using the offsets from dragging and a resize message for open and close. I.e. somewhere between the two. I'll take a look at the relevant messages tomorrow and see what I can come up with before the meeting. That work for you?

kriswest avatar Aug 28 '24 14:08 kriswest

I think we can defer persisting anything for simplicity. Start by focusing on showing, styling and moving the things!

kriswest avatar Aug 28 '24 14:08 kriswest

@robmoffat @julianna-ciq I've made adjustments to the iframe messages in 6bb5699460019eea433877ee359a1949c360e940 Heres the summary:

  • Flipped Hello and Handshake messages, the UI now sends hello and the DA proxy setup by getAgent() responds with handshake
  • Added initial CSS to set on iframe to Hello message from iframe
  • Converted resize message into a restyle message that provides updated CSS (same properties as in hello)
  • Adjusted drag message and made it generic (you might want to drag an intent resolver in addition to a channel selector)
  • Removed location from iFrameChannels as it now duplicates the info in hello

kriswest avatar Aug 29 '24 10:08 kriswest

OK I'll take a look thanks!

btw I think I can implement the drag functionality with just my IFrameRestyle message - no need for anything else. I've got a meeting now otherwise I'd have a demo ready for our web meeting.

robmoffat avatar Aug 29 '24 12:08 robmoffat

I don't think you can or should use restyle to implement dragging - the iframe doesn't know its own coordinates in the window, nor the bounds of the window. It would be better to follow all the other examples of draggable iframes and send offsets to the parent window which can handle the move and keeping the widget within the viewport.

kriswest avatar Aug 29 '24 12:08 kriswest

@robmoffat Adjusted the messages as discussed yesterday in: abcc5776f4908ac343f170990e19789d7410736b

kriswest avatar Sep 04 '24 10:09 kriswest

Just pulling in your changes - there's still an issue about required properties. https://github.com/finos/FDC3/pull/1191#discussion_r1756688557

I think this is trying to be too clever - we should allow people to set the properties they want. for example, if you set right, you might not want to also set left.

robmoffat avatar Sep 12 '24 11:09 robmoffat

Based on our offline discussion about this @robmoffat @julianna-ciq I still think some CSS properties should be required in the iframeHello message (so that a UI implementation is driving its display size and position). As it stands the initialCSS object is required, but non of its subproperties - that may be enough... We could require one of several possible combos in the JSON schema (e.g. width + height + top + left / width+ height + bottom+ right / top + left + bottom + right etc.) and document that, but the typescript generated from that may make all the properties optional...

Hence, we may be ok here and can close this, or I make some small schema changes to require one of several minimal combos...

I remain of the opinion we don't need a position property as position: fixed should always be applied to the UIs by getAgent().

let me know your thoughts on closing this out.

kriswest avatar Sep 18 '24 13:09 kriswest

  • I would prefer the getAgent() to automatically apply position: fixed to the iframe containers.
  • I think it makes sense to require width and height, but I'm not dedicated to that.
  • I think requiring oneOf top | bottom AND oneOf left | right gets complicated. I understand that it's not going to look great if you don't assign those properties, but you can at least still see the iframe as long as width and height are applied. And juggling weird requirements like this sounds like more effort than it's worth.

Just my two cents

julianna-ciq avatar Sep 18 '24 17:09 julianna-ciq

@julianna-ciq 100% agree on position: fixed, we already took it out of the properties as it has to be set for all implementations, or we could be injecting things into the document flow and that would not be right.

I think your other suggestions are a good compromise - require at least width/height.

@robmoffat you ok with that?

kriswest avatar Sep 18 '24 17:09 kriswest

💯 on the position: fixed

However, I'm already doing this for the intent resolver in the demo:

const DEFAULT_EXPANDED_CSS = {
    'z-index': 1000,
    left: "10%",
    top: "10%",
    right: "10%",
    bottom: "10%"
}

robmoffat avatar Sep 19 '24 08:09 robmoffat

Leaving it without required properties.

kriswest avatar Sep 19 '24 10:09 kriswest