optikalefx / OpenJS-Grid

OpenJS Grid is the easiest jQuery Grid ever. With very little work you can have a data grid that can do everything from sorting and searching to complex database queries. Best of all, its open source. So you can learn how it's all done.
http://square-bracket.com/openjs
MIT License
96 stars 46 forks source link

Text after single quotation mark not displaying #53

Open Miqi180 opened 10 years ago

Miqi180 commented 10 years ago

Hi,

I've noticed that text after single quotes (') is not showing in the grid. You can save such a string correctly in the DB from the grid and the whole string is included in the JSON returned from the server when then grid is rendered next time, but the string is then truncated. For instance, try saving the string "Patricia's car" and only "Patricia" will display the next time the grid is rendered.

Any idea how to fix this?

Thanks.

Miqi180 commented 10 years ago

Hi again,

After reviewing the php code, I managed to fix this issue the following way:

In the grid.php load function add the following 4 lines of code before adding the cell content to the multidimensional array:

            if (strpos($cell, "'")) {
                $_c = str_replace("'", "&#39", $cell); 
                $cell = $_c;
            }

Then add to the array as usual:

$data['rows']["_".$key][$col] = utf8_encode($cell);

I suppose you could use preg_replace() here instead, if more characters turn out to be a problem, but for now this seems to suffice. Text with single quotes is still being stored correctly in the DB after saving from the grid and everything seems to work correctly this way.

optikalefx commented 10 years ago

This is odd because this bug should have been fixed a long time ago. I just tested with the version live on square-bracket.com/openjs and it's fine.

see this line http://cl.ly/image/0n2S3Y06421Q which is this database item http://cl.ly/image/2v0x1T0e1Y0J

So I wonder what is special about your apostrophe. Hmmm

Miqi180 commented 10 years ago

Weird indeed, so let's dig a little deeper.

1) It's not a browser issue this time, because I checked in both FF and Chrome (both latest versions) and it produces the same result for me.

2) The truncated text occurs on both my own site and on square-bracket. (The previous "Patricia example" was actually from your site).

3) The problem character is single quote (keydown keycode 222, '), not apostrophe (’) which is not a standard ASCII quote (if it's hard to spot the difference, try zooming in). For easy reference: http://www.javascripter.net/faq/keycodes.htm

4) This doesn't appear to be an encoding issue. I tried changing to charset=UTF-8 from iso-8859-1 which I normally use, but to no avail.

I wonder if this only occurs on my system, since you can't seem to reproduce the problem? Or maybe you used another type of single quotation mark? Hard to tell from the pics... :)

optikalefx commented 10 years ago

So was your character a "fancy" quote? I never use anything but ' and " for all quote types. The web generally hates fancy quotes, “ ” vs " " and ‘ ’ vs ' '. I'm guessing you had the fancy curled single quote, in which I would call that bad data.

Now that's not to say the grid shouldn't be able to handle it, but in my career, I go through a lot of trouble to clean all curly quotes from input and make sure they are all standard ascii quotes.

Miqi180 commented 10 years ago

No, there's nothing curly or fancy about the problem character in question, the single quote (keydown code 222, dec 39 in ASCII). (For easy reference: http://www.ascii-code.com/). It is completely standard and straight. However, I do agree with the rest of your points. Generally, I simply parse user input and replace with standard chars if anything fancy is found, but the single quote is as standard as it gets and sometimes necessary.

optikalefx commented 10 years ago

Key code 222 as seen on this site http://www.cambiaresearch.com/articles/15/javascript-char-codes-key-codes by typing ' is allowed. That's what I used in my example above.

If you pull up SQB right now does the line item show up just fine? Or was this a past example grid?

Miqi180 commented 10 years ago

I just tried again on SQB (editing table) and noticed the following (after page refresh!). edit example

The single quote string is obviously stored correctly, but for whatever reason the DB data seems to render differently depending on table type. The editing table seems to behave differently from the others.

Edit: Just to be clear: the problem persists, but appears only to be linked to the editing table. Don't know if that point came across previously..

optikalefx commented 10 years ago

AH, so it display fine if it's NOT in a textbox. But when it has to display in a textbox, that's when it fails. Which is a funny problem

http://cl.ly/image/1S1W3u0X3j0Z

What's happening is that these textboxes are "plugins" in my system which are defined by strings surrounded by single quotes. So when this value is brought in with single quotes, it breaks the string and can't display inside the value="" of the input box.

That's where we need to fix this, in grid.js when we put in the input value of the text box.

Miqi180 commented 10 years ago

.Èxcellent, mystery solved then.

So you prefer to fix this on the client side because not all grids contain textboxes and hence it's slightly more efficient, or...?

I mean, essentially what you need to do in jQuery is pretty much the same I did in php. Unless of course my fix causes problems when such strings are displayed normally (i.e. not in textboxes), which is hard to believe. (Haven't tested it, though).

And how do you make those fancy little image snippets? :)

optikalefx commented 10 years ago

Well it is strictly a client side display issue. All that's happening is that because i'm setting the value="" property of the textbox, i'm not accounting for quotes there. So I can look at that next week, should be a simple fix. I don't think i'll a string replace, I think the right move is to handle the character escapes.

