Skip to content
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

  displays with Firefox and getFile .content #204

Closed
zspecza opened this issue Feb 1, 2013 · 25 comments
Closed

  displays with Firefox and getFile .content #204

zspecza opened this issue Feb 1, 2013 · 25 comments

Comments

@zspecza
Copy link

zspecza commented Feb 1, 2013

Playing around with 0.2.1 along with syncing with a <textarea> and syntax highlighting via highlight.js, saving content to a MongoDB database, I find that within code tags, any indentation via spaces is outputted as &nbsp; for each space except the last, that one outputs fine.

I've tried breaking things a little by removing the &nbsp;'s from .replace() in lines 183, 1377 and 1380 of epiceditor.js, but I still see that on output even if I create a new document after the changes... Is really doing my head in. I've also tried looking at highlight.js but there's not even a mention of &nbsp; there that I can find with Sublime Text.

This only happens in code tags, nowhere else... Any thoughts?

@OscarGodson
Copy link
Owner

I'm not sure off the top of my head, but I will look into it. I believe highlight.js actually adds those in their code and I'm not sure the sequence of events that happen. However, are you using the textarea option I just added? And does this happen without highlight.js but just manually typing in:

<code>
Foo
  Hello
        World
</code>

? Also, it'd be awesome if you could upload a testcase or put something on JSBin :)

@zspecza
Copy link
Author

zspecza commented Feb 1, 2013

I am using the textarea option you just added. It happens without highlight.js too. http://jsbin.com/udaboy/1/edit
Seems it works on non-code tags too. That didn't happen on my local instance after converting to html, which was done simply by wrapping self.getFiles(textareaFileName, true).content in a marked() call on lines 931 and 935.

@OscarGodson
Copy link
Owner

Perfect! Thanks man, I'll take a look at it tonight or tomorrow. This helps a lot.

@OscarGodson
Copy link
Owner

Hmm, ok, having trouble reproing. I put this in the editor. All export functions and the text too doesnt have any &nbsp;. Do you have an example of the exact steps or a screencast to repro?

Screen Shot 2013-01-31 at 5 49 52 PM

@zspecza
Copy link
Author

zspecza commented Feb 1, 2013

That's strange. Hmmm...

Have you looked at the JSBin I provided? That reproduces the issue as far as text goes. Would the browser I'm using make any difference? Using the latest Firefox.

@OscarGodson
Copy link
Owner

That could be it. I tried chrome. The screenshot is from your JSBin.


