Fix rendering on JupyterHub 4.1: respect `absolute_url` for protocol determination, support `data-absolute-url` #14003

krassowski · 2024-07-30T10:24:01Z

issues: fixes [BUG] Embedded server websocket cannot be used in srcdoc iframe #14002
- which together with Fix panel preview rendering on JupyterHub 4.1+ holoviz/pyviz_comms#125
- helps to fix Panel preview does not work on JupyterHub 4.1+ hosted on a single domain holoviz/panel#7039
tests passed (see below for manual testing instructions)
release document entry (if new feature or API change)

This PR:

fixes ws/wss determination when absolute_url is passed
supports reading the absolute URL from data-absolute-url of an iframe as srcdoc iframes cannot pass it otherwise (e.g. there is no motion of URL parameters for these)

Before	After

How to test without installing JupyterHub

Install pyviz_comms with Fix panel preview rendering on JupyterHub 4.1+ holoviz/pyviz_comms#125 applied
Run jupyter --paths and choose a writable config path from the config: section, e.g. /home/username/my-env/etc/jupyter

Open (create if needed) a /home/username/my-env/etc/jupyter/jupyter_server_config.py, paste the following:

 c.LabApp.tornado_settings = {
     'hub_prefix': '/hub',
     'hub_host': 'test',
     'user': 'test',
     'headers': {'Content-Security-Policy': "frame-ancestors 'none'"}
 }

Start jupyter lab
Open dev tools Network tab, click on a request and expand Response Headers, verify that CSP for frame ancestors is set to none:
Ensure that page data includes hubPrefix by viewing the source of JupyterLab page
Follow reproduction steps in Panel preview does not work on JupyterHub 4.1+ hosted on a single domain holoviz/panel#7039

codecov · 2024-07-30T10:32:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (branch-3.6@b902fb5). Learn more about missing BASE report.

Additional details and impacted files

@@              Coverage Diff              @@
##             branch-3.6   #14003   +/-   ##
=============================================
  Coverage              ?   93.11%           
=============================================
  Files                 ?      281           
  Lines                 ?    19828           
  Branches              ?        0           
=============================================
  Hits                  ?    18462           
  Misses                ?     1366           
  Partials              ?        0

mattpap · 2024-07-31T08:41:14Z

bokehjs/src/lib/embed/server.ts

+  if (absolute_url === undefined && frameElement !== null && (frameElement as HTMLIFrameElement).dataset.absoluteUrl !== undefined) {
+    absolute_url = (frameElement as HTMLIFrameElement).dataset.absoluteUrl


Let's remove all this casting with:

Suggested change

if (absolute_url === undefined && frameElement !== null && (frameElement as HTMLIFrameElement).dataset.absoluteUrl !== undefined) {

absolute_url = (frameElement as HTMLIFrameElement).dataset.absoluteUrl

if (absolute_url === undefined && frameElement instanceof HTMLElement && frameElement.dataset.absoluteUrl !== undefined) {

absolute_url = frameElement.dataset.absoluteUrl

}

or alternatively use HTMLIFrameElement instead of HTMLElement, though I'm not sure if we need to be that specific.

My thinking was the same here! But it does not work. This is because frameElement is an HTMLElement, but not window.HTMLElement. It is an HTMLElement from the context of the parent frame. I can make this work without type cast by checking for the right instance - which is just a bit more code - but frameElement by definition is an HTMLElement ("element" is in the name).

Do you still think I should modify it here or do you agree with the type cast here?

For example, using frameElement instanceof window.parent.HTMLElement can result in:

DOMException: Failed to read a named property 'HTMLElement' from 'Window': Blocked a frame with origin "http://localhost:5151" from accessing a cross-origin frame.

Resolved in c639a85

bokehjs/src/lib/embed/server.ts

Co-authored-by: Mateusz Paprocki <[email protected]>

mattpap · 2024-07-31T08:47:56Z

We need to be able to test this somehow, preferably outside of jupyter. Perhaps an example showing <iframe> embedding would suffice (under examples/server).

krassowski · 2024-07-31T11:15:43Z

@mattpap thank you for your review (dziękuję!) - I've added an example which both demonstrates the problem and a solution under examples/server/embed:

I also added a dedicated type guard to remove the explicit type cast (I do not like them either!).

bryevdv · 2024-07-31T16:13:36Z

examples/server should only contain subdirectories to match the docs structure, not individual examples. Also "embed" by itself is too general, there are lots of concepts about embedding. Possible suggestions:

examples/server/app/iframe_embed
somewhere under examples/integration

krassowski · 2024-07-31T17:08:38Z

Thank you @bryevdv, I moved it to examples/server/app/iframe_embed following your suggestion.

bryevdv · 2024-07-31T18:06:58Z

@mattpap will have to comment on the test screenshot error

ERROR: Path for Bokeh server application does not exist: /home/runner/work/bokeh/bokeh/bokeh_server.py
tests/test_examples.py::test_server_examples[/home/runner/work/bokeh/bokeh/examples/server/app/iframe_embed-example12-config12] 
[FAIL] Unhandled rejection at:  ProtocolError: Cannot take screenshot with 0 width.
    at /home/runner/work/bokeh/bokeh/bokehjs/node_modules/chrome-remote-interface/lib/chrome.js:94:35
    at Chrome._handleMessage (/home/runner/work/bokeh/bokeh/bokehjs/node_modules/chrome-remote-interface/lib/chrome.js:272:17)
    at WebSocket.<anonymous> (/home/runner/work/bokeh/bokeh/bokehjs/node_modules/chrome-remote-interface/lib/chrome.js:240:22)
    at WebSocket.emit (node:events:518:28)
    at Receiver.receiverOnMessage (/home/runner/work/bokeh/bokeh/bokehjs/node_modules/chrome-remote-interface/node_modules/ws/lib/websocket.js:1070:20)
    at Receiver.emit (node:events:518:28)
    at Receiver.dataMessage (/home/runner/work/bokeh/bokeh/bokehjs/node_modules/chrome-remote-interface/node_modules/ws/lib/receiver.js:517:14)
    at Receiver.getData (/home/runner/work/bokeh/bokeh/bokehjs/node_modules/chrome-remote-interface/node_modules/ws/lib/receiver.js:435:17)
    at Receiver.startLoop (/home/runner/work/bokeh/bokeh/bokehjs/node_modules/chrome-remote-interface/node_modules/ws/lib/receiver.js:143:22)
    at Receiver._write (/home/runner/work/bokeh/bokeh/bokehjs/node_modules/chrome-remote-interface/node_modules/ws/lib/receiver.js:78:10) {
  request: {
    method: 'Page.captureScreenshot',
    params: { format: 'png', clip: [Object] },
    sessionId: undefined
  },
  response: { code: -32000, message: 'Cannot take screenshot with 0 width.' }
}

FAILED

krassowski · 2024-08-01T07:43:46Z

Looking at:

bokeh/tests/examples.yaml

Lines 18 to 31 in 54252f8

    
           - path: "examples/output/apis/*" 
        
             skip: ["autoload_static.py", "autoload_static_flask.py", "components.py", "components_themed.py", "json_item.py", "json_item_themed.py", "server_document", "server_session"] 
        
           - path: "examples/plotting/*" 
        
           - path: "examples/server/app/*" 
        
             type: server 
        
             skip: [ 
        
               "faces", 
        
               "simple_hdf5", 
        
               "spectrogram", 
        
               "stocks",      # Bokeh.index not avaiable, can't compute bbox 
        
               "surface3d",   # Bokeh.index not avaiable, can't compute bbox 
        
             ]

It seems that similar examples (e.g. examples server_document, json_item) are skipped. Should this one be skipped too?

bryevdv · 2024-08-01T16:48:50Z

Should this one be skipped too?

Let's just go with that option for now

krassowski · 2024-08-01T17:49:27Z

It did not help. I think this is because the top-level file is not an example of Bokeh server with a single handler, but a flask application which embeds code which connects to the server running in a subprocess.

I am not sure how to proceed, possibly moving it elsewhere would be better. If a maintainer would prefer to rehash it please do push to my branch.

krassowski · 2024-08-02T10:02:39Z

Sorry, I was wrong it did help, another job failed which appears unrelated. I will restart CI by pushing an empty commit to see if it helps.

krassowski · 2024-08-02T10:20:14Z

Previously Bokeh-CI / unit-test (macos-latest, 3.12) failed and now Bokeh-CI / unit-test (macos-latest, 3.11) failed - it looks like the macos tests are flaky, is this correct?

bryevdv · 2024-08-02T16:14:21Z

Yes there is one server test that probably ought to get a retry policy or similar.

bryevdv · 2024-08-02T16:21:18Z

@krassowski I really appreciate your patience, I have one more request. After thinking about it, I would prefer this live under examples/server/api instead of app. All the examples under app are meant to be run with bokeh serve by the user, this example is much more like the embedding examples under api. I don't want to create confusion where someone tries to run bokeh serve iframe_embed (like all the other directory examples there) and it does not work.

krassowski · 2024-08-05T07:46:53Z

@bryevdv make sense - done, and it looks all tests passed!

bryevdv · 2024-08-05T20:33:05Z

Thanks @krassowski !

krassowski · 2024-08-05T21:18:48Z

Amazing, thank you! Is there a chance for backporting this PR or will it be only available in 3.6?

bryevdv · 2024-08-05T23:54:26Z

It seems like it would be easy to backport, so I have added the label. However, that just means that if there is a new release on previous branches, then this is a candidate to go in it. It's reasonably likely there will end up being more earlier point releases, but I can't guarantee it.

Edit: though looking at other PRs marked for backport, it seems fairly like there will be at some point.

…determination, support `data-absolute-url` (#14003) * Fix rendering on JupyterHub 4.1 * Get rid of the mutable variable Co-authored-by: Mateusz Paprocki <[email protected]> * Add an example * Do not use type casting, define a custom type guard * Reduce size of the negative example iframe * Lint * Move to `app/iframe_embed` * Skip `iframe_embed` for now * Retrigger CI * Move to `examples/server/api` --------- Co-authored-by: Mateusz Paprocki <[email protected]>

bryevdv · 2024-08-23T20:33:46Z

@krassowski this fix is available now in 3.5.2

…determination, support `data-absolute-url` (bokeh#14003) * Fix rendering on JupyterHub 4.1 * Get rid of the mutable variable Co-authored-by: Mateusz Paprocki <[email protected]> * Add an example * Do not use type casting, define a custom type guard * Reduce size of the negative example iframe * Lint * Move to `app/iframe_embed` * Skip `iframe_embed` for now * Retrigger CI * Move to `examples/server/api` --------- Co-authored-by: Mateusz Paprocki <[email protected]>

github-actions · 2024-12-04T15:02:20Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Fix rendering on JupyterHub 4.1

4e10311

This was referenced Jul 30, 2024

Fix panel preview rendering on JupyterHub 4.1+ holoviz/pyviz_comms#125

Merged

Panel preview does not work on JupyterHub 4.1+ hosted on a single domain holoviz/panel#7039

Closed

bryevdv added the status: ready label Jul 30, 2024

bryevdv modified the milestone: 3.6 Jul 30, 2024

bryevdv requested a review from mattpap July 30, 2024 17:11

mattpap added this to the 3.6 milestone Jul 31, 2024

mattpap reviewed Jul 31, 2024

View reviewed changes

bokehjs/src/lib/embed/server.ts Outdated Show resolved Hide resolved

Get rid of the mutable variable

0f1ca85

Co-authored-by: Mateusz Paprocki <[email protected]>

krassowski added 3 commits July 31, 2024 11:41

Add an example

3dcc898

Do not use type casting, define a custom type guard

c639a85

Reduce size of the negative example iframe

f56e971

krassowski requested a review from mattpap July 31, 2024 11:16

Lint

5ed140d

Move to app/iframe_embed

47b1258

Skip iframe_embed for now

9887855

Retrigger CI

99dc9e0

Move to examples/server/api

5d4181a

bryevdv approved these changes Aug 5, 2024

View reviewed changes

bryevdv added status: accepted and removed status: ready labels Aug 5, 2024

bryevdv merged commit 0300cc4 into bokeh:branch-3.6 Aug 5, 2024
24 checks passed

bryevdv added the NEEDS BACK PORT label Aug 5, 2024

mattpap mentioned this pull request Aug 9, 2024

Backports for 3.5.2 #14018

Merged

12 tasks

mattpap modified the milestones: 3.6, 3.5.2 Aug 22, 2024

mattpap removed the NEEDS BACK PORT label Aug 22, 2024

github-actions bot locked as resolved and limited conversation to collaborators Dec 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rendering on JupyterHub 4.1: respect `absolute_url` for protocol determination, support `data-absolute-url` #14003

Fix rendering on JupyterHub 4.1: respect `absolute_url` for protocol determination, support `data-absolute-url` #14003

krassowski commented Jul 30, 2024

codecov bot commented Jul 30, 2024 •

edited

Loading

mattpap Jul 31, 2024 •

edited

Loading

krassowski Jul 31, 2024

krassowski Jul 31, 2024

krassowski Jul 31, 2024

krassowski Jul 31, 2024

mattpap commented Jul 31, 2024

krassowski commented Jul 31, 2024

bryevdv commented Jul 31, 2024 •

edited

Loading

krassowski commented Jul 31, 2024

bryevdv commented Jul 31, 2024

krassowski commented Aug 1, 2024

bryevdv commented Aug 1, 2024

krassowski commented Aug 1, 2024

krassowski commented Aug 2, 2024

krassowski commented Aug 2, 2024

bryevdv commented Aug 2, 2024

bryevdv commented Aug 2, 2024

krassowski commented Aug 5, 2024

bryevdv commented Aug 5, 2024

krassowski commented Aug 5, 2024

bryevdv commented Aug 5, 2024 •

edited

Loading

bryevdv commented Aug 23, 2024

github-actions bot commented Dec 4, 2024

		if (absolute_url === undefined && frameElement !== null && (frameElement as HTMLIFrameElement).dataset.absoluteUrl !== undefined) {
		absolute_url = (frameElement as HTMLIFrameElement).dataset.absoluteUrl

Fix rendering on JupyterHub 4.1: respect absolute_url for protocol determination, support data-absolute-url #14003

Fix rendering on JupyterHub 4.1: respect absolute_url for protocol determination, support data-absolute-url #14003

Conversation

krassowski commented Jul 30, 2024

codecov bot commented Jul 30, 2024 • edited Loading

Codecov Report

mattpap Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

krassowski Jul 31, 2024

Choose a reason for hiding this comment

krassowski Jul 31, 2024

Choose a reason for hiding this comment

krassowski Jul 31, 2024

Choose a reason for hiding this comment

krassowski Jul 31, 2024

Choose a reason for hiding this comment

mattpap commented Jul 31, 2024

krassowski commented Jul 31, 2024

bryevdv commented Jul 31, 2024 • edited Loading

krassowski commented Jul 31, 2024

bryevdv commented Jul 31, 2024

krassowski commented Aug 1, 2024

bryevdv commented Aug 1, 2024

krassowski commented Aug 1, 2024

krassowski commented Aug 2, 2024

krassowski commented Aug 2, 2024

bryevdv commented Aug 2, 2024

bryevdv commented Aug 2, 2024

krassowski commented Aug 5, 2024

bryevdv commented Aug 5, 2024

krassowski commented Aug 5, 2024

bryevdv commented Aug 5, 2024 • edited Loading

bryevdv commented Aug 23, 2024

github-actions bot commented Dec 4, 2024

Fix rendering on JupyterHub 4.1: respect `absolute_url` for protocol determination, support `data-absolute-url` #14003

Fix rendering on JupyterHub 4.1: respect `absolute_url` for protocol determination, support `data-absolute-url` #14003

codecov bot commented Jul 30, 2024 •

edited

Loading

mattpap Jul 31, 2024 •

edited

Loading

bryevdv commented Jul 31, 2024 •

edited

Loading

bryevdv commented Aug 5, 2024 •

edited

Loading