Skip to content

Conversation

@suhaibabsi-inst
Copy link
Contributor

@suhaibabsi-inst suhaibabsi-inst commented Dec 22, 2025

refs: MBL-19574
affects: Student, Teacher, Parent
builds: Student, Teacher, Parent
release note: Enhanced experience for media embeds

Changes

This is mainly to support the following functions related to iframes of Studio media:

1- Adding javascript file (CanvasLTIPostMessageHandler.js) provided by Studio team, to fix an issue of resizing for content loaded into WebView as an HTML text.
2- Removing the feature of inserting "Open in Details View" button below iframes for embeds types. See ticket description for more information about this decision.
3- Support launching immersive view for Studio media, by two ways: Tapping on "Expand View" button (of menu item) which loaded into the iframe, and tapping on the iframe itself (only for the case thumbnail embed).

The handling is done by 2 main ways:

1- Examining URL of navigation action passed to WebView delegate method: webView(_:decidePolicyFor:decisionHandler:)
2- Handling a WebView message fullWindowLaunch which is posted in CanvasLTIPostMessageHandler.js:298 upon listening to iframe requestFullWindowLaunch message event.

For more details on the specs of those frames, see document linked in ticket description.

Test Plan

  • Create a page with a couple of videos embedded through Studio LTI.
  • In iOS app, on page appearance, tapping on "Expand view" button must also trigger modal screen of immersive view on all Studio & Canvas Upload embed types.
  • For thumbnail embeds, tapping on video thumbnail image must also show modal screen of immersive view.
  • Make sure upon switching feature flag "RCE Studio Embed improvements" on/off, "Open in Details View" buttons are no longer inserted below iframes of those embeds.

Checklist

  • Follow-up e2e test ticket created
  • A11y checked
  • Tested on phone
  • Tested on tablet
  • Tested in dark mode
  • Tested in light mode
  • Approve from product

refs: MBL-19574
affects: Student, Teacher, Parent
builds: Student, Teacher, Parent
release note: Enhanced experience for media embeds
@suhaibabsi-inst suhaibabsi-inst self-assigned this Dec 22, 2025
@suhaibabsi-inst suhaibabsi-inst changed the title Restore Previous changed for Studio embeds [MBL-19574][S/T/P] Support Studio, Quiz & Archive embeds in WebView & Immersive view Dec 22, 2025
@inst-danger
Copy link
Contributor

inst-danger commented Dec 22, 2025

Warnings
⚠️ One or more files are below the minimum test coverage 50%

Release Note:

Enhanced experience for media embeds

Affected Apps: Student, Teacher, Parent

Builds: Student, Teacher, Parent

MBL-19574

Coverage New % Master % Delta
Canvas iOS 91.32% 81.06% 10.25%
Core/Core/Common/Extensions/PDFKit/PDFDocumentExtensions.swift 0% -- --

Generated by 🚫 dangerJS against af316b3

@inst-danger
Copy link
Contributor

inst-danger commented Dec 22, 2025

Builds

Commit: Remove feature for inserting "Open in details view" buttons below iframes (af316b3)
Build Number: 1135
Built At: Jan 13 21:01 CET (01/13 01:01 PM MST)

Student
Teacher
Parent

@instructure instructure deleted a comment from claude bot Dec 25, 2025
@claude
Copy link

claude bot commented Dec 28, 2025

Claude Code Review

Updated: 2025-12-28

Critical Issues Fixed ✅

  • Double URL encoding: RESOLVED - Swift now properly encodes URLs using URLQueryItem, eliminating the previous double-encoding issue.

Remaining Issues

  • Missing script file fallback: Bundle resource loading silently returns nil if files missing. No logging/assertions to detect early. Add os_log or assert().

  • Accumulated DOMContentLoaded listeners: Event listeners added without {once: true}. Accumulate on reloads, causing duplicate button injection execution.

  • URL parsing without validation: findCanvasUploadLink() and findLtiEmbedLink() call new URL() which throws on relative paths. Validate absolute URLs first.

Minor

  • Fragile duplicate detection uses == 1 instead of === true
  • Title sanitization only removes first .mp4 occurrence

Improvements ✅

  • JavaScript extracted to separate files (cleaner maintenance)
  • Script injection order explicit (insertValues → insertScript dependency clear)
  • LTI embed support added alongside Canvas uploads
  • New CanvasLTIPostMessageHandler for iframe resize functionality

