search

qooxdoo / qooxdoo

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

dblclick event is not fired when showCellFocusIndicator = false (BZ#605) #811

Closed qx-bug-importer closed 8 years ago

qx-bug-importer commented 16 years ago

Denis (nomus) wrote:

dblclick event is not fired/dispatched (at least always) for TreeVirtual if showCellFocusIndicator = false.

Code snippet:

var tree = new qx.ui.treevirtual.TreeVirtual(["Name"]); tree.set({ backgroundColor : "white", border : "inset-thin", left : 10, top : 10, width : "40%", height : "70%" }); tree.setOpenCloseClickSelectsRow(true); tree.setShowCellFocusIndicator(false); tree.setStatusBarVisible(false); var resizeBehavior = tree.getTableColumnModel().getBehavior(); resizeBehavior.set(0, { width:"1*" }); tree.addToDocument(); tree.addEventListener("dblclick", function(oEvent) { alert("dblclick - " + this.getSelectedNodes()[0].label); }, tree);

var dataModel = tree.getDataModel(); var root = dataModel.addBranch(null, "Root", true); var f1 = dataModel.addBranch(root, "Folder 1", false); for (var i = 1; i < 10; i++) { dataModel.addLeaf(f1, "File 1" + i); } var f2 = dataModel.addBranch(root, "Folder 2", false); for (var i = 1; i < 5; i++) { dataModel.addLeaf(f2, "File 2" + i); } dataModel.setData();

Best regards, Dioc

assigned to Derrell Lipman (@derrell)

qx-bug-importer commented 16 years ago

Derrell Lipman (@derrell) wrote:

I'm not able to reproduce this problem. You say that it only sometimes doesn't work. Can you find some sequence of clicks or events that reliably makes it fail? I'll close this ticket for now, but please re-open it if you can find some reproducible sequence. I'd love to fix it if there's really a generic problem.

Derrell

qx-bug-importer commented 16 years ago

Denis (nomus) wrote:

Hello,

I suppose I found a reproducible sequence so I will reopen this bug. If a user double clicks on empty space of row (not to node's label), then dblclick event is fired. But if the user tries to double click on node's label, the event is not fired. If showCellFocusIndicator = true, double clicking causes firing of the event but sometimes with some delay.

Best regards, Dioc

qx-bug-importer commented 16 years ago

Derrell Lipman (@derrell) wrote:

> But if the user tries to double click on node's label, the event is not fired.

Consistently not fired if the double click is on the label? I can't make it fail. Hmmm... What browser are you using? What OS?

Derrell

qx-bug-importer commented 16 years ago

Denis (nomus) wrote:

Yes, the event is never fired if the double click is on a node's label. I am using Windows XP Professional SP2 Russian. The problem is noticed in Firefox 2.0.0.6, Internet Explorer 6, Opera 9.22. Did you test mentioned code snippet?

Dioc

qx-bug-importer commented 16 years ago

Derrell Lipman (@derrell) wrote:

> Yes, the event is never fired if the double click is on a node's label. I am > using Windows XP Professional SP2 Russian. > The problem is noticed in Firefox > 2.0.0.6, Internet Explorer 6, Opera 9.22.

All right. I'm testing on Linux, not Windows which may be a difference. I'll see if I can test on Windows later today.

> Did you test mentioned code snippet?

Yes, that's exactly what I'm using for my tests.

qx-bug-importer commented 16 years ago

Sebastian Werner (@swernerx) wrote:

What is the status of this bug, Derrell? Planned for 0.7.2.

qx-bug-importer commented 16 years ago

Derrell Lipman (@derrell) wrote:

> What is the status of this bug, Derrell? Planned for 0.7.2. >

I haven't had a chance to get back to it. Feel free to pursue, as my "qooxdoo time" is still quite limited.

Derrell

qx-bug-importer commented 16 years ago

Sebastian Werner (@swernerx) wrote:

Mass move of bugs to target 0.7.3

qx-bug-importer commented 16 years ago

Fabian Jakobs (@fjakobs) wrote:

move to 0.7.4

qx-bug-importer commented 16 years ago

Denis (nomus) wrote:

I cannot reproduce this problem in Firefox 2.0.0.14, Opera 9.26 and Safari 3.1.1 by using qooxdoo 0.7.3. But the problem still takes place in IE 7. Dblclick event is not fired when user clicks on a node that is not currently selected. If a node is selected then dblclick event is fired properly.

qx-bug-importer commented 15 years ago

Sebastian Werner (@swernerx) wrote:

TreeVirtual development continues in 0.8.x. To 0.8.1

qx-bug-importer commented 15 years ago

Andreas Ecker (@ecker) wrote:

to 0.8.2.

qx-bug-importer commented 15 years ago

Andreas Ecker (@ecker) wrote:

to 0.8.3 as discussed with Derrell.

qx-bug-importer commented 15 years ago

Derrell Lipman (@derrell) wrote:

Ok, the problem here is that no "dblclick" event is being generated by the DOM in IE7. I'll reassign it to Fabian whose expertise in this area is the eighth wonder of the world. :-)

To reproduce my test results, apply this small patch then run the program that follows the patch. Click on the label in the tree, in a row that is NOT already selected, click on OK if an alert box pops up immediately (it will in FF but not in IE) and then wait 10 seconds for the timer's alert box to appear. In FF it'll display a series of DOM events with mousedown, mouseup, mousedown, mouseup, click, dblclick. In IE, however, it will display mousedown, mouseup, mouseup. I'm not sure why there are two consecutive mouseup events without an intervening mousedown, and note that no click or dblclick events occurred. They're either occurring on some object that isn't listening for these DOM events, or they're not occuring at all.

Derrell

--- a/qooxdoo/framework/source/class/qx/event/handler/Mouse.js
+++ b/qooxdoo/framework/source/class/qx/event/handler/Mouse.js
@ -341,7 +341,7 @ qx.Class.define(&quot;qx.event.handler.Mouse&quot;,
     {
       var type = domEvent.type;
       var target = domEvent.target || domEvent.srcElement;
-
+custom.Application.output.push(&quot;Mouse.js: &quot; + type + &quot; on &quot; + target);
       // Safari (and maybe gecko) takes text nodes as targets for events
       // See: http://www.nczonline.net/archive/2008/2/556
       if (qx.core.Variant.isSet(&quot;qx.client&quot;, &quot;gecko|webkit&quot;))

and then use this test application:

/*

require(qx.log.appender.Native)

require(qx.log.appender.Console)

*/

qx.Class.define("custom.Application", { extend : qx.application.Standalone,

statics : { output : [] },

members : { main : function() { this.base(arguments);

  var tree = new qx.ui.treevirtual.TreeVirtual([&quot;Name&quot;]);
  tree.setOpenCloseClickSelectsRow(true);
  tree.setShowCellFocusIndicator(false);
  tree.setStatusBarVisible(false);

  var resizeBehavior = tree.getTableColumnModel().getBehavior();
  resizeBehavior.set(0, { width:&quot;1*&quot; });

  this.getRoot().add(tree, { edge : 10 });

  tree.addListener(&quot;dblclick&quot;, function(e)
                   {
                     alert(&quot;dblclick - &quot; +
                           this.getSelectedNodes()[0].label);
                   },
                   tree);

  var dataModel = tree.getDataModel();
  var root = dataModel.addBranch(null, &quot;Root&quot;, true);

  var f1 = dataModel.addBranch(root, &quot;Folder 1&quot;, false);
  for (var i = 1; i &lt; 10; i++)
  {
      dataModel.addLeaf(f1, &quot;File 1&quot; + i);
  }

  var f2 = dataModel.addBranch(root, &quot;Folder 2&quot;, false);
  for (var i = 1; i &lt; 5; i++)
  {
      dataModel.addLeaf(f2, &quot;File 2&quot; + i);
  }

  dataModel.setData();

  var timer = qx.util.TimerManager.getInstance();
  timer.start(function(userData, timerId)
              {
                alert(custom.Application.output.join(&quot;\n&quot;));
              },
              0,
              this,
              null,
              10000);
}

} });

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

accept

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

Derrell,

the reason for the IE bug is that the first mouse down event happens on an element, which is removed from the DOM during the "mousedown" event handler. IE cannot handle this case and thus does not fire the "mouseup" and "click" events for this element. The second click is OK but it is not recognized as double click.

The reason for the removal of the first mousedown target is the "alwaysUpdateCells" , which forces a complete update even on selection changes. This complete update is performed on the first click of the double click and causes the trouble.

This bug occurs only in the tree virtual because in the tree this property defaults to "true".

Derrell, does the tree depend on this property? If not we could just deprecate it since it is probably not used widely by table users.

qx-bug-importer commented 15 years ago

Derrell Lipman (@derrell) wrote:

> Derrell, does the tree depend on this property? If not we could just deprecate it since it is probably not used widely by table users.

Unfortunately, I don't think we can simply deprecate it. When a node is added to a tree, two icons can be provided: one for when the node is not selected, and one for when it is selected. The icon must be refreshed when the row is selected, which is why TreeVirtual sets the 'alwaysUpdateCells' property to true.

Derrell

qx-bug-importer commented 15 years ago

Derrell Lipman (@derrell) wrote:

BTW, IIRC, that feature was implemented to copy a similar feature in Tree. The only use I can think of for it would be if you were using the icon to indicate selections, and were disabling the normal selection styling (highlighting). That's reasonable, I suppose, and it's possible someone is doing it.

I'd still be quite hesitant to deprecate a feature that's been in both Table and TreeVirtual for years...

Derrell

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

> I'd still be quite hesitant to deprecate a feature that's been in both Table and > TreeVirtual for years...

Me, too. What about changing the default to "false"?

I don't see how we can fix the dblclick problem if "alwaysUpdateCells" is true. Do you have any idea?

qx-bug-importer commented 15 years ago

Derrell Lipman (@derrell) wrote:

> Me, too. What about changing the default to "false"?

I think I'd be ok with that, A big note should be added to the release notes about the change.

Hmmm... What about having it default to false unless a selectedIcon is provided (meaning they need the feature that turning on this property was intended to provide), and setting it to true only in that case?

Derrell

qx-bug-importer commented 15 years ago

Derrell Lipman (@derrell) wrote:

Nope, scratch that. It'll be nasty. Each time the model data is changed we'd have to reapply the property or not do so but risk needing to have done so. I think maybe we should just document what won't work properly if they specify a separate selected icon, and let them set the property to true.

Let me know your thoughts. I can make the changes if you'd like.

Derrell

qx-bug-importer commented 15 years ago

Fabian Jakobs (@fjakobs) wrote:

Sounds good to me. I'll assign it back to you.

qx-bug-importer commented 15 years ago

Derrell Lipman (@derrell) wrote:

Fixed with r18438 and r18439 by leaving the default value of 'alwaysUpdateCells' at Table's default of false. If users want to be able to provide a unique icon for display in a TreeVirtual they'll now have to manually set this property to true, but incur a failure of double click events in IE and also of immediate selection of a row upon right-click (and sometimes left-click too).

I've reassigned this bug to Andreas to create an entry in the next Release Notes regarding this change.

Derrell

qx-bug-importer commented 14 years ago

Andreas Ecker (@ecker) wrote:

Added a section to the preliminary 0.8.3 release notes as suggested, http://qooxdoo.org/about/release_notes/0.8.3 . Unfortunately, there doesn't seem to be a solution possible without drawbacks for all situations ...

Derrell, I think it would be the best that you (not me) added some info to the appropriate JSdocs of TreeVirtual, and close the bug afterwards, thanks.

qx-bug-importer commented 14 years ago

Derrell Lipman (@derrell) wrote:

I just confirmed that I had already added appropriate notes in the API documentation. We're set to go. Closing this bug.

Derrell

qx-bug-importer commented 10 years ago

Martin Wittemann (@wittemann) wrote:

Closed all bugs already shipped with a release.