vaadin / framework

Vaadin 6, 7, 8 is a Java framework for modern Java web applications.
http://vaadin.com/
Other
1.77k stars 729 forks source link

scrollIntoView no longer works within Grid as of Vaadin 8.18.0 #12580

Closed pogocode closed 1 year ago

pogocode commented 1 year ago

As of Vaadin 8.18.0, it is no longer possible to use scrollIntoView to move cells within a grid.

Minimal Reproducible Example

The following application using Vaadin version 8.17.0 works correctly (i.e. scrollIntoView works):

mpr-demo-mpr-6-grid-scrolling-8-17-0.zip

The following application using Vaadin version 8.18.0 does not work correctly (i.e. scrollIntoView does not work):

mpr-demo-mpr-6-grid-scrolling-8-18-0.zip

For both of the above applications (which are the same with the exceptions of the Vaadin version)

Start the application:

mvn -Pproduction jetty:run

Navigate to the following:

http://localhost:8080/legacy

In the 8.17.0 version:

In the 8.18.0 version:

Expected behaviour

It should be possible to use scrollIntoView within the grid.

Actual behaviour

As of version 8.18.0 it is no longer possible to use scrollIntoView to navigate within the grid.

Versions:

Vaadin Framework version (e.g. 8.3.1) Browser version - Chrome version 110.0.5481.100 (Official Build) (x86_64) Web container name and version - Jetty 9.4.14.v20181114

Also the following, although these are probably not relevant to the issue (it just happens that I work with MPR and this was the simplest way for me to present the issue:

Flow: 23.3.6 MPR: 6.1.1 Java JRE / JDK: 11

Ansku commented 1 year ago

I can reproduce the issue, but why do you need the scrollIntoView? Wouldn't grid.scrollToColumn(grid.getColumns().get(9)) serve the same purpose with less hassle?

Ansku commented 1 year ago

As a bit of a background, the functionality was changed in 8.18.0 because of #12560, and the scrollToColumn was also added then. That method has existed on the client side for a long time, but now it's easier to call it from the server too. I'm not entirely certain that those changes can coexist with the direct JavaScript call, but I can look into it if you have a use case that can't be solved by that new method or simply focusing a component within the target cell.

pogocode commented 1 year ago

Thanks for the quick update. Our actual problem is that quite a lot of our integration tests using selenium rely on us scrolling a cell into view before we can make assertions on it.

If scrollIntoView cannot co-exist with the fix that you mentioned, is there an alternative client side call that we can make? Is scrollToColumn available?

If that's the case then I can update our tests with a more appropriate call, but it wasn't obvious to me how I could achieve this.

If not, and this is intentional behaviour / won't be fixed, then we'll need to modify our tests to account for this.

Thanks for your help!

On Tue, 28 Feb 2023, 17:52 Anna Koskinen, @.***> wrote:

As a bit of a background, the functionality was changed in 8.18.0 because of #12560 https://github.com/vaadin/framework/issues/12560, and the scrollToColumn was also added then. That method has existed on the client side for a long time, but now it's easier to call it from the server too. I'm not entirely certain that those changes can coexist with the direct JavaScript call, but I can look into it if you have a use case that can't be solved by that new method or simply focusing a component within the target cell.

— Reply to this email directly, view it on GitHub https://github.com/vaadin/framework/issues/12580#issuecomment-1448614149, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC7IKSEGU4YS7OC7MGT2OFLWZY3NXANCNFSM6AAAAAAVKRVQMY . You are receiving this because you authored the thread.Message ID: @.***>

Ansku commented 1 year ago

Client-side Grid has scrollToColumn(int columnIndexDOM, ScrollDestination destination), and it calls Escalator scrollToColumn(final int columnIndex, final ScrollDestination destination, final int padding), where the default padding is destination == ScrollDestination.MIDDLE ? 0 : 10. I think it should be possible to call either of these methods directly.

Ansku commented 1 year ago

I just realized I misread your comment. No, there is no built-in JavaScript method for custom calls for the client-side. With TestBench you can simply call $(GridElement.class).first().getCell(0, 9); to scroll the queried cell into view. I haven't tried that with pure Selenium, but I'd say those helper methods in TestBench exist for a good reason...

pogocode commented 1 year ago

Hi,

Yes, it's a shame that our integration tests don't utilise TestBench, but it would be non-trivial for us to migrate at this time.

It might be a case of navigating the grids via key presses in the tests then. Okay, thanks. Happy for this to be closed if it's a "won't fix".

On Tue, 28 Feb 2023, 18:45 Anna Koskinen, @.***> wrote:

I just realized I misread your comment. No, there is no built-in JavaScript method for custom calls for the client-side. With TestBench you can simply call $(GridElement.class).first().getCell(0, 9); to scroll the queried cell into view. I haven't tried that with pure Selenium, but I'd say those helper methods in TestBench exist for a good reason...

— Reply to this email directly, view it on GitHub https://github.com/vaadin/framework/issues/12580#issuecomment-1448685638, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC7IKSBB3MPPA2BEQNIYJG3WZZBVRANCNFSM6AAAAAAVKRVQMY . You are receiving this because you authored the thread.Message ID: @.***>

Ansku commented 1 year ago

As a quick-and-dirty workaround, you could also extend the GridConnector e.g. like this (with the JSNI call updated to match your package structure):

@SuppressWarnings("deprecation")
@Connect(com.vaadin.ui.Grid.class)
public class CustomGridConnector extends GridConnector {
    @Override
    protected void init() {
        super.init();
        exposeScrollToColumn(this, getWidget().getElement());
    }

    private void scrollToColumn(int index) {
        getWidget().scrollToColumn(index, ScrollDestination.MIDDLE);
    }

    private native void exposeScrollToColumn(GridConnector connector,
            Element grid)
    /*-{
        grid.scrollToColumnJS = $entry(function(index) {
            return connector.@org.vaadin.mprdemo.client.CustomGridConnector::scrollToColumn(*)(index);
        });
    }-*/;
}

and then call it with "document.getElementById('the-scrolling-grid').scrollToColumnJS(9)".

I haven't spent a whole lot of time thinking that through so it's not very polished, but it seems to work in my test project at least.