From: declandewetmailto:notifications@github.com
Sent: ‎1/‎31/‎2013 6:22 PM
To: OscarGodson/EpicEditormailto:EpicEditor@noreply.github.com
Cc: Oscar Godsonmailto:oscargodson@outlook.com
Subject: Re: [EpicEditor]   behaviour within code tags (#204)

That's strange. Hmmm...

Have you looked at the JSBin I provided? That reproduces the issue as far as text goes. Would the browser I'm using make any difference? Using the latest Firefox.


Reply to this email directly or view it on GitHub:
#204 (comment)

@zspecza
Copy link
Author

zspecza commented Feb 1, 2013

Hardly recognized it (the image) because the basePath I added didn't affect the style of the editor div on my end, which I thought was just a JSBin thing, heh. I'm downloading Chrome now to see if it's any different to Firefox on my end. If it is then I guess we've nailed what's causing the issue...

@OscarGodson
Copy link
Owner

Yeah, Firefox and chrome handle content editable differently. Hopefully that's it. I'm not near a computer but will try Firefox as well when I can.

@zspecza
Copy link
Author

zspecza commented Feb 1, 2013

It works on Chrome, so this issue only applies to Firefox - but I suspect it can become a quick nuisance to websites that allow every user to add new content using EpicEditor and then storing that data in a DB... everyone using Firefox would become incredibly annoying, lol.

@OscarGodson
Copy link
Owner

Yeah, totally. I'll fix it for sure and add tests :)

@OscarGodson
Copy link
Owner

Just an update, I was able to repro it, added a failing test, but struggling to actually fix it. This will for sure be fixed soon tho and will be in the 0.2.1 release.

@OscarGodson
Copy link
Owner

@tylermolamphy what?! haha

@zspecza
Copy link
Author

zspecza commented Feb 6, 2013

Mhm I've been trying to come up with a fix for this myself - though I'm not a seasoned JS developer... No luck yet though.

@OscarGodson
Copy link
Owner

So, thanks to @lamplightdev your problem will be patched when you pull develop next, but I changed your ticket name. He fixed it by changing the method to get the content. exportFile goes through some cleansing before displaying it whereas getFile does not. However, getFile should should give the same raw text as exportFile.

@OscarGodson
Copy link
Owner

Note to self: getFiles doesn't have any documentation in the code. Update this as well.

@Gankra
Copy link
Collaborator

Gankra commented Jun 5, 2013

So to clarify, this bug is now just that the text obtained via getFiles is not sanitized? Could you just tweak the structure of the files so that .content is just a getter method that runs the text sanitization?

@OscarGodson
Copy link
Owner

Of the files? The files are stringified so they can't be functions.

@Gankra
Copy link
Collaborator

Gankra commented Jun 5, 2013

Right, so when you call getFiles, after you've loaded them in, move all the .contents to ._content and make .content a getter that sanitizes ._content.

Or always keep content in ._content so you don't need to move stuff around when you load stuff in.

@OscarGodson
Copy link
Owner

Sure, whatever works :) Just make sure whatever method you use that it keeps all the whitespace and also wraps correctly and that on reload the content doesn't shift and works across the browsers the same.

So, make sure:

1
  2
    3
  2
1

keeps the right whitespace.

Also make sure stuff like

Some really long string would go here that wraps to two lines

Doesn't get turned into:

Some&nbsp;really&nbsp;long&nbsp;string&nbsp;would&nbsp;go&nbsp;here&nbsp;that&nbsp;wraps&nbsp;to&nbsp;two&nbsp;lines

Which would prevent the text from wrapping.

@Gankra
Copy link
Collaborator

Gankra commented Jun 5, 2013

I'm having trouble reproducing the issue described, are there precise repro steps and/or a failing unit test written somewhere?

@OscarGodson
Copy link
Owner

No unit tests for this case yet, but in Firefox:

foo
  foo
          foo

Type something like that. Then:

var x = editor.getFiles();
console.log(x);

notice that the content contains &nbsp;'s

@Gankra
Copy link
Collaborator

Gankra commented Jun 5, 2013

So I've noticed that, at least in my version of the develop branch, editor.exportFile(); seems to strip the \n's from the text in Firefox but not Chrome. Is this correct behaviour?

It means your sample gets crushed to

foo foo foo

in firefox if you just paste it and refresh

@OscarGodson
Copy link
Owner

Not happening for me on Firefox :\ checked against master and develop. I can send a screencast too if you want.

@Gankra
Copy link
Collaborator

Gankra commented Jun 5, 2013

Ah, never mind, it was just an emergent property of #100.

Gankra pushed a commit to Gankra/EpicEditor that referenced this issue Jun 5, 2013
Gankra pushed a commit to Gankra/EpicEditor that referenced this issue Jun 5, 2013
Gankra pushed a commit to Gankra/EpicEditor that referenced this issue Jun 6, 2013
getFiles includes content by default again
getFiles has a flag to exclude content now
exportFiles has a 'raw' option
exportFiles plaintext escape is now abstracted to a private function
updated docs and tests to reflect this
@OscarGodson
Copy link
Owner

Excited to say this is closed and 0.2.1 is ready for QAing and to have the release notes written up. Thanks a lot @gankro!!!!!

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

No branches or pull requests

3 participants