✅ Approved with minor observations

@instructure instructure deleted a comment from claude bot Dec 28, 2025
@instructure instructure deleted a comment from claude bot Dec 28, 2025
@suhaibabsi-inst suhaibabsi-inst marked this pull request as ready for review December 28, 2025 08:29
lazy var optionsButton = UIBarButtonItem(image: .moreLine, style: .plain, target: self, action: #selector(showOptions))
@IBOutlet weak var webViewContainer: UIView!
public var webView = CoreWebView()
public var webView = CoreWebView(features: [.canvasLTIPostMessageHandler])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to include this feature by default in every CoreWebView instance we create just like we do with the .dynamicFontSize feature. This way we won't leave out a single screen.

CoreWebView.swift:97
let features = features + [.dynamicFontSize]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is meant for screens loading HTML content as text. (Through method loadHTMLString(_:String, baseURL:))
For those loading content through remote URL, wouldn't that interfere with referenced scripts in the loaded HTML page? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is meant for screens loading HTML content as text.

Was this a specific ask from the Studio team? Interference depends on how good the js we inject. I'd think the Studio team made it pretty specific to our use case.

@claude
Copy link

claude bot commented Jan 6, 2026

Claude Code Review

Updated: 2026-01-06

Critical Issues Found

  • JavaScript crash in URL parsing (scanFramesScript lines 48, 52): new URL(frameSource) and new URL(playerSource) throw if arguments are relative/malformed. No try-catch wrapper. Script execution will crash on invalid iframe src.

  • Unsafe webkit message posting (CanvasLTIPostMessageHandler.js line 298): webkit.messageHandlers.fullWindowLaunch.postMessage() has no existence check. Will crash if handler not registered on some contexts.

  • Silent script loading failure (CanvasLTIPostMessageHandler.swift lines 24-28): Bundle resource loading returns nil silently on missing file. No logging/assertions to detect. Degraded feature goes unnoticed.

Other Issues

  • Accumulated addEventListener('message') listeners without {once: true} - duplicate button injection on repeated page loads
  • Fragile selector .open_detail_button_container may fail silently if DOM structure changes

Summary

Review identifies 3 crash vectors and 2 reliability issues. URL parsing should use try-catch. Webkit handlers need guards. Script injection needs debug visibility.

❌ Issues found

@suhaibabsi-inst suhaibabsi-inst marked this pull request as draft January 13, 2026 09:46
@claude
Copy link

claude bot commented Jan 13, 2026

Claude Code Review

Updated: 2026-01-13

Issues Fixed Since Last Review ✅

  • URL parsing now wrapped in try-catch (InsertStudioOpenInDetailButtons.js:28-40, 48-74) - prevents crashes on malformed iframe sources
  • URL encoding uses proper Swift URLQueryItem approach (CoreWebViewStudioFeaturesInteractor.swift:174-186)

Remaining Issues

  • Silent script loading failure (CanvasLTIPostMessageHandler.swift:24-28): Bundle resource loading returns nil without logging if file missing. No visibility into failures.

  • Duplicate DOMContentLoaded listeners (InsertStudioOpenInDetailButtons.js:155): Listener added without {once: true}. Potential duplicate button injection on page reload.

  • Unguarded webkit message (CanvasLTIPostMessageHandler.js:298): No existence check before calling webkit.messageHandlers.fullWindowLaunch. Low-risk but could crash in edge cases.

Summary

Significant improvements in URL parsing safety. Main concerns: accumulated event listeners and silent script loading failures.

✅ Approved with minor observations

@suhaibabsi-inst suhaibabsi-inst marked this pull request as ready for review January 13, 2026 20:04
Copy link
Collaborator

@vargaat vargaat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a few issues that we could solve before merging

  1. Opening the immersive view from discussion details doesn't open the native dialog, but it opens in place.
    Image
  2. Rotating immersive view from portrait to landscape make the "Details" and "Transcript" tabs not to appear. The layout is also strange, scrollview inside scrollview.
    https://github.com/user-attachments/assets/6361f684-824e-4fbb-8b30-95764391daec
  3. The "X" button inside the immersive view does nothing. We could hide this with a CSS injection not to have two close buttons.
    https://github.com/user-attachments/assets/a0cf8565-7df7-44bc-a4d3-c95cb4cf087b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants