qooxdoo / qooxdoo

qooxdoo - Universal JavaScript Framework
http://qooxdoo.org
Other
764 stars 260 forks source link

Table can block the browser (BZ#2262) #2401

Closed qx-bug-importer closed 8 years ago

qx-bug-importer commented 15 years ago

Jean-Baptiste Briaud -- Novlog (j-b.briaud) wrote:

As a summary : this was reproductible under FF Vista, XP, MacOS. This happen when the lines in a table fit exactly the graphical space availlable. The height the table must have to reproduce the bug depends on OS and probably configuration. So far : 100 pixels for 3 lines under all Windows tested and 98 on Mac.

Note : lines can be more or less than 3 (in the 100 pixels example) it will be OK. the bug happen when 3 and only 3 lines are there for 100 pixels height table. Columns doesn't matter.

Yesterday, we found that "sometime" the browser could be blocked (sometime we also got the "script take too long dialog) when user click inside a table. This bug turn us mad for few hours. We manage to get a scenario that always reproduce the bug : Under FF/win vista, After having added 3 lines in the table, clicking on any of this lines block the browser. No way to reproduce the bug in FF on Mac and on XP ! (I told you, it turn us mad).

First, the result look like incredible, but I then remember on that table, I set the height to 100 pixels. I change that to 101 pixel and the bug was no more reproductible.

We then noticed that the bug happened when the table had lines in it that filled exactly the graphical space availlable. In FF/win Vista, it happen for 100 pixels and 3 lines. So 100 pixels = 3 lines + header + bottom summary => 20 pixels per lines.

We try to fix the height to 80 and the bug happen for 2 lines under FF/win Vista.

To reproduce it on FF/mac for 3 lines, I had to set the height to 98.

Any idea ? Is it a qx bug ? If yes, what could we do to help ? Can one of u reproduce the bug in existing code ? I'll try to reproduce the bug in a minimal set of code as it might also be in the way we add lines.

Here is the way I set the height :

qx.Class.define("novlog.fwk.ui.table.Table", {

extend : qx.ui.table.Table,

construct : function(dataModel) { this.base(arguments, dataModel); this.set({ height: 100 }); } });

assigned to Fabian Jakobs (@fjakobs)

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

I'm not able to reproduce this bug. This is my test code:

  // Create the initial data
  var rowData = [
    [1],  [2],  [3]
  ];

  // table model
  var tableModel = this._tableModel = new qx.ui.table.model.Simple();
  tableModel.setColumns([ "ID"]);
  tableModel.setData(rowData);

  // table
  var table = new qx.ui.table.Table(tableModel);
  this.getRoot().add(table)

  table.set({
    height: 98
  });

You simply paste it into the playground <http://demo.qooxdoo.org/devel/playground&gt;.

Are you able to change this code to make it break?

qx-bug-importer commented 15 years ago

Jean-Baptiste Briaud -- Novlog (j-b.briaud) wrote:

I'm not able to make the bug happen in the playground. I upgraded to FF 3.0.9 and still able to reproduce it 100% locally. There is common thing : simple table, SimpleModel, the way data is added, ... but there are few differences :

I take it and come back here with a minimal code snipset to reproduce the bug.

qx-bug-importer commented 15 years ago

Jean-Baptiste Briaud -- Novlog (j-b.briaud) wrote:

I take it and come back here with a minimal code snipset to reproduce the bug.

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

thanks

qx-bug-importer commented 15 years ago

Jean-Baptiste Briaud -- Novlog (j-b.briaud) wrote:

It is quite complicated to extract the minimal set of code to reproduce the bug. I'm now able to bypass our backend to reproduce it 100% but I can't reproduce it if I totally bypass the request object. I have to add the data to the model inside our "request framework". Our "request framework" is a big word to name few classes and method to encapsulate call to the backend using the io.request object, marshal/unmarshal the json string from HTTP query and do some graphical stuff like progress indicator. => it might be related to thread.

I also notices that each time the bug happen, the GUI is frozen and the table doesn't have scrollbar. Each time the bug do not happen, the table have scrollbar and there is a "virtual" last empty line that can be seen by using the scrollbar. When the bug happen, the last line is not empty and nothing can move.

Code is still too big to be submited here, I continue to work on extracting the minimal set of code. Ideas on where to look on are welcome :-)

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

Maybe you can mimic the request by using a setTimeout call. The asynchronism could trigger the bug.

qx-bug-importer commented 15 years ago

Massimo Petrillo (mappopo) wrote:

Maybe it can help.

I have the same problem reported by Jean-Baptiste. It happens only at certain browser windows size, while a slight change in the dimension of window does not show any problem. I also tried to reproduce the problem in a minimal code snipset, with no success. But I may be noticeable than:

  1. When the block is shown, Internet Explorer reports an error on “qx.ui.decoration.Background”, method “resize” where the parameter “height” results to be NaN (Firefox seems to be in a loop and does not report any error).
  2. In my case, the problem happens only if the table is in maximized qx.ui.window.Window.
qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

That's a starting point. The height should never be NaN. If you have IE8 you can turn on the debugger and add this line to qx.ui.decoration.Background.resize():

if (isNaN(height)) { debugger; }

If the debugger breaks at this line you can inspect the call stack and see, where height gets this invalid value.

qx-bug-importer commented 15 years ago

Massimo Petrillo (mappopo) wrote:

Fabian, I failed to find the reason real of the problem. I was able to replicate it only in the build version, which is very hard to debug. However it is origineted in the method "renderLayout" of "qx.ui.layout.Grid". The following patch seems to work for me

      var cellWidth = Math.max(cellHint.minWidth, Math.min(spanWidth-marginLeft-marginRight, cellHint.maxWidth));
      var cellHeight = Math.max(cellHint.minHeight, Math.min(spanHeight-marginTop-marginBottom, cellHint.maxHeight));
      if (isNaN(cellHeight)) { 
        //alert(&quot;cellHint.minHeight&quot;+cellHint.minHeight)
        cellHeight =cellHint.minHeight+10;
      }
      var cellAlign = this.getCellAlign(row, col);
      var cellLeft = left + Util.computeHorizontalAlignOffset(cellAlign.hAlign, cellWidth, spanWidth, marginLeft, marginRight);
      var cellTop = top + Util.computeVerticalAlignOffset(cellAlign.vAlign, cellHeight, spanHeight, marginTop, marginBottom);

where my code is only

      if (isNaN(cellHeight)) { 
        //alert(&quot;cellHint.minHeight&quot;+cellHint.minHeight)
        cellHeight =cellHint.minHeight+10;
      }

if I remove "+10" an infinite loop seems to start. The commented "alert" was used to be sure there is a NaN value. I hope you can now understand why there is NaN value.

 Jean-Baptiste,

Please try it in your application 
qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

For "cellHeight" to become undefined one of cellHint.minHeight, spanHeight, marginTop, marginBottom or cellHint.maxHeight must have an invalid value. Can you post the values of these fields if cellHeight is NaN?

If you can't reproduce it in a demo app, do you see a way to give me access to your app running on your server so I can try to debug it remotely?

qx-bug-importer commented 15 years ago

Massimo Petrillo (mappopo) wrote:

I have some other detail, all seems due to

  var rowHeights = [];

  for (var row=0; row&lt;=maxRowIndex; row++)
  {
  try {
    offset = rowStretchOffsets[row] ? rowStretchOffsets[row].offset : 0;
    rowHeights[row] = prefHeights[row].height + offset;
    } catch(e) {
    }
  }

or somethere earlyer. rowStretchOffsets[row] and prefHeights[row] are both undefined.

My patch somethime leads to an infinite loop.

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

Its OK for rowStretchOffsets[row] to be undefined but prefHeights[row] must always be defined.

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

The but might be related to a buggy caching in the grid layout. I have changed the implementation a little in r18907. Could both of you please try this revision?

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

Reverted some changes from the last commit since it broke the rendering. (r18909)

qx-bug-importer commented 15 years ago

Jean-Baptiste Briaud -- Novlog (j-b.briaud) wrote:

The workaround is very simple : let the table have a height of 101 instead of 100. As we are very very short on time (that is start'up life ...) I, unfortunatly, won't have any more time to investigate that bug as I was not able to get that small line of code after hours of work as my last comment explain. So, I'm reasigning Fabian to let him do the best for qooxdoo with that bug.

qx-bug-importer commented 15 years ago

Massimo Petrillo (mappopo) wrote:

> The workaround is very simple :

It's not always so simple. In some cases the table (or the treevirtual, as in mine) resizes according to the browser window. In such case the block (or the infinite loop of r18909) conontinue to be shown.

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

increase priority

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

With Massimo's test case I was able to reproduce the problem locally. I think I have finally found the problem. Could both of you please check, whether it is fixed in rev. 19053?

qx-bug-importer commented 15 years ago

Massimo Petrillo (mappopo) wrote:

Fabian, I pasted the rows from 270 and 271 in qx.ui.layout.Grid and everything is ok. I will corfirm it as sonn as possible.

qx-bug-importer commented 15 years ago

Massimo Petrillo (mappopo) wrote:

:'-(

Even if they are more seldom, some new troubles seems to be present. They happen only in my application in stressed context when the user clicks as a madman on "Allestimento" section. I this case the scrollbar in "storico selezione" disappear even if there are enough rows to need it. A little more seldom, there is an infinite loop or something similar. I am going to investigate if it happens even in less stressed context and if it depends on my application rather than qooxdoo.

I also downloaded version 19053.

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

At least we are now one step further.

If you have any chance to reproduce it in a skeleton app again, this would be really helpful. Debugging such a complex bug in the online build version is just too hard.

qx-bug-importer commented 15 years ago

Massimo Petrillo (mappopo) wrote:

It is very hard to find a minimal test case for the infinite loop.

In my trial, however I find he method setData of table model raises many "resize" events. I suppose every "resize" runs a "renderLayout". Maybe it can be improved and the result can heavely affect perfomance with slower browsers like MS Internet Explorer.

qx-bug-importer commented 15 years ago

Massimo Petrillo (mappopo) wrote:

I was (partially) wrong. The resize events start only after setData ends. They start many times only when the vertical scrollbar appear the first time. In all other cases "resize" events is raised just once (when real resizing take place) or is not raised at all (when no scrollbar is nedded after setData or the scrollbar already appeared). I am going to send a new test case, it do not show the infinite loop but can be a new step further.

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

I have added a fix for the 50+ resize calls in r19142. I'm not sure whether this fixes all you issues but it should be better now. Please let me know if you still have issues.

qx-bug-importer commented 15 years ago

Massimo Petrillo (mappopo) wrote:

Fabian, I get the error "dH[i].setPaneWidth is not a function". I can not investigate more for a couple of days. As soon as possible I will send you further detail.

qx-bug-importer commented 15 years ago

Massimo Petrillo (mappopo) wrote:

Fabian, don't worry. It was a problem of mine.

Next week I hope to send you a definitive answer about the block.

qx-bug-importer commented 15 years ago

Massimo Petrillo (mappopo) wrote:

I sent a new test case

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

some more fixes in r19289.

qx-bug-importer commented 14 years ago

Massimo Petrillo (mappopo) wrote:

> some more fixes in r19289. >

With 19512 the original bug is not fixed.

The workaround is to force always the vertical scrolbar to be visible as in this patch of qx.ui.table.Table

_updateScrollBarVisibility : function() { if (!this.getBounds()) { return; }

  var horBar = qx.ui.table.pane.Scroller.HORIZONTAL_SCROLLBAR;
  var verBar = qx.ui.table.pane.Scroller.VERTICAL_SCROLLBAR;
  var scrollerArr = this._getPaneScrollerArr();

  // Check which scroll bars are needed
  var horNeeded = false;
  var verNeeded = false;

  for (var i=0; i&lt;scrollerArr.length; i++)
  {
    var isLast = (i == (scrollerArr.length - 1));

    // Only show the last vertical scrollbar
    var bars = scrollerArr[i].getNeededScrollBars(horNeeded, !isLast);

    if (bars &amp; horBar) {
      horNeeded = true;
    }

    if (isLast &amp;&amp; (bars &amp; verBar)) {
      verNeeded = true;
    }
  }

var verNeeded = true; //THIS IS THE NEW LINE

  // Set the needed scrollbars
  for (var i=0; i&lt;scrollerArr.length; i++)
  {
    var isLast = (i == (scrollerArr.length - 1));
    var bHadVerticalScrollBar;

    // Only show the last vertical scrollbar
    scrollerArr[i].setHorizontalScrollBarVisible(horNeeded);

    // If this is the last meta-column...
    if (isLast)
    {
      // ... then get the current (old) use of vertical scroll bar
      bHadVerticalScrollBar = scrollerArr[i].getVerticalScrollBarVisible();
    }

    scrollerArr[i].setVerticalScrollBarVisible(isLast &amp;&amp; verNeeded);

    // If this is the last meta-column and the use of a vertical scroll bar
    // has changed...
    if (isLast &amp;&amp; verNeeded != bHadVerticalScrollBar)
    {
      // ... then dispatch an event to any awaiting listeners
      this.fireDataEvent(&quot;verticalScrollBarChanged&quot;, verNeeded);
    }
  }
},

I can not try to find a new test case, at least for a while. But I hope the workaround show you the way to find the right solution.

qx-bug-importer commented 14 years ago

vvandens@gmail.com (vvandens) wrote:

The following snippet reproduces the problem in the 0.8.3 playground. Run it, then pull-up the splitpane separator until the scrollbar should appear on the topmost table :

var data = [ ["cell11","cell12"], ["cell21","cell22"] ];

var tm = new qx.ui.table.model.Simple(); tm.setData(data); tm.setColumns([ "col1", "col2"]);

var t1 = new qx.ui.table.Table(tm); var t2 = new qx.ui.table.Table(tm);

// Create a vertical split pane with the 2 tables var sp = new qx.ui.splitpane.Pane("vertical").set({ width : 450, height : 300 });

sp.add(t1); sp.add(t2);

var doc = this.getRoot();

doc.add(sp, { left : 100, top : 50 });

qx-bug-importer commented 14 years ago

Massimo Petrillo (mappopo) wrote:

Vincent, if you patch qooxdoo as in http://bugzilla.qooxdoo.org/show_bug.cgi?id=2262#c29 it works, but vertical scrollbars are always visible. For me it is not too bad, but maybe the fix of this issue will speed up the creation of tables, and it will be very nice.

qx-bug-importer commented 14 years ago

Fabian Jakobs (@fjakobs) wrote:

The snippet seems to work in the current devel version:

http://demo.qooxdoo.org/devel/playground/#%7B%22code%22%3A%20%22var%2520data%2520%253D%2520%255B%250A%255B%2522cell11%2522%252C%2522cell12%2522%255D%252C%250A%255B%2522cell21%2522%252C%2522cell22%2522%255D%250A%255D%253B%250A%250Avar%2520tm%2520%253D%2520new%2520qx.ui.table.model.Simple%28%29%253B%250Atm.setData%28data%29%253B%250Atm.setColumns%28%255B%2520%2522col1%2522%252C%2520%2522col2%2522%255D%29%253B%250A%250Avar%2520t1%2520%253D%2520new%2520qx.ui.table.Table%28tm%29.set%28%257B%250A%2520%2520minHeight%253A%25205%250A%257D%29%250Avar%2520t2%2520%253D%2520new%2520qx.ui.table.Table%28tm%29%253B%250A%250A%250Avar%2520sp%2520%253D%2520new%2520qx.ui.splitpane.Pane%28%2522vertical%2522%29.set%28%257B%250Awidth%2520%253A%2520450%252C%250Aheight%2520%253A%2520300%250A%257D%29%253B%250A%250Asp.add%28t1%29%253B%250Asp.add%28t2%29%253B%250A%250Avar%2520doc%2520%253D%2520this.getRoot%28%29%253B%250A%250Adoc.add%28sp%252C%250A%257B%250Aleft%2520%253A%2520100%252C%250Atop%2520%253A%252050%250A%257D%29%253B%22%7D

qx-bug-importer commented 14 years ago

Fabian Jakobs (@fjakobs) wrote:

The 0.8.3 release shows the bug:

http://demo.qooxdoo.org/0.8.3/playground/#%7B%22code%22%3A%20%22var%2520data%2520%253D%2520%255B%250A%255B%2522cell11%2522%252C%2522cell12%2522%255D%252C%250A%255B%2522cell21%2522%252C%2522cell22%2522%255D%250A%255D%253B%250A%250Avar%2520tm%2520%253D%2520new%2520qx.ui.table.model.Simple%28%29%253B%250Atm.setData%28data%29%253B%250Atm.setColumns%28%255B%2520%2522col1%2522%252C%2520%2522col2%2522%255D%29%253B%250A%250Avar%2520t1%2520%253D%2520new%2520qx.ui.table.Table%28tm%29.set%28%257B%250A%2520%2520minHeight%253A%25205%250A%257D%29%250Avar%2520t2%2520%253D%2520new%2520qx.ui.table.Table%28tm%29%253B%250A%250A%250Avar%2520sp%2520%253D%2520new%2520qx.ui.splitpane.Pane%28%2522vertical%2522%29.set%28%257B%250Awidth%2520%253A%2520450%252C%250Aheight%2520%253A%2520300%250A%257D%29%253B%250A%250Asp.add%28t1%29%253B%250Asp.add%28t2%29%253B%250A%250Avar%2520doc%2520%253D%2520this.getRoot%28%29%253B%250A%250Adoc.add%28sp%252C%250A%257B%250Aleft%2520%253A%2520100%252C%250Atop%2520%253A%252050%250A%257D%29%253B%22%7D

qx-bug-importer commented 14 years ago

Andreas Ecker (@ecker) wrote:

mass renaming of 0.9 target to 1.0-beta1

qx-bug-importer commented 10 years ago

Martin Wittemann (@wittemann) wrote:

Closed all bugs already shipped with a release.