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

Navigation problem when ClickShortcut is present in LegacyWrapper #115

Closed
paodb opened this issue Jul 27, 2022 · 9 comments · Fixed by vaadin/flow#14397
Closed

Navigation problem when ClickShortcut is present in LegacyWrapper #115

paodb opened this issue Jul 27, 2022 · 9 comments · Fixed by vaadin/flow#14397

Comments

@paodb
Copy link

paodb commented Jul 27, 2022

Description

Navigation seems to break when trying to navigate to a Flow View containing a LegacyWrapper of a Vaadin 7 component. The issue seems to be caused when adding a ClickShortcut to a Vaadin 7 Button that is inside a Legacy Wrapper in a Vaadin Flow View. If I remove that ClickShortcut, the navigation works okay. See attached image of the problem:

mpr_navigation1

Minimal reproducible example

import com.vaadin.flow.component.button.Button;
import com.vaadin.flow.component.orderedlayout.VerticalLayout;
import com.vaadin.flow.router.Route;

@Route(value = "navigate")
public class NavigatePage extends VerticalLayout {

    public NavigatePage() {
        Button button = new Button("Navigate", event -> {
            event.getSource().getUI().ifPresent(ui -> ui.navigate("button"));
        });

        add(button);
    }
}
import com.vaadin.event.ShortcutAction;
import com.vaadin.flow.component.orderedlayout.VerticalLayout;
import com.vaadin.flow.router.Route;
import com.vaadin.mpr.LegacyWrapper;
import com.vaadin.ui.Button;

@Route(value = "button")
public class ButtonPage extends VerticalLayout {

    public ButtonPage() {
        Button button = new Button("Vaadin 7 Button");
        button.setClickShortcut(ShortcutAction.KeyCode.ENTER); // comment to make it work

        add(new LegacyWrapper(button));
    }
}

Complete example here: mpr_navigation_sources.zip

Versions

  • Vaadin Framework: 7.7.32
  • Flow: 23.1.3
  • MPR: 6.0.1
  • Java JRE / JDK: 11
@mcollovati mcollovati added the BFP label Jul 28, 2022
@mshabarov mshabarov added the bug Something isn't working label Jul 28, 2022
@mshabarov mshabarov added this to Needs triage in OLD Vaadin Flow bugs & maintenance (Vaadin 10+) via automation Jul 28, 2022
@mshabarov mshabarov moved this from Needs triage to P1 - High priority in OLD Vaadin Flow bugs & maintenance (Vaadin 10+) Jul 28, 2022
@MarcinVaadin
Copy link
Member

MarcinVaadin commented Aug 5, 2022

Investigation results:

Why going root -> Button or navigate -> Button does not brake the location? Because:

  1. Framework UI is being lazely loaded when opening view with v7 component
  2. Current location is stored in v7 Page object
  3. Two UIDL updates are returned, one for Flow (no dirty v7 components for state update yet) and one after Framework initialization

