Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Tracked by #5012
Legioth opened this issue Apr 18, 2024 · 18 comments · Fixed by #19329
Closed
Tracked by #5012

Comments

@Legioth
Copy link
Member

Legioth commented Apr 18, 2024

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)
@mshabarov
Copy link
Contributor

I propose to retest this issue after we implement #19127.

@Legioth
Copy link
Member Author

Legioth commented Apr 19, 2024

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.

@caalador
Copy link
Contributor

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.

@Legioth
Copy link
Member Author

Legioth commented Apr 22, 2024

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...)

@tltv
Copy link
Member

tltv commented May 6, 2024

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>
...

@Legioth
Copy link
Member Author

Legioth commented May 6, 2024

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.

@tltv
Copy link
Member

tltv commented May 7, 2024

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);

@Legioth
Copy link
Member Author

Legioth commented May 7, 2024

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.

@tltv
Copy link
Member

tltv commented May 7, 2024

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 added a commit that referenced this issue May 8, 2024
`UIInternals#setTitle(String)` sets page title via JavaScript to `document.title` and optionally `window.Vaadin.documentTitleSignal.value` where documentTitleSignal is expected but not limited to be Signal<string> type with a value field. This allows Hilla main layout, when used, being kept in sync with the Flow page title even when set via PageTitle annotation or HasDynamicTitle interface.

Fixes: #19200
@Legioth
Copy link
Member Author

Legioth commented May 8, 2024

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.

@tltv
Copy link
Member

tltv commented May 8, 2024

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.

@Legioth
Copy link
Member Author

Legioth commented May 8, 2024

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.

@tltv
Copy link
Member

tltv commented May 8, 2024

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>
  );
}

@knoobie
Copy link
Contributor

knoobie commented May 8, 2024

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.

@Legioth
Copy link
Member Author

Legioth commented May 8, 2024

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.

mshabarov added a commit that referenced this issue May 13, 2024
`UIInternals#setTitle(String)` sets page title via JavaScript to `document.title` and optionally `window.Vaadin.documentTitleSignal.value` where documentTitleSignal is expected but not limited to be Signal<string> type with a value field. This allows Hilla main layout, when used, being kept in sync with the Flow page title even when set via PageTitle annotation or HasDynamicTitle interface.

Fixes: #19200

Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
@mshabarov mshabarov reopened this May 13, 2024
vaadin-bot pushed a commit that referenced this issue May 13, 2024
`UIInternals#setTitle(String)` sets page title via JavaScript to `document.title` and optionally `window.Vaadin.documentTitleSignal.value` where documentTitleSignal is expected but not limited to be Signal<string> type with a value field. This allows Hilla main layout, when used, being kept in sync with the Flow page title even when set via PageTitle annotation or HasDynamicTitle interface.

Fixes: #19200

Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
mshabarov added a commit that referenced this issue May 13, 2024
`UIInternals#setTitle(String)` sets page title via JavaScript to `document.title` and optionally `window.Vaadin.documentTitleSignal.value` where documentTitleSignal is expected but not limited to be Signal<string> type with a value field. This allows Hilla main layout, when used, being kept in sync with the Flow page title even when set via PageTitle annotation or HasDynamicTitle interface.

Fixes: #19200

Co-authored-by: Tomi Virtanen <tltv@vaadin.com>
Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
@vaadin-bot
Copy link
Collaborator

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

@mshabarov
Copy link
Contributor

mshabarov commented May 15, 2024

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 added a commit to vaadin/flow-hilla-hybrid-example that referenced this issue May 15, 2024
Uses signal for dynamically updated page title.

Part of vaadin/flow#19200
tltv added a commit to vaadin/docs that referenced this issue May 17, 2024
russelljtdyer added a commit to vaadin/docs that referenced this issue May 21, 2024
* docs: document documentTitleSignal

RelatedTo: vaadin/flow#19200

* First full pass at editing additions.

* Second pass: edited full document.

* Third pass: minor edits.

---------

Co-authored-by: Russell J.T. Dyer <6652767+russelljtdyer@users.noreply.github.com>
platosha pushed a commit to vaadin/skeleton-starter-hilla-react that referenced this issue May 22, 2024
Uses signal for dynamically updated page title.

Part of vaadin/flow#19200
@vaadin-bot
Copy link
Collaborator

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

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