flow icon indicating copy to clipboard operation
flow copied to clipboard

Flow page title is not propagated to the top part of the app layout in a Hilla main layout

Open Legioth opened this issue 4 months ago • 17 comments

Description of the bug

When using a Hilla main menu from start.vaadin.com, the page title of a Flow view (set using @PageTitle or HasDynamicTitle) is applied to document.title but the same value is not shown in the top area above the view (the <h2 slot="navbar"> element). Instead, the title in the navbar falls back to the application name. This is inconsistent with how it works for Hilla views in the same application and with how it works in an application with a main layout implemented in Flow.

Even though this should probably be ultimately fixed in Hilla or Start, I'm filing it against Flow since I expect that some new feature will have to be added to Flow first and then only enabled through Hilla or Start.

Expected behavior

Expected that the Flow view title is shown in the same way as Hilla page titles.

Minimal reproducible example

  1. Create an application at start.vaadin.com with one Hilla view and one Flow view. Give each view a distinct name (e.g. "Hilla" and "Flow")
  2. mvn
  3. Navigate to the Hilla view. Observe that that the name of the view is shown in the top of the main layout.
  4. Navigate to the Flow view (not that it's not in the menu so you need to type the URL or go thorough the dev mode 404 view). Observe that the value shown in the top of the main layout is the name of the app rather than the name of the view.

Versions

  • Vaadin / Flow version: Vaadin 24.4.0.alpha22 (or alpha21 which is what you get from Start today)

Legioth avatar Apr 18 '24 07:04 Legioth

I propose to retest this issue after we implement https://github.com/vaadin/flow/issues/19127.

mshabarov avatar Apr 19 '24 06:04 mshabarov

The menu title does not necessarily have anything to do with the page title, especially in the case of HasDynamicTitle and views that aren't even directly shown in the menu.

The view title needs to be propagated to the <h2> in the main layout at runtime in a way that is consistent with how document.title is set rather than based on the declarative menu configuration.

Furthermore, the page title should be updated even if the application doesn't use the automatic main menu feature at all but instead defines the menu manually.

Legioth avatar Apr 19 '24 07:04 Legioth

This is implemented in the layout component as currentTitle = useRouteMetadata()?.title so that would mean that the Flow element should implement the metadata for handle function with the correct content for the current route: https://reactrouter.com/en/main/hooks/use-matches#breadcrumbs

I think for Hilla the generated file-routes.json is responsible for the metadata.

caalador avatar Apr 22 '24 10:04 caalador

The metadata is known at compile time for Hilla views but it's only known after a round trip for Flow views. (Though there's also the question on how to implement dynamic page titles for Hilla views...)

Legioth avatar Apr 22 '24 11:04 Legioth

Browser knows already the correct title but only via document.title after Flow sets it. But that's too late for main layout. We can't use file-routes.json because it only contains client routes. We can't use window.Vaadin.server.views either because it only contains server routes shown in the menu (only accessible views).

So using document.title should work. For example by adding following code in @layout.tsx it works without need to add anything new to Flow:

