Clicking inline editing buttons should not remove selection

[flack]

To reproduce:

• go to http://openpsa.github.io/jsgrid/demos/kitchensink.html
• select a row by clicking into it
• click the edit icon

Expected Result:

• inline editing opens and row stays selected

Actual Result:

• inline editing opens and selection is removed

This currently happens only when multiselect is true, but it will happen in single select, too if we implement #2

I haven't looked at the code yet, but I think we should prevent the click events of the inline buttons from bubbling (and call select explicitly so that we are sure the row is selected)

[OlegKi]

the simple fix can solve the problem.

[flack]

Thanks @OlegKi. I just tried your patch locally, but it doesn't work with the demo mentioned above. From reading the patch, I don't really understand how it is supposed to work anyways, because p.savedRow will be empty, at least the first time you click on the edit icon. But even when I manually select the row again after entering edit mode, clicking on save removes the highlight.

[flack]

P.S.: Also, with your patch, the "selec all" checkbox is no longer working

[OlegKi]

@flack : Thank you! The problem with "select all" checkbox exists because I fixed typing error ("selectAlCheckbox" instead of "selectAllCheckbox") not in all placed of the code. The bug is fixed now.

About the fix which I posted. One should distinguish old jqGrid problems from new problems which exists because of our changing in it. I fixed first of all deselecting the row in single selection mode. It's new problem because of implementing singleSelectClickMode: "toggle" option. Try the demo to see that the new problem is fixed now. I'll improve the code a little more later.

There are exist old jqGrid problem where the row can be unselected if one uses multiselect mode (it's your original example). I will prepare the corresponding fix later. I want just it works with all editing modes.

[flack]

I tried your demo now, and it fixes the immediate problem, i.e. clicking the edit button does not deselect the row, but when you click save or cancel afterwards, the row is still deselected. Maybe it is a matter of taste, but I think interaction with the inline editing buttons should never change the selection status of the row.

EDIT: what I meant was the inline editing buttons should never remove the selection (instead, clicking one of the buttons should always result in the row being selected).

[OlegKi]

@flack I full agree with you in the subject. I'm working now on the fix of all described problems. As I wrote before, the problem is old and it's not so simple. Nevertheless I interpret the behavior as a bug. I already wrote the fix, but I'm in testing of it now. I don't want to post the fix on the fix later...

[OlegKi]

@flack : I posted the fix. Let me known if you would find some scenario where it is not works like expected. I know only some cased in usage Next/Prev buttons in form Editing/View. The problem I plan fix (probably today).

[flack]

@OlegKi I have tested and merged your fixes now. So far, it looks ok. There are two minor irritations I found on http://openpsa.github.io/jsgrid/demos/kitchensink.html:

• when a row is selected, and you click one of the inline editing icons, for a very short time (200ms maybe?) the highlight style is replaced by the hover style. It seems as if the row is shortly deselected and then reselected again. But it only happens sometimes (tested in Firefox)
• when the row is selected (by clicking anywhere), and then I click on the checkbox to deselect it, the row gets deselected, but the checkbox stays checked

[flack]

P.S.: Another thing: When I select a row, and then click on the subgrid expand icon, it gets deselected. This didn't happen before I merged your changes. Is this a regression, a merge problem, or an intentional change?

[OlegKi]

@flack Thank you for your feedback.

I can't see the problem with deselection (for about 200ms) probably because your demo http://openpsa.github.io/jsgrid/demos/kitchensink.html uses jQuery UI Theme which contains not enough contrast colors. I have some problems with my eyes and I can see not so good. So I can neither confirm nor contradict your statement. If the problem exists I would interpret it as a bug which should be fixed too.

Other problems, which you describes (deselected checkbox with selected row and deselecting on expanding subgrid), seems to exist only in your repository. I can reproduce the problem on your demo http://openpsa.github.io/jsgrid/demos/kitchensink.html, but the problem don't exist on any my demos (try http://www.ok-soft-gmbh.com/jqGrid/OK/autoresizeOnLoad2.htm for example).

I made some general changes recently in formatter: "actions". You did another changes (starting with this one) compared with your repository. I suppose that the difference in the changes are the reason of the problems. I personally don't like to have the function activateInlineButtons in base module. In the way you will have the parts of code in base module which depends on implementation details of specific formatter. It's not good in my opinion, but I don't want to criticize your approach.

I introduced pageFinalization of formatter (see here, here, here and here). In the way one have full flexible way to make some finalizations required for specific formatter. One could have for example formatter, which places HTML buttons in every row of the grid and one will call jQuery UI Button to initialize jQuery UI buttons. In the same way I thought about moving the code of onclick, onmouseover and onmouseout from $.fn.fmatter.actions to $.fn.fmatter.actions.pageFinalization, but I din't did it till now. I planed to make clean experiment in the future where I would measure performance and memory requirements (in different web browser) and compare the both implementations. In any way $.fn.fmatter.XXX.pageFinalization is my point of view on solving of some common existing problems in formatters.

Moreover the code which you use have some other problems. For example the code like $('.ui-inline-button').on('mouseover', function(){...}) (see activateInlineButtons) sets mouseover callback on all inline buttons of the page and not just inside of the current grid. If you would have more as one grids on the page you will register the event handler twice for the grid which will be filled the first. Sorting of data in a column of the second grid will follow executing of addJSONData and setting of one more event handler on the first grid and so on. So you current code of activateInlineButtons have clear bug.

[flack]

I have fixed the activateInlineButtons scoping issue now. As to the question why it is in base: I would say that @bouks put it there for the same reason you have put detectRowEditing into base: Because that is where the function is called, and if it were in e.g. common, then we would have a circular dependency.

The pageFinalization thing sounds kind of useful, but the problem is that you did so many refactorings that merging changes is both difficult and risky. So unless it is a bug fix or something extremely useful, I don't think any of us will want to spend a lot of time to resolve and debug merge conflicts

[meh-uk]

I think @flack is right on this I'm afraid. @OlegKi if you want to contribute lots of changes to this project you have to be part of it and fork this repository and be prepared to treat us as equals to yourself.

[OlegKi]

@meh-uk : I don't want to contribute my changes on this project, but I allow you to use my changes in your project at least because of MIT licence. I just answered to @flack . I thought initially that the described problem exists because I implemented singleSelectClickMode: "toggle" suggested by him. Only because of that I posted my answer. In the same way I post sometime comments to Tony's repository or he post his comments to my repository.

So I prefer to answer currently only on direct questions addressed to me (or in case I fill that the question are addressed to me indirectly).

[meh-uk]

That's a shame :(

[OlegKi]

@meh-uk I know now your option, but it's my choice. The more persistently somebody will force me to do something the later (or never) I will do this.

[flack]

Fixed as of now