Endless loop of horizontal and vertical scrollbar visibility change in Grid

[janArb]

• Vaadin Framework version: 8.12.0.alpha1

• Browser version: Windows Chrome Version 90.0.4430.93 (64 bit)

• Web container name and version: Payara 5.2020.2

• Description of the bug

  • Grid with rows that fit without vertical scrollbar (depends on exact viewport size, etc.)
  • I didn't understand yet why, but horizontal scrollbar appears
  • horizontal scrollbar needs vertical space, i.e. vertical scrollbar is needed now after that
  • vertical scrollbar appears
  • vertical scrollbar needs space
    • since Add column width recalculation when vertical scrollbar hidden/shown. #12058 column resizing is triggered due to vertical scrollbar needing horizontal space
    • columns are updated
    • most likely horizontal scrollbar was never needed, at least it is disappearing now
    • vertical space is enough to remove vertical scrollbar again
    • column resizing is triggered again
  • I didn't understand yet why, but horizontal scrollbar appears
  • endless loop on client side never terminates

• Minimal reproducible example

  • I don't know how to create a minimal example, we have a very complex monolith and I would need help to start with a minimal environment, is there a tutorial or something?
  • I guess it could be necessary to call com.vaadin.ui.Grid#recalculateColumnWidths from different cases to get the minimal example working, we have that e.g. for
    • grid.addColumnReorderListener((ColumnReorderListener) event -> grid.recalculateColumnWidths());

    • dataProvider.addDataProviderListener(event -> {
          if (!(event instanceof DataRefreshEvent)) {
              grid.recalculateColumnWidths();
          }
      });
      

    • resizeListener = new ComponentResizeListener() {
         @Override
         public void sizeChanged(ComponentResizeEvent cre) {
            grid.recalculateColumnWidths();
         }
      };
      new SizeReporter(this).addResizeListener(resizeListener);
      
      addAttachListener((AttachListener) event -> {
          if (getUI() instanceof ResizableUI)
              ((ResizableUI) getUI()).addComponentResizeListener(resizeListener);
      });
      
      addDetachListener((DetachListener) event -> {
          if (getUI() instanceof ResizableUI)
              ((ResizableUI) getUI()).removeComponentResizeListener(resizeListener);
      });
      

    • other even more complex dependencies also call

      grid.getDataProvider().refreshAll();
      grid.recalculateColumnWidths();
      

• Expected behavior

  • no endless loop on client side in conjunction with column resizing and scrollbars visibility changes

• Actual behavior

  • endless loop blocking GUI completely

I verified that removing the new part from verticalScrollbar.addVisibilityHandler in line 7347 of Escalator from #12058 prevents this endless loop. As I understood this might be needed in other cases it would be good to have a endless loop detection to prevent this very generally.

Here is my patch I used for verfication:

Index: client/src/main/java/com/vaadin/client/widgets/Escalator.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- client/src/main/java/com/vaadin/client/widgets/Escalator.java	(revision 5b06221caab657dc17a8710c4397c1b2ce097a04)
+++ client/src/main/java/com/vaadin/client/widgets/Escalator.java	(date 1619681134305)
@@ -7321,29 +7321,29 @@
         root.appendChild(verticalScrollbar.getElement());
         verticalScrollbar.addScrollHandler(scrollHandler);
         verticalScrollbar.setScrollbarThickness(scrollbarThickness);
-        verticalScrollbar
-                .addVisibilityHandler(new ScrollbarBundle.VisibilityHandler() {
-
-                    private boolean queued = false;
-
-                    @Override
-                    public void visibilityChanged(
-                            ScrollbarBundle.VisibilityChangeEvent event) {
-                        if (queued) {
-                            return;
-                        }
-                        queued = true;
-
-                        /*
-                         * We either lost or gained a scrollbar. In either case,
-                         * we may need to update the column widths.
-                         */
-                        Scheduler.get().scheduleFinally(() -> {
-                            fireVerticalScrollbarVisibilityChangeEvent();
-                            queued = false;
-                        });
-                    }
-                });
+//        verticalScrollbar
+//                .addVisibilityHandler(new ScrollbarBundle.VisibilityHandler() {
+//
+//                    private boolean queued = false;
+//
+//                    @Override
+//                    public void visibilityChanged(
+//                            ScrollbarBundle.VisibilityChangeEvent event) {
+//                        if (queued) {
+//                            return;
+//                        }
+//                        queued = true;
+//
+//                        /*
+//                         * We either lost or gained a scrollbar. In either case,
+//                         * we may need to update the column widths.
+//                         */
+//                        Scheduler.get().scheduleFinally(() -> {
+//                            fireVerticalScrollbarVisibilityChangeEvent();
+//                            queued = false;
+//                        });
+//                    }
+//                });
 
         root.appendChild(horizontalScrollbar.getElement());
         horizontalScrollbar.addScrollHandler(scrollHandler);

[janArb]

Hi @Ansku, could you send me a minimal example template project I could use to try to reproduce this issue?

[TatuLund]

Vaadin Framework version: 8.12.0.alpha1

Have you tested the newest version of Vaadin 8, i.e. 8.13.0?

I am asking this, since this patch was included in 8.12.1

https://github.com/vaadin/framework/pull/12145/files

And it fixes reported regression #12139 which is similar to yours

[janArb]

Hi, yes as you can see in the patch I tested with 8.13.0 and patched revision 5b06221 to verify this. I saw this fix, but didn't understand the idea of the new com.vaadin.client.widget.escalator.ColumnConfiguration#setColumnWidths(java.util.Map<java.lang.Integer,java.lang.Double>, boolean) approach, as this is internal in Escalator in my eyes and cannot be used from outside. But it might very well be a question of how to use the grid correctly, yes. The even more complex dependencies involve a restoring of column weights from persisted user settings via:

    private void setColumnWidthInPercent(int colIdx, int widthInPercent) {
        getColumn(colIdx).ifPresent(column -> {
            String columnId = getColumnId(colIdx);
            columnWidthsInPercent.put(columnId, widthInPercent);
            String styleName = "column-weight-" + widthInPercent;
            columnStyles.put(getColumnId(colIdx), styleName);
            grid.getHeaderRow(0).getCell(column).setStyleName(styleName);
        });
    }

Maybe that has to be done another way to avoid an endless loop?

[Ansku]

Hi @Ansku, could you send me a minimal example template project I could use to try to reproduce this issue?

I'm not quite sure what you are looking for exactly, but https://vaadin.com/vaadin-8/get-started has instructions for creating basic projects with different tools, https://github.com/vaadin/vaadin8-example has an existing example project without a Grid, and https://github.com/vaadin/framework/tree/master/uitest/src/main/java/com/vaadin/tests/components/grid contains a random collection of regression tests UI with different sorts of Grids -- they aren't really meant as code examples and they are only updated when something breaks, so don't expect them to be pretty or to follow best practices, but I'd assume there's something to get you started.

I guess my first question would be: why do you want to call recalculateColumnWidths() in all those situations? For example, Grid should do an automatic column width recalculation when its size changes, and the order of columns shouldn't affect their widths...

[Ansku]

I tested with 8.13.0 and patched revision 5b06221 to verify this.

To clarify: did you test 8.13.0 without the changes in your patch too?

[janArb]

Hi @Ansku, yes of course. I tested 8.12.3, 8.12.4 and 8.13.0 without changes, too.

[janArb]

Hi @Ansku, could you send me a minimal example template project I could use to try to reproduce this issue?

I'm not quite sure what you are looking for exactly, but https://vaadin.com/vaadin-8/get-started has instructions for creating basic projects with different tools, https://github.com/vaadin/vaadin8-example has an existing example project without a Grid, and https://github.com/vaadin/framework/tree/master/uitest/src/main/java/com/vaadin/tests/components/grid contains a random collection of regression tests UI with different sorts of Grids -- they aren't really meant as code examples and they are only updated when something breaks, so don't expect them to be pretty or to follow best practices, but I'd assume there's something to get you started.

I guess my first question would be: why do you want to call recalculateColumnWidths() in all those situations? For example, Grid should do an automatic column width recalculation when its size changes, and the order of columns shouldn't affect their widths...

Because we had certain situations were the automatics didn't work properly. I already deactivated all of this calls and it stays in the endless loop also for only having the restore of the weight changes from user settings. I still try to analyze how to avoid the endless loop from our side, but for this issue here I would argue more generally that an endless loop protection for all theoretical calls of recalculate column widths in connection with the described loop (horizontal bar -> vertical bar -> ...) would really be a good safe guard.

[Ansku]

I would argue more generally that an endless loop protection for all theoretical calls of recalculate column widths in connection with the described loop (horizontal bar -> vertical bar -> ...) would really be a good safe guard

That is sadly easier said than done without getting it needlessly stuck in other situations. But if you can provide me a reproducible example I'll certainly give it another shot :)

[Ansku]

One option that could trigger unwanted resizing is if your theme contains something that affects the column widths. It's generally safer to do settings like that through the code, since the column width calculation logic is quite complicated.

[janArb]

I tried to find the API to do that via code, can you indicate me to the correct methods to do so?

While I still fear the efforts for the minimal example will be beyond my possibilities, I saw the reason for the appearance of the horizontal scrollbar. It's coming from the complicated (I tend to say ugly) calculation logic itself:

com.vaadin.client.widgets.Grid.AutoColumnWidthsRecalculator#applyColumnWidthsWithExpansion

Where in 8.13.0 in line 3630 columns where expanded, although pixelsToDistribute are in my endless loop really negative (-6), this is leading to a scrollbar that is needed. I could successfully patch only this place. After this change the grid is rendered successfully and no loop is freezing the GUI anymore. As the comment below this code also says that further calculations break the GridEditRow test I would suggest to apply my patch to avoid my endless loop case. At least for really negative pixels this should be changed!

Index: client/src/main/java/com/vaadin/client/widgets/Grid.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- client/src/main/java/com/vaadin/client/widgets/Grid.java	(revision 5b06221caab657dc17a8710c4397c1b2ce097a04)
+++ client/src/main/java/com/vaadin/client/widgets/Grid.java	(date 1620294863654)
@@ -3627,7 +3627,7 @@
             if (pixelsToDistribute <= 0 || totalRatios <= 0) {
                 if (pixelsToDistribute <= 0) {
                     // Set column sizes for expanding columns
-                    setColumnSizes(columnSizes, true);
+                    setColumnSizes(columnSizes, false);
                 }
                 /*
                  * If pixelsToDistribute > 0 the element size recalculation

Also the idea for minimal example gets clearer, the grid needs to have space to expand the columns, but the weights must round up to a too large expansion to be in this special case noted above.

[janArb]

I could fix the loop by removing the width settings from CSS and using Column.setWidth() only. For details see https://stackoverflow.com/questions/67474238/vaadin-8-grid-scrollbars-behave-erratically-when-css-is-used-for-column-widths/