import { useSignal } from '@vaadin/hilla-react-signals';
...
export default function MainLayout() {
...
useEffect(() => {
    document.title = currentTitle;
  }, [currentTitle]);

  const documentTitle = useSignal(currentTitle);

  new MutationObserver((mutations: MutationRecord[]) => {
    documentTitle.value = mutations[0].target.textContent || '';
  }).observe(
    document.querySelector('title') as Node,
    { subtree: true, characterData: true, childList: true }
  );
...
<h2 slot="navbar" className="text-l m-0">
        {documentTitle}
      </h2>
...

tltv avatar May 06 '24 13:05 tltv

That sounds like a hack even though it would in one way also solve the related problem of how Hilla views would update the title shown in the layout in case the view is dynamically updating the client.

One approach that I was thinking of would be that Vaadin defines a signal where the title is stored. The layout can then import and render it and any logic that wants to update the title can import as set it. One such party would be the logic in Flow that currently updates document.title based on the router metadata that it receives from the server.

Legioth avatar May 06 '24 14:05 Legioth

Setting signal e.g to window.Vaadin.documentTitleSignal in the main layout and setting value by Flow same time when document.title is set would work without MutationObserver.

So code in @layout.tsx would look like this instead:

import { useSignal } from '@vaadin/hilla-react-signals';
...
export default function MainLayout() {
...
useEffect(() => {
    document.title = currentTitle;
    documentTitle.value = currentTitle;
  }, [currentTitle]);

  const documentTitle = useSignal(currentTitle);
  // @ts-ignore
  window.Vaadin.documentTitleSignal = documentTitle;
...
<h2 slot="navbar" className="text-l m-0">
        {documentTitle}
      </h2>
...

useEffect has to now update the signal value to keep hilla view titles updated.

And in Flow's UIInternals#setTitle(String) would set the title:

JavaScriptInvocation invocation = new JavaScriptInvocation(
                "document.title = $0; if(window?.Vaadin?.documentTitleSignal) { window.Vaadin.documentTitleSignal.value = $0; }",
                title);

tltv avatar May 07 '24 09:05 tltv

I would rather implement the main layout in a slightly different way that should be basically equivalent from a functional point of view but might be easier to understand.

@layout.tsx would drop the current have these lines of code outside the render function:

window.Vaadin.documentTitleSignal = signal("");
effect(() => {
  document.title = window.Vaadin.documentTitleSignal.value;
});

It can then directly assign the signal in the render function: window.Vaadin.documentTitleSignal.value = currentTitle and use {window.Vaadin.documentTitleSignal} instead of {documentTitle} in the template.

Legioth avatar May 07 '24 10:05 Legioth

window.vaadin.documentTitleSignal needs a type declaration too. Without going outside of the @layout.tsx it would look like this now:

import { Signal, useSignal } from '@vaadin/hilla-react-signals';
const vaadin = window.Vaadin as {
  documentTitleSignal: Signal<string>;
};

export default function MainLayout() {
...
  vaadin.documentTitleSignal = useSignal("");

  useEffect(() => {
    vaadin.documentTitleSignal.value = currentTitle;
    document.title = vaadin.documentTitleSignal.value;
  }, [currentTitle]);
      <h2 slot="navbar" className="text-l m-0">
        {vaadin.documentTitleSignal}
      </h2>

I think that we can add PR in flow that sets window.Vaadin.documentTitleSignal already unless there are better ideas to do this. It would make sense to move the type declaration and signal outside and just import the signal for example from hilla-file-router which is already in @layout.tsx. That part can be split in another ticket.

tltv avatar May 07 '24 11:05 tltv

That example might work in this particular case but it can be improved for clarity and to work as expected also in other cases.

The first problem is that the signal instance is now owned by a MainLayout component instance but it's exported to a wider scope (since window.Vaadin may outlive the MainLayout instance). The signal instance should instead be created outside the render function (but it can still be in the same module for now).

The second problem is that document.title is updated only in a React useEffect which means that it's updated only when the main layout is re-rendered. If the application has this kind of signal available, then it would be quite natural for a view to e.g. update the signal value based on updates received asynchronously from the server. With the current implementation, that would update the value in the layout but it wouldn't update document.title. The update should be done in a signal effect rather than in a React effect.

With those two adjustments, we would have these lines of code before the render function:

import { signal, effect } from '@vaadin/hilla-react-signals';

const vaadin = window.Vaadin as {
  documentTitleSignal: Signal<string>;
};
vaadin.documentTitleSignal = signal("");
effect(() =>  document.title = vaadin.documentTitleSignal.value);

With those changes, the React useEffect would be redundant and we could instead directly assign the signal value in the render function (vaadin.documentTitleSignal.value = currentTitle) since the signal implementation already takes care of change detection.

Legioth avatar May 08 '24 07:05 Legioth

Starter's useEffect would still need to be changed to update the signal value with vaadin.documentTitleSignal.value = currentTitle;. Otherwise title is only updated for Flow view.

Also another thing to note with this new code is how the starter sets currentTitle with const currentTitle = useViewConfig()?.title ?? defaultTitle; causing default title to show up just for a blink of an eye when navigating from Hilla to Flow view (caused by delay it takes from the main layout render to javascript command received from the server). This is fixed simply by showing blank title as a default value, or by keeping the old title.

tltv avatar May 08 '24 09:05 tltv

I don't think there's any reason to have vaadin.documentTitleSignal.value = currentTitle; inside useEffect. All that useEffect does in that case is to avoid running the callback again if currentTitle hasn't changed but that's something that is already handled by the documentTitleSignal instance.

For defaultTitle, I'm starting suspect that it's never correct to update the title in the layout's render function. But we should maybe accept the flickering for now and then separately check with the Hilla team to see if they have some idea for how to better update the title for Hilla views.

Legioth avatar May 08 '24 11:05 Legioth

To summarize all so far, following @layout.tsx works with the start.vaadin.com app with #19329 change in Flow:

import { createMenuItems, useViewConfig } from '@vaadin/hilla-file-router/runtime.js';
import { AppLayout, DrawerToggle, Icon, SideNav, SideNavItem } from '@vaadin/react-components';
import { Suspense } from 'react';
import { Outlet, useLocation, useNavigate } from 'react-router-dom';
import { Signal, signal, effect } from '@vaadin/hilla-react-signals';

const vaadin = window.Vaadin as {
  documentTitleSignal: Signal<string>;
};
vaadin.documentTitleSignal = signal("");
effect(() =>  document.title = vaadin.documentTitleSignal.value);

export default function MainLayout() {
  const currentTitle = useViewConfig()?.title ?? "";
  const navigate = useNavigate();
  const location = useLocation();

  vaadin.documentTitleSignal.value = currentTitle;

  return (
    <AppLayout primarySection="drawer">
      <div slot="drawer" className="flex flex-col justify-between h-full p-m">
        <header className="flex flex-col gap-m">
          <h1 className="text-l m-0">My App</h1>
          <SideNav onNavigate={({ path }) => navigate(path!)} location={location}>
            {createMenuItems().map(({ to, title, icon }) => (
              <SideNavItem path={to} key={to}>
                {icon ? <Icon src={icon} slot="prefix"></Icon> : <></>}
                {title}
              </SideNavItem>
            ))}
          </SideNav>
        </header>
      </div>

      <DrawerToggle slot="navbar" aria-label="Menu toggle"></DrawerToggle>
      <h2 slot="navbar" className="text-l m-0">
        {vaadin.documentTitleSignal}
      </h2>

      <Suspense>
        <Outlet />
      </Suspense>
    </AppLayout>
  );
}

tltv avatar May 08 '24 12:05 tltv

I'm wondering if that could create some kind of unexpected side effect / bug for people that use the "oldish" pattern to change the page title to e.g. add (number of notifications) in front of the title.. this shouldn't probably be added to the h2 of the main layout.. because Page::setTitle from flow delegates to the changed method that also updates the documentTitleSignal.

knoobie avatar May 08 '24 13:05 knoobie

All the actual logic for interpreting the value that Flow sets in the signal instance is in @layout.tsx which is owned by the application. The application developer can remove all the new logic from that file and then everything will work like in Vaadin 24.3. Or they can modify the logic to e.g. combine the documentTitleSignal value with the value of an application-specific signal to form the actual document.title value.

Legioth avatar May 08 '24 13:05 Legioth

This ticket/PR has been released with Vaadin 24.4.0.beta3 and is also targeting the upcoming stable 24.4.0 version.

vaadin-bot avatar May 15 '24 09:05 vaadin-bot

I've updated the referenced skeleton starters. ~Wanted to update the main layout templates in Start, but noticed that they don't have a header tag that could use the page title from server.~ No, it's actually used in some templates.

We need to document also the vaadin.documentTitleSignal object and how to use it.

mshabarov avatar May 15 '24 13:05 mshabarov