vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
618 stars 167 forks source link

ViewConfig title gets reset briefly on navigation #20207

Open marcushellberg opened 2 weeks ago

marcushellberg commented 2 weeks ago

Description of the bug

When navigating between a Hilla and Flow view, the view title briefly gets reset, visible in this 0.1x screen capture.

https://github.com/user-attachments/assets/127667ab-5bb1-47c1-b8e1-dbbce906d57c

The string is coming from

const currentTitle = useViewConfig()?.title ?? defaultTitle;

Which leads me to suspect that something in the navigation logic resets the value between the navigation.

Expected behavior

The old title should be updated to the new title right away, not get reset in between.

Minimal reproducible example

App with a Flow and Hilla view from start.vaadin.com

Versions

tltv commented 1 week ago

Reason for delayed title update is originated from flow generated javascript command which is run after server round trip is done. Unfortunately this command is executed very late making layout render title with default value first. Technically flow could set the title a bit quicker but it would still not be enough. Something has to be changed in the @layout.tsx. I get best result by adjusting the @layout.tsx file a bit like this:

const currentTitle = useViewConfig()?.title ?? (documentTitleSignal.value || '');

Which means that it will keep showing previous title until documentTitleSignal.value has actually changed, or if signal is not yet set, then it shows just blank title until new title is loaded (full page load).

If this works (I didn't test it with more complex cases or with 24.5 yet), then probably we can update start to do it like that.

marcushellberg commented 1 week ago

That's quite a lot of complexity to add to a starter. Could we somehow hide some of that logic within useViewConfig()?.title?

tltv commented 1 week ago

Complexity can be moved to new function like useViewConfigTitle(defaultTitle). Added PR draft in Hilla 24.4.

With this, instead of importing useViewConfig, you import useViewConfigTitle and use following line where given default title is optional:

const currentTitle = useViewConfigTitle(defaultTitle);
platosha commented 2 days ago

Such fallback from current view title to document title would have impact on this use case: I want my document title to contain the app name, for example: ${currentViewTitle} — My App, so that the app name is visible in the browser tab, while the layout heading only displays the currentViewTitle. It isn't implemented in the starter, but should be common and straightforward IMO.

I therefore think that it's not a good idea to make view title depend on the document title.

platosha commented 2 days ago

I'd suggest to avoid resetting documentTitleSignal if viewConfig().title is missing:

const defaultTitle = document.title;
const documentTitleSignal = signal(defaultTitle);
effect(() => {
  document.title = documentTitleSignal.value;
});

// Publish for Vaadin to use
(window as any).Vaadin.documentTitleSignal = documentTitleSignal;

export default function MainLayout() {
  const currentViewTitle = useViewConfig()?.title;
  useEffect(() => {
    if (currentViewTitle) {
      documentTitleSignal.value = currentViewTitle;
    }
  }, [currentViewTitle]);
  // ...
}
tltv commented 2 days ago

With both ways showing tab title like ${currentViewTitle} — My App would be simple as document.title = documentTitleSignal.value + ' — My App';. It just means that from Flow code, given page title, dynamic or not, can't include anything extra other than pure view title.

I like this way to just add if-check for undefined and remove ?? defaultTitle. Seems to work just same as with the new utility function. If it's good like this, then there's no need to introduce new utility.

platosha commented 2 days ago

document.title = documentTitleSignal.value + ' — My App';

The snippet above works perfectly well for the use case, but looks unexpected that documentTitleSignal is not applied directly to document.title.

Perhaps documentTitleSignal is misnamed then, should have a different name. Should it be pageTitleSignal, as it's related with Page.setTitle() in Flow?

tltv commented 2 days ago

documentTitleSignal is named more from Flow's perspective. Actually it is signal for document.title set by Flow in UIInternals#generateTitleScript where it also sets the same value to document.title before updating signal value for client main layout.

But in starter, local documentTitleSignal could be renamed to something like pageTitleSignal instead as you suggested.

platosha commented 1 day ago

Alright then. Renaming only local documentTitleSignal in the starter is not necessary IMO.