Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IFramePositioning #1302

Closed
robmoffat opened this issue Jul 31, 2024 · 19 comments · May be fixed by #1309
Closed

IFramePositioning #1302

robmoffat opened this issue Jul 31, 2024 · 19 comments · May be fixed by #1309
Assignees
Milestone

Comments

@robmoffat
Copy link
Member

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 robmoffat added the bug Something isn't working label Jul 31, 2024
@kriswest
Copy link
Contributor

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:

"selected": {
"title": "Selected Channel",
"description": "The id of the channel that should be currently selected, or `null` if none should be selected",
"oneOf": [
{"type": "string"}, {"type": "null"}
]
},
"location": {
"title": "Location",
"description": "If the channel selector was previously displayed in this window its location may be restored by setting the location coordinates",
"type": "object",
"properties": {
"x": {
"type": "integer"
},
"y": {
"type": "integer"
}
},
"required": ["x", "y"],
"additionalProperties": false
}
},

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:

"payload": {
"title": "iframeChannelResize Payload",
"type": "object",
"properties": {
"dimensions": {
"title": "Dimensions",
"description": "The updated dimensions of the UI",
"type": "object",
"properties": {
"width": {
"type": "integer"
},
"height": {
"type": "integer"
}
},
"required": ["width", "height"],
"additionalProperties": false
},
"resizeAnchor": {
"title": "Resizing",
"description": "When resizing anchor at the indicated location,\ne.g.\n\t- for top-left and a larger size: the bottom right corner should move down and out.\n\t- for top and smaller size: both the bottom corners should move in and up.",
"anyOf": [
{ "type": "string", "const": "top-left" },
{ "type": "string", "const": "top" },
{ "type": "string", "const": "top-right" },
{ "type": "string", "const": "right" },
{ "type": "string", "const": "bottom-right" },
{ "type": "string", "const": "bottom" },
{ "type": "string", "const": "bottom-left" },
{ "type": "string", "const": "left" },
{ "type": "string", "const": "center" }
]
}
},

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
Copy link
Contributor

kriswest commented Aug 1, 2024

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 ;-)

@robmoffat
Copy link
Member Author

robmoffat commented Aug 27, 2024

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

@kriswest
Copy link
Contributor

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.

@robmoffat
Copy link
Member Author

robmoffat commented Aug 28, 2024

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.

@kriswest
Copy link
Contributor

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:

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.

@robmoffat
Copy link
Member Author

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

@kriswest
Copy link
Contributor

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
Copy link
Contributor

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

@kriswest
Copy link
Contributor

@robmoffat @julianna-ciq I've made adjustments to the iframe messages in 6bb5699
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

@robmoffat
Copy link
Member Author

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.

@kriswest
Copy link
Contributor

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
Copy link
Contributor

kriswest commented Sep 4, 2024

@robmoffat Adjusted the messages as discussed yesterday in: abcc577

@robmoffat
Copy link
Member Author

robmoffat commented Sep 12, 2024

Just pulling in your changes - there's still an issue about required properties. #1191 (comment)

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.

@kriswest
Copy link
Contributor

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.

@julianna-ciq
Copy link
Contributor

  • 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

@kriswest
Copy link
Contributor

@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?

@robmoffat
Copy link
Member Author

💯 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%"
}

@kriswest
Copy link
Contributor

Leaving it without required properties.

@kriswest kriswest added this to the 2.2 candidates milestone Sep 20, 2024
@kriswest kriswest removed the bug Something isn't working label Sep 20, 2024
robmoffat added a commit that referenced this issue Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants