Skip to content

Commit

Permalink
client: fix problems with the browser history
Browse files Browse the repository at this point in the history
The browser history of the client never *really* worked. When you were
redirected to a Lona app from an external site, you couldn't go back by
clicking on the back button. When you were three links into an Lona app and
refreshed the tab, the history before the refresh was unreachable.

Previously, both client1 and client2 created a new history entry, using
`history.pushState`, when a new view started, regardless the reason of the view
starting. So initial setup, refreshes, and reconnects also created history
items.
This is not correct! `history.pushState` should only be called when
you want to *extend* the browser history, beyond the normal history behaviour.

This patch limits all calls to `history.pushState` to cases of actual in-app
navigation, and adds a test to check the now corrected behaviour.

Signed-off-by: Florian Scherf <mail@florianscherf.de>
  • Loading branch information
fscherf committed Mar 20, 2024
1 parent 9a68ca7 commit c726709
Show file tree
Hide file tree
Showing 5 changed files with 116 additions and 20 deletions.
6 changes: 3 additions & 3 deletions lona/client/_lona/client/input-events.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export class LonaInputEventHandler {
event.stopPropagation();

try {
lona_window.run_view(node.href);
lona_window.run_view(node.href, {}, true);

} catch(error) {
lona_window.crash(error);
Expand Down Expand Up @@ -253,10 +253,10 @@ export class LonaInputEventHandler {
href += '?' + query;
};

lona_window.run_view(href);
lona_window.run_view(href, {}, true);

} else if(method == 'post') {
lona_window.run_view(action, form_data);
lona_window.run_view(action, form_data, true);

};

Expand Down
15 changes: 8 additions & 7 deletions lona/client/_lona/client/window.js
Original file line number Diff line number Diff line change
Expand Up @@ -271,10 +271,6 @@ export class LonaWindow {
this._view_runtime_id = view_runtime_id;
this._view_running = true;

if(this.lona_context.settings.update_address_bar) {
history.pushState({}, '', this.get_url().href);
};

this._clear();
this._clear_node_cache();

Expand All @@ -285,7 +281,7 @@ export class LonaWindow {
// TODO: implement loop detection

if(this.lona_context.settings.follow_redirects) {
this.run_view(payload);
this.run_view(payload, {}, true);

} else {
console.debug(
Expand Down Expand Up @@ -355,7 +351,7 @@ export class LonaWindow {
};
};

run_view(url, post_data) {
run_view(url, post_data, add_to_history) {
// Save the requested url to only show HTML messages that are related
// to this request.
// This prevents glitches when switching urls fast.
Expand All @@ -369,6 +365,11 @@ export class LonaWindow {
this._view_runtime_id = undefined;
this._set_url(url);

// add url to history
if(this.lona_context.settings.update_address_bar && add_to_history) {
history.pushState({}, '', this.get_url().href);
};

// reset view start timeout
if(this._view_start_timeout != undefined) {
clearTimeout(this._view_start_timeout);
Expand Down Expand Up @@ -412,6 +413,6 @@ export class LonaWindow {
};

setup(url) {
this.run_view(url);
this.run_view(url, {}, false);
};
};
6 changes: 3 additions & 3 deletions lona/client2/_lona/client2/input-events.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export class LonaInputEventHandler {
event.stopPropagation();

try {
lona_window.run_view(node.href);
lona_window.run_view(node.href, {}, true);

} catch(error) {
lona_window.crash(error);
Expand Down Expand Up @@ -253,10 +253,10 @@ export class LonaInputEventHandler {
href += '?' + query;
};

lona_window.run_view(href);
lona_window.run_view(href, {}, true);

} else if(method == 'post') {
lona_window.run_view(action, form_data);
lona_window.run_view(action, form_data, true);

};

Expand Down
15 changes: 8 additions & 7 deletions lona/client2/_lona/client2/window.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,6 @@ export class LonaWindow {
this._view_runtime_id = view_runtime_id;
this._view_running = true;

if(this.lona_context.settings.update_address_bar) {
history.pushState({}, '', this.get_url().href);
};

this._rendering_engine._clear();
this._rendering_engine._clear_node_cache();

Expand All @@ -118,7 +114,7 @@ export class LonaWindow {
// TODO: implement loop detection

if(this.lona_context.settings.follow_redirects) {
this.run_view(payload);
this.run_view(payload, {}, true);

} else {
console.debug(
Expand Down Expand Up @@ -188,7 +184,7 @@ export class LonaWindow {
};
};

run_view(url, post_data) {
run_view(url, post_data, add_to_history) {
// Save the requested url to only show HTML messages that are related
// to this request.
// This prevents glitches when switching urls fast.
Expand All @@ -202,6 +198,11 @@ export class LonaWindow {
this._view_runtime_id = undefined;
this._set_url(url);

// add url to history
if(this.lona_context.settings.update_address_bar && add_to_history) {
history.pushState({}, '', this.get_url().href);
};

// reset view start timeout
if(this._view_start_timeout != undefined) {
clearTimeout(this._view_start_timeout);
Expand Down Expand Up @@ -245,6 +246,6 @@ export class LonaWindow {
};

setup(url) {
this.run_view(url);
this.run_view(url, {}, false);
};
};
94 changes: 94 additions & 0 deletions tests/test_0307_history.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import pytest


@pytest.mark.parametrize('client_version', [1, 2])
@pytest.mark.parametrize('browser_name', ['chromium', 'firefox', 'webkit'])
async def test_client_history(
browser_name,
client_version,
lona_app_context,
):

"""
This test checks if the browser history works correctly by navigating
a browser page using the back and forward "buttons".
"""

from playwright.async_api import async_playwright

from lona.html import HTML, H1, A
from lona import View

def setup_app(app):

@app.route('/<title>')
class TestView(View):
def handle_request(self, request):
return HTML(
H1(request.match_info['title']),
A('foo', href='/foo', _id='foo'),
A('bar', href='/bar', _id='bar'),
A('baz', href='/baz', _id='baz'),
)

@app.route('/')
class Index(View):
def handle_request(self, request):
return HTML(
H1('index'),
A('foo', href='/foo', _id='foo'),
A('bar', href='/bar', _id='bar'),
A('baz', href='/baz', _id='baz'),
)

context = await lona_app_context(setup_app)

async with async_playwright() as p:
browser = await p.chromium.launch()
browser_context = await browser.new_context()
page = await browser_context.new_page()

# external site, about:blank
await page.goto('about:blank')
await page.wait_for_url('about:blank')

# /
await page.goto(context.make_url('/'))
await page.wait_for_url('/')
await page.wait_for_selector('h1:has-text("index")')

# /foo
await page.click('#foo')
await page.wait_for_url('/foo')
await page.wait_for_selector('h1:has-text("foo")')

# /bar
await page.click('#bar')
await page.wait_for_url('/bar')
await page.wait_for_selector('h1:has-text("bar")')

# back to /foo
await page.go_back()
await page.wait_for_url('/foo')
await page.wait_for_selector('h1:has-text("foo")')

# forward to /bar
await page.go_forward()
await page.wait_for_url('/bar')
await page.wait_for_selector('h1:has-text("bar")')

# back to /
await page.go_back()
await page.go_back()
await page.wait_for_url('/')
await page.wait_for_selector('h1:has-text("index")')

# back to external site
await page.go_back()
await page.wait_for_url('about:blank')

# forward to /foo
await page.go_forward()
await page.go_forward()
await page.wait_for_url('/foo')
await page.wait_for_selector('h1:has-text("foo")')

0 comments on commit c726709

Please sign in to comment.