And why going back and forth is breaking location:

  1. Adding clickShortcut is causing MprUI to be marked as dirty
  2. Dirty visible components state is added to UIDL to force component repaint
  3. MprUI state contains location from Page instance (see point 2 from list above)
  4. Vaadin 23 UidlRequestHandler.removeOffendingMprHashFragment is looking for location and forces unnecessary history state push (with previously stored improper location) - (introduced in fix path when full location is provided flow#7693)

@mshabarov mshabarov self-assigned this Aug 9, 2022
@mshabarov mshabarov moved this from P1 - High priority to WIP in OLD Vaadin Flow bugs & maintenance (Vaadin 10+) Aug 9, 2022
@mshabarov
Copy link

mshabarov commented Aug 16, 2022

Indeed, this is a regression after applying a patch for fixing vaadin/flow#7540 .

UidlRequestHandler::removeOffendingMprHashFragment adds an extra http://localhost:8080 to the history, through this method:
https://github.com/vaadin/flow/blob/020ee677e59d3b0f411232e402c55a68f34924f2/flow-server/src/main/java/com/vaadin/flow/server/communication/UidlRequestHandler.java#L309

The navigation works well if the server-side bootstrapping is turned on through the parameter vaadin.useDeprecatedV14Bootstrapping=true in application.properties. In server-side bootstrapping mode the fragment manipulations are disabled. This can be used as a workaround until the issue is fixed.

@kshashov
Copy link

We tried turning useDeprecatedV14Bootstrapping on but the issue was still reproducible in a slightly different way
mpr_navigation2

@mshabarov
Copy link

@kshashov thanks, indeed, this also doesn't fix the problem. Looks like a completely different issue in Flow vaadin/flow#14323 .

mshabarov added a commit to vaadin/flow that referenced this issue Aug 22, 2022
Fixes the hashed segment removal logic:
if no segment with hash is present in the location,
while navigating between Flow and legacy views/components,
the update of the browser history is not needed.

Fixes vaadin/multiplatform-runtime#115
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from In progress to Done - pending release Aug 22, 2022
OLD Vaadin Flow bugs & maintenance (Vaadin 10+) automation moved this from WIP to Closed Aug 22, 2022
vaadin-bot pushed a commit to vaadin/flow that referenced this issue Aug 22, 2022
Fixes the hashed segment removal logic:
if no segment with hash is present in the location,
while navigating between Flow and legacy views/components,
the update of the browser history is not needed.

Fixes vaadin/multiplatform-runtime#115
vaadin-bot pushed a commit to vaadin/flow that referenced this issue Aug 22, 2022
Fixes the hashed segment removal logic:
if no segment with hash is present in the location,
while navigating between Flow and legacy views/components,
the update of the browser history is not needed.

Fixes vaadin/multiplatform-runtime#115
vaadin-bot pushed a commit to vaadin/flow that referenced this issue Aug 22, 2022
Fixes the hashed segment removal logic:
if no segment with hash is present in the location,
while navigating between Flow and legacy views/components,
the update of the browser history is not needed.

Fixes vaadin/multiplatform-runtime#115
taefi pushed a commit to vaadin/flow that referenced this issue Aug 22, 2022
)

Fixes the hashed segment removal logic:
if no segment with hash is present in the location,
while navigating between Flow and legacy views/components,
the update of the browser history is not needed.

Fixes vaadin/multiplatform-runtime#115

Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
taefi pushed a commit to vaadin/flow that referenced this issue Aug 22, 2022
)

Fixes the hashed segment removal logic:
if no segment with hash is present in the location,
while navigating between Flow and legacy views/components,
the update of the browser history is not needed.

Fixes vaadin/multiplatform-runtime#115

Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
taefi pushed a commit to vaadin/flow that referenced this issue Aug 22, 2022
)

Fixes the hashed segment removal logic:
if no segment with hash is present in the location,
while navigating between Flow and legacy views/components,
the update of the browser history is not needed.

Fixes vaadin/multiplatform-runtime#115

Co-authored-by: Mikhail Shabarov <61410877+mshabarov@users.noreply.github.com>
@Tostino
Copy link

Tostino commented Aug 22, 2022

Looking for guidance on when the next hotfix release will be that will include this, or the best way to incorporate this just this fix into our build, as we need to get our software released very soon and are looking for the path that gives us the least resistance here.

@mshabarov
Copy link

@Tostino which version of Vaadin do you use? Is it V23.1.x ? Since the fix is on the Flow side, I can release the corresponding version of Flow with this patch tomorrow morning and you can override it in your project. Platform release comes later on according to a normal schedule, so once it's ready you would remove the override and just upgrade the platform version. Does it sound fine for you?

@Tostino
Copy link

Tostino commented Aug 22, 2022

We made the original report, so we are on 23.1.3 right now. That seems like it'll work just fine, we can just upgrade the flow version.

Thanks for the help here.

@mshabarov
Copy link

@Tostino please try a new Flow 23.1.6 with this patch:

    <dependencyManagement>
        <dependencies>
            <dependency>
                <groupId>com.vaadin</groupId>
                <artifactId>flow-bom</artifactId>
                <version>23.1.6</version>
                <type>pom</type>
                <scope>import</scope>
            </dependency>
            <dependency>
                <groupId>com.vaadin</groupId>
                <artifactId>vaadin-bom</artifactId>
                <type>pom</type>
                <scope>import</scope>
                <version>23.1.6</version>
            </dependency>
        </dependencies>
    </dependencyManagement>

Will be included into next 23.1.x platform release.

@vaadin-bot
Copy link

This ticket/PR has been released with Vaadin 23.1.8.

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

Successfully merging a pull request may close this issue.

7 participants