Miqi180 commented 10 years ago

Well, I agree that your approach does seem cleaner than mine, even though it will produce exactly the same results. After your fix, would you be kind enough to post the changes in this thread? I've modified your js code quite a bit to suit the needs of my project and replacing the whole grid.js file is not an option for me.

Thanks in advance.

optikalefx commented 10 years ago

Yea the diff will have exactly what was changed. Mind if I ask why you had the change the main grid.js file? I tried to make as many hooks as needed to not need to do that.

Miqi180 commented 10 years ago

Well, mainly because instead of having just one add button for new rows below the grid (as in your "Using_the_API" video at the end) we opted to make a column with add buttons, just like your delete button column in the "Deleting" grid on SQB. Now, I'm not saying this cannot be achieved without augmenting grid.js, but it just seemed a whole lot easier to do it that way. Feel free to correct me if I'm wrong. I think you've done a marvellous job with the grid, but adding new rows is arguably the "Achilles heel " of your otherwise excellent documentation. (For the record, I do appreciate that documentation takes time, no need to point that out :)

Secondly, and less importantly, the English text in the alerts has been translated to another language.

optikalefx commented 10 years ago

Completely agree with the localization of languages. When I make a nodeJS backend version of this grid and update it to bootstrap 3, I'll probably make a localize json file.

I thought adding a column wasn't too bad

$(this).grid("addColumn","Add",{
    width: 60, 
    insertAt: "end",     // index | column | "end" | "start"
    header : "Action",
    cellClass : "center"
}, function(i, data) {
    return "<button class='btn btn-warning btn-mini'>Add</button>";
});

It allows you to do a lot. You could of course also just have your backend end send an empty column, and add a new cellType as seen in this example

Custom Callbacks and Custom Cell Types To let you have like unlimited functionality with that column and all data within it.

Not seen in the example docs page is that the function above, (i, data) passes that 2nd parameter of data, which is all of the grid data for that row, as it's creating that part of the column.

And you could apply the event listener directly to this during the creation, though its MUCH better to delegate events to buttons.

Miqi180 commented 10 years ago

Let me know if you need localization to Danish then... :)

With regards to the other issue, yes the addColumn function works very well, but the addRow function in grid.js was another story and far more complicated to get to work (at least for me). First of all, we didn't want the grid to be reloaded upon row insertion (cf. https://github.com/optikalefx/OpenJS-Grid/issues/34). Hence, I modified the addRow function to accomodate for our cell types and added an ajax call for blank row insertion and now the user can simply tab to the add button in the grid, press enter and then the first cell of the new row is focused, without reloading the grid. Again, feel free to correct me if I'm wrong, but I just don't see an easy way of achieving that without modifying grid.js.

I should probably mention that I have developed a keyboard navigation system for the grid allowing the user to move up/down and sideways without ever using the mouse. And it still works if some of the rows are invisible, i.e. when the filter function is applied. (This is in a separate file and not a modification of grid.js)

I completely agree with your point about delegation.

optikalefx commented 10 years ago

Ah row adding. Yea. So I took that out in this version of the grid. Why? Because I never used it. I added rows all the time, but never with any blanket way. I either

A) dialog popup B) new page C) needed default data or logic

Simply adding a blank row to the grid was never good enough in production. Moreover, are you adding a row by making an ajax call and prepending that row to the top? Similar to how the old add function was?

Or are you adding rows and then saving all the new rows at once? Problem with that, is that not all cells can be saved. Some cells come from different tables, and this grid only handles saving back to the original table. Yes you can "join" in data to -show- but not to edit. So how can you add a row with data that the user is expecting to edit, but the grid can't save it? Well, I would make a custom save for that grid. So now I had this blank row add, and custom PHP to save. It was a mess.

The solution was to add a row however my app needed, be it a popup or page, and just refresh the grid in place and it would grab that new data.

Last point, adding a row to the top of the grid usually means its out of sort order now.

Miqi180 commented 10 years ago

Yes, it was quite a mess to get it to work consistently. Let me outline my solution: 1) make the ajax and let the server return the primary key of the new inserted row in the DB In the callback function: 2) insert new grid row beneath clicked row 3) update the object self.rows["_"+pKey] with empty col:cell values

This way you can add new rows dynamically, regardless of current row position and without reloading the grid. No need to save the whole grid, or any row for that matter because you save the changes directly in the self.rows object, which means the markForSaving function will work (if you don't do this it won't!). You also avoid inserting at the top which is a rather clumsy solution, considering we have a whole column of add buttons. Sort order doesn't become an issue either.

I don't currently have any grids with joined tables, and I'm not sure how well this method would fit such scenarioes, but for for our purposes and needs it works very well.

ElmerCat commented 7 years ago

Solution to single quotation mark problem

Grid.js line 64 — problem

cellValue: "<input type='text' value='"+value+"'/>"

change to:

cellValue: "<input type='text' value='"+value.replace(/'/g , "&apos;")+"'/>"

If the value contained a single quote ' it was breaking out of the quoted value parameter sequence. Adding a regex to encode the quote as an apostrophe fixed the problem for me.