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

Table should stopEditing when cell editor loses focus #9504

Open
milandamen opened this Issue Mar 9, 2018 · 22 comments

Comments

Projects
None yet
4 participants
@milandamen
Contributor

milandamen commented Mar 9, 2018

When I click outside of the table, the table doesn't stop or cancel editing, thus the cell editor will still be visible and the value of the cell editor will not have been committed to the table.

Is this intended behavior, or is this a bug? How should I go about implementing this behavior?

@johnspackman

This comment has been minimized.

Member

johnspackman commented Mar 9, 2018

You're looking for the myTable.stopEditing() function

@milandamen

This comment has been minimized.

Contributor

milandamen commented Mar 9, 2018

@johnspackman I know, but this function should be called automatically when the cell editor and table loses focus, as it does when you click anywhere else in the table.

@johnspackman

This comment has been minimized.

Member

johnspackman commented Mar 9, 2018

I agree, it should - you need to trap the "blur" event and then call stopEditing from there. If you want to make a PR that would be great 👍 :)

@milandamen

This comment has been minimized.

Contributor

milandamen commented Mar 9, 2018

The problem that I get, is that the blur event gets called after all regular events have been done.

When I add a blur listener to the cell editor (which I can't access through the table or table scroller) and do a stopEditing on that event:

  • when I click outside the table, it correctly stops editing, but
  • when I do stopEditing, focus a different cell, and startEditing, all in the same thread (by using the Enter key for example), the table stops editing after the new cell editor has started editing, because the blur event happens after all of this, not inbetween the stopEditing and startEditing.

I don't really know how to solve this problem.

@johnspackman

This comment has been minimized.

Member

johnspackman commented Mar 9, 2018

funny I thought I'd solved this in my apps but I've just tested one and it's not there!

It would worth a quick test to see if a blur handler on the table would work, but I suspect that when the cell editor pops up that would cause the table to "blur".

So if blur on the table doesn't work, you'd need to modify the table class so that when a click or whatever it is that calls startEditing on another cell, it sets a flag to say that's what it is about to do; this flag would be used to avoid calling stopEditing in the cell editors "blur" handler.

@milandamen

This comment has been minimized.

Contributor

milandamen commented Mar 9, 2018

Yeah I've checked if a blur handler on the table would work, but it is as you say, the table blurs when the cell editor opens.

@level420

This comment has been minimized.

Member

level420 commented Mar 9, 2018

wouldn't it be best to listen to the blur event of the cell editor?

@milandamen

This comment has been minimized.

Contributor

milandamen commented Mar 9, 2018

That blur event gets fired after stopEditing and startEditing have been called, so if I edit a different cell using code, the table will still stop editing if I use the blur event. It also it is not possible to get the current cell editor from the table or table scroller.

@cboulanger

This comment has been minimized.

Contributor

cboulanger commented Mar 9, 2018

You should be able to get the current cell editor with something like

var x = table.getTableColumnModel().getVisibleX( currentColumn );
var metaColumn = table._getMetaColumnAtColumnX(x);
var cellEditor = table.getPaneScroller(metaColumn)._cellEditor;

(untested). This is, of course, horrible code and should get a proper getter.

@level420

This comment has been minimized.

Member

level420 commented Mar 9, 2018

@milandamen you could try to attach the listener for the cell editors blur event somewhere here: https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/ui/table/pane/Scroller.js#L1908
Try using a addListenerOnce which should be removed in case that editing is started for another cell. A possible position to do this is in https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/ui/table/pane/Scroller.js#L1921
If the table looses the focus, then the blur event handler should call qx.ui.table.pane.Scroller.stopEditing.
If the cell editor looses focus because another cell is going into editing mode, then the event handler should be removed in StopEditing before it is invoked, thus allowing to start editing on the other cell.
To have this working you have to store the event handler id somewhere, best in the cell editor instance.

Also not tested, but I think worth considering.

@milandamen

This comment has been minimized.

Contributor

milandamen commented Mar 9, 2018

I like your idea a lot.

One problem with that though, is that the table never has focus when the scroller is editing, so listening to focusout on the table would not accomplish it.

Also, wouldn't it be better to keep the handler id in the scroller instead of the cell editor instance? The cell editor shouldn't know about when it should remove a listener; also, each different cell editor would need to have this code then.

@level420

This comment has been minimized.

Member

level420 commented Mar 9, 2018

@milandamen could you please try the following playground sample code: https://gist.github.com/level420/247c4a8e89be9d2a447836db9a425f40
Please paste it from there into http://www.qooxdoo.org/current/playground
The column "A number" is editable

@level420

This comment has been minimized.

Member

level420 commented Mar 9, 2018

@milandamen the idea is not to listen for events of table or scroller, but to listen for the blur event of the cell editor!

@level420

This comment has been minimized.

Member

level420 commented Mar 9, 2018

Some explanations regarding my gist: the class qx.ui.table.pane.ScrollerMod is a full copy of the original class qx.ui.table.pane.Scroller with a single addition of a blur event handler which stops editing. See https://gist.github.com/level420/247c4a8e89be9d2a447836db9a425f40#file-playground-js-L1877-L1879
The full copy was needed because I'm not able to override qx.ui.table.pane.Scroller.startEditing because it uses private members.
The gist was necessary as the resulting url in the playground is to long for shortening.

@level420

This comment has been minimized.

Member

level420 commented Mar 9, 2018

updated my gist which now only stops editing on blur if the cell editor did not change. this avoids stopping editing when having consecutive edits of cells by hitting return

@milandamen

This comment has been minimized.

Contributor

milandamen commented Mar 12, 2018

@level420 I have simplified your code and added some stuff: https://gist.github.com/milandamen/d288453491f49923e3c5d6857ee19747

However, just like in your code, the table stops editing when you press Enter, which is unintended. But it works fine when using the browser debugger to break on the 3 methods. I think it has something to do with native focus handling; see the following stacktrace:

004075 qx.ui.table.pane.ScrollerMod[96-0]: stacktrace
qx.log.appender.Native.process() @ Native.js:58
qx.log.Logger.__log() @ Logger.js:597
qx.log.Logger.error() @ Logger.js:299
qx.core.Object.prototype.__logMessage() @ MLogging.js:101
qx.core.Object.prototype.error() @ MLogging.js:74
qx.ui.table.pane.ScrollerMod.prototype._onBlurCellEditorStopEditing() @ Application.js:137
callback @ MEvent.js:82
qx.ui.core.EventHandler.prototype._dispatchEvent() @ EventHandler.js:315
qx.event.dispatch.Direct.prototype.dispatchEvent() @ Direct.js:135
wrappedFunction @ Interface.js:537
qx.event.Manager.prototype.dispatchEvent() @ Manager.js:887
qx.event.Registration.dispatchEvent() @ Registration.js:272
qx.event.handler.Focus.prototype.__fireEvent() @ Focus.js:343
qx.event.handler.Focus.prototype._applyFocus() @ Focus.js:1265
qx.event.handler.Focus.prototype.resetFocus() @ VM21066:3
default @ Focus.js:751
qx.event.handler.Focus.prototype.__onNativeFocusOut() @ GlobalError.js:133
(anonymous) @ Function.js:337
(anonymous) @ Focus.js:260
setTimeout (async)
qx.event.handler.Focus.prototype.focus() @ Focus.js:257
qx.bom.Element.focus() @ Element.js:129
qx.html.Element.flush() @ Element.js:350
(anonymous) @ Manager.js:176
qx.ui.core.queue.Manager.__executeAndRescheduleOnError() @ Manager.js:222
qx.ui.core.queue.Manager.flush() @ Manager.js:97
qx.event.dispatch.Direct.prototype.dispatchEvent() @ Direct.js:135
wrappedFunction @ Interface.js:537
qx.event.Manager.prototype.dispatchEvent() @ Manager.js:887
qx.event.Registration.__fireEvent() @ Registration.js:302
qx.event.Registration.fireEvent() @ Registration.js:321
qx.event.handler.Keyboard.prototype._fireSequenceEvent() @ Keyboard.js:219
qx.event.handler.Keyboard.prototype._fireSequenceEvent() @ Keyboard.js:211
qx.event.handler.Keyboard.prototype._idealKeyHandler() @ Keyboard.js:539
webkit @ Keyboard.js:377
qx.event.handler.Keyboard.prototype.__onKeyUpDown() @ GlobalError.js:133
(anonymous) @ Function.js:337

For some reason, it resets the focus, which fires blur on the cell editor. Do you have any idea why this happens?

@level420

This comment has been minimized.

Member

level420 commented Mar 12, 2018

@milandamen when using your gist in playground with my browser (chrome 65 on windows) it does not stop editing on hitting return:

  1. Double click cell in row 1, column "A number". The cell enters editing mode.
  2. Hit return or enter. As a result cell in row1, column "A number" looses focus and cell in row 2, column "A number" is entering edit mode.

Does your browser react different?

@milandamen

This comment has been minimized.

Contributor

milandamen commented Mar 12, 2018

@level420 I have tried using Chrome 65.0.3325.146 and Firefox 58.0.2 and they both don't work. I'm using Linux Mint.

I am using the master version (qooxdoo.org/devel/playground) and the master version in a separate application. When I try in the old playground (qooxdoo.org/current/playground) it does work correctly. It seems something changed between 2016-09-19 and now.

@level420

This comment has been minimized.

Member

level420 commented Mar 12, 2018

@milandamen you're right!

I've accidently used the current playground and the gists do work with it, but not with the devel playground.

I've tried to modify my gist so that the blur event handler only gets attached to the cell editor if the cell editor already got the focus by listening to the focusin event and decouple the addition of the blur event handler from the current event queue run by using a qx.event.Timer.

I've modified your gist with this code. I don't think that tracking the event listener id and removing the listener is needed, as the cell editor get's instantiated for each edit and then destroyed afterwards.

Please try https://gist.github.com/level420/247c4a8e89be9d2a447836db9a425f40

@milandamen

This comment has been minimized.

Contributor

milandamen commented Mar 13, 2018

Thank you for the help, it works as expected. Even when adding a button that gets the data from the table, it gets the correct (new) data.

Should I add this code to the qx.ui.table.pane.Scroller class and make a pull-request? Because in my opinion this feature should be default behavior. People that don't want this feature could override _onBlurCellEditorStopEditing or _onFocusinCellEditorAddBlurListener.

@level420

This comment has been minimized.

Member

level420 commented Mar 13, 2018

@milandamen sadly my solution does not work with IE11 and win7. when clicking outside the table but on the background behind the table the cell editor does not get the blur event. But this may be due to the special behavior in the playground and should be tested in a real application. It works as expected if clicking outside e.g. into the source editor of the playground.

Regarding the PR: I'd propose to add a property which may be set in order to activate the new behavior by having values for do nothing, accept edit or cancel edit with do nothing as default which is the behavior without the PR. The current gist works like accept edit but there may be people wanting the edition of the value to be canceled if the table is loosing focus.

And please create that PR as I'm currently not using editable cells in my apps. It's always good to have someone using the proposed PR. Thank you.

@milandamen

This comment has been minimized.

Contributor

milandamen commented Mar 16, 2018

@level420 I have now implemented this into the Scroller class in Qooxdoo, but i came across a weird problem regarding the combobox cell editor:

  • in Firefox, the functionality works without issue, the combobox opens correctly, but
  • in Chrome, the table immediately stops editing when the "A string" field is double-clicked. The code somehow detects a blur event and closes the editor..

I've tried tweaking the timer setting but it doesn't help.

You can reproduce it in this gist: https://gist.github.com/milandamen/d3428ec8deb8b5efed1454ac58fb99c1

I would appreciate your help a lot.

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