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

Live Preview should work for .xhtml files #5058

Closed
uptownnickbrown opened this Issue Sep 4, 2013 · 23 comments

Comments

Projects
None yet
7 participants
@uptownnickbrown

When attempting to Live Preview a file with a .xhtml extension, this error pops up:

"Live Preview Error - Open an HTML file in order to launch live preview"

I've seen this in all Sprints on Windows and Mac, up to and including the latest release (Sprint 30). This happens for any file with a .xhtml extension.

Removing the "x" from the filename fixes the problem - with a .html extension the same content previews in Chrome just fine.

I propose the Live Preview button treat .xhtml files the same as .html files - it looks like that transition was already made for syntax highlighting (#785) a while back.

The use case here (or at least my use case) is editing EPUB 3 content files, which must be XHTML conformant.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Sep 4, 2013

Member

If the XML syntax already works properly with the new HTML Live Preview feature landing this sprint (including constructs like <div/>), then this is just a straightforward matter of updating FileUtils.isStaticHtmlFileExt(). But we'd probably want to do some testing to make sure XML really does work.

Member

peterflynn commented Sep 4, 2013

If the XML syntax already works properly with the new HTML Live Preview feature landing this sprint (including constructs like <div/>), then this is just a straightforward matter of updating FileUtils.isStaticHtmlFileExt(). But we'd probably want to do some testing to make sure XML really does work.

@njx

This comment has been minimized.

Show comment
Hide comment
@njx

njx Sep 4, 2013

Member

Self-closing-tag syntax does work. I think the main thing in XHTML that we don't handle properly is CDATA blocks (which is only legal in foreign tags in HTML5, I believe, but is legal anywhere where character data can occur in XHTML). Our tokenizer handles it, but we don't process it properly as a node during our parsing/diffing/editing phase, so edits to it would probably be ignored, and edits around it might get applied incorrectly.

Member

njx commented Sep 4, 2013

Self-closing-tag syntax does work. I think the main thing in XHTML that we don't handle properly is CDATA blocks (which is only legal in foreign tags in HTML5, I believe, but is legal anywhere where character data can occur in XHTML). Our tokenizer handles it, but we don't process it properly as a node during our parsing/diffing/editing phase, so edits to it would probably be ignored, and edits around it might get applied incorrectly.

@uptownnickbrown

This comment has been minimized.

Show comment
Hide comment
@uptownnickbrown

uptownnickbrown Sep 5, 2013

Thanks for the quick feedback - happy to do some testing/hacking on this. I haven't done any work on Brackets before, so I just set myself up with the latest master branch. I can confirm that simply adding "xhtml" to the _staticHtmlFileExts array in FileUtils.js allows me to launch .xhtml files with Live Preview.

Is the new HTML Live Preview feature on master at this point, or do I need to check out another branch to test my xhtml files out with that feature? I'm not seeing any issues with previewing/editing my content so far, though I haven't tested any files with CDATA blocks yet.

Thanks for the quick feedback - happy to do some testing/hacking on this. I haven't done any work on Brackets before, so I just set myself up with the latest master branch. I can confirm that simply adding "xhtml" to the _staticHtmlFileExts array in FileUtils.js allows me to launch .xhtml files with Live Preview.

Is the new HTML Live Preview feature on master at this point, or do I need to check out another branch to test my xhtml files out with that feature? I'm not seeing any issues with previewing/editing my content so far, though I haven't tested any files with CDATA blocks yet.

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Sep 5, 2013

Contributor

Live HTML has not yet been turned on in master, but will be soon. For now, with Live Development off, go to Debug > Show Dev Tools, switch to console and type brackets.livehtml = true to turn it on.

Contributor

redmunds commented Sep 5, 2013

Live HTML has not yet been turned on in master, but will be soon. For now, with Live Development off, go to Debug > Show Dev Tools, switch to console and type brackets.livehtml = true to turn it on.

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn Sep 5, 2013

Member

It's now enabled in master.

Member

peterflynn commented Sep 5, 2013

It's now enabled in master.

@uptownnickbrown

This comment has been minimized.

Show comment
Hide comment
@uptownnickbrown

uptownnickbrown Sep 6, 2013

Just did some testing using the latest master. The bulk of my content works great. As predicted, there is some unpredictable behavior when using CDATA tags.

  • Loading a page that already has CDATA tags surrounding some text:
    • The CDATA-wrapped text initially displays correctly
    • Editing the CDATA-wrapped text in Brackets does not auto-update in the browser
    • Removing the CDATA tags causes the text to be duplicated in the browser. Eg, changing this <span><![CDATA[Exploring the Theme]]></span> to this <span>Exploring the Theme</span> caused this error:
      screen shot 2013-09-06 at 12 05 57 pm
    • Editing other text chunks on the page works fine
    • Adding new elements to the page does not work reliably
  • Loading a page without CDATA tags in it:
    • Adding CDATA tags to a chunk of text causes it to disappear.

Debugging this might be beyond me, but I'm happy to take a look - where does the parsing/diffing/editing code that would need adjustment live? Any tips?

Just did some testing using the latest master. The bulk of my content works great. As predicted, there is some unpredictable behavior when using CDATA tags.

  • Loading a page that already has CDATA tags surrounding some text:
    • The CDATA-wrapped text initially displays correctly
    • Editing the CDATA-wrapped text in Brackets does not auto-update in the browser
    • Removing the CDATA tags causes the text to be duplicated in the browser. Eg, changing this <span><![CDATA[Exploring the Theme]]></span> to this <span>Exploring the Theme</span> caused this error:
      screen shot 2013-09-06 at 12 05 57 pm
    • Editing other text chunks on the page works fine
    • Adding new elements to the page does not work reliably
  • Loading a page without CDATA tags in it:
    • Adding CDATA tags to a chunk of text causes it to disappear.

Debugging this might be beyond me, but I'm happy to take a look - where does the parsing/diffing/editing code that would need adjustment live? Any tips?

@njx

This comment has been minimized.

Show comment
Hide comment
@njx

njx Sep 6, 2013

Member

It might be rather complicated, unfortunately. The issue is that the way that we map the source structure to the DOM in the browser is by injecting special "data-brackets-id" attributes into tags when we send the file to the browser. We can't do that same hack for non-tag nodes without disrupting the content. So, for text nodes, what we do is just treat the text as "whatever's between the tags", without tracking individual nodes, and just delete/replace all the text between two tags if any of it changes. That's generally fine for text since nothing should really care about a specific text node, but it would be more complex for CDATA because we would need to know which portion of the text was wrapped in CDATA.

That said, I don't actually know how CDATA is represented in the DOM--it might be that it just gets turned into ordinary text once you get to the DOM. If that's the case, then this might be easier--we would just treat it as text on the browser side, and just be careful not to parse it for entities. (Since we don't have to map back from the browser to our source, it doesn't really matter if we have to do a transformation on the way to the browser, I think.)

Member

njx commented Sep 6, 2013

It might be rather complicated, unfortunately. The issue is that the way that we map the source structure to the DOM in the browser is by injecting special "data-brackets-id" attributes into tags when we send the file to the browser. We can't do that same hack for non-tag nodes without disrupting the content. So, for text nodes, what we do is just treat the text as "whatever's between the tags", without tracking individual nodes, and just delete/replace all the text between two tags if any of it changes. That's generally fine for text since nothing should really care about a specific text node, but it would be more complex for CDATA because we would need to know which portion of the text was wrapped in CDATA.

That said, I don't actually know how CDATA is represented in the DOM--it might be that it just gets turned into ordinary text once you get to the DOM. If that's the case, then this might be easier--we would just treat it as text on the browser side, and just be careful not to parse it for entities. (Since we don't have to map back from the browser to our source, it doesn't really matter if we have to do a transformation on the way to the browser, I think.)

@njx

This comment has been minimized.

Show comment
Hide comment
@njx

njx Sep 6, 2013

Member

BTW, the relevant code is in src/language, in HTMLInstrumentation.js, HTMLSimpleDOM.js, and HTMLDOMDiff.js, as well as LiveDevelopment/Agents/RemoteFunctions.js. Basically, HTMLInstrumentation takes the HTML in the editor, instruments it with the data-brackets-id attributes for sending to the browser, and then maintains a local simple DOM-style structure. As the user edits in the editor, we update the local DOM tree, then diff it against the previous tree to generate a stream of edits (e.g. add DOM node, remove node, replace text, set attribute) that we then send to the browser (via RemoteFunctions) to "replay" in the real browser DOM.

This code is still under development but we just added a bunch of comments yesterday so it should be more legible :)

Member

njx commented Sep 6, 2013

BTW, the relevant code is in src/language, in HTMLInstrumentation.js, HTMLSimpleDOM.js, and HTMLDOMDiff.js, as well as LiveDevelopment/Agents/RemoteFunctions.js. Basically, HTMLInstrumentation takes the HTML in the editor, instruments it with the data-brackets-id attributes for sending to the browser, and then maintains a local simple DOM-style structure. As the user edits in the editor, we update the local DOM tree, then diff it against the previous tree to generate a stream of edits (e.g. add DOM node, remove node, replace text, set attribute) that we then send to the browser (via RemoteFunctions) to "replay" in the real browser DOM.

This code is still under development but we just added a bunch of comments yesterday so it should be more legible :)

@uptownnickbrown

This comment has been minimized.

Show comment
Hide comment
@uptownnickbrown

uptownnickbrown Sep 6, 2013

Thanks, I'll dive in and see how it looks.

For frame of reference - we deal with quite a lot of XHTML content and I've never actually seen a client use CDATA tags around text, only in <script> tags. So we may be talking about an edge case within an edge case when we talk about CDATA text in XHTML/HTML5 hybrid files...

In fact, the WHATWG says explicitly (when discussing XHTML files served as HTML5): "Do not use CDATA sections (except possibly for script and style tags--see element-specific behavior below or for SVG/MathML)."

Thanks, I'll dive in and see how it looks.

For frame of reference - we deal with quite a lot of XHTML content and I've never actually seen a client use CDATA tags around text, only in <script> tags. So we may be talking about an edge case within an edge case when we talk about CDATA text in XHTML/HTML5 hybrid files...

In fact, the WHATWG says explicitly (when discussing XHTML files served as HTML5): "Do not use CDATA sections (except possibly for script and style tags--see element-specific behavior below or for SVG/MathML)."

@njx

This comment has been minimized.

Show comment
Hide comment
@njx

njx Sep 12, 2013

Member

Marking move to backlog. I think this is nontrivial, though not terribly difficult. But it also just occurred to me that we probably don't do the right thing even with <script> tags in HTML right now, because we always try to parse text for entities. I'll file a separate bug on that issue--if/when we fix that, that would probably make this easier, as we would have to start tracking which text nodes are intended to be parsed for entities and which ones aren't.

Member

njx commented Sep 12, 2013

Marking move to backlog. I think this is nontrivial, though not terribly difficult. But it also just occurred to me that we probably don't do the right thing even with <script> tags in HTML right now, because we always try to parse text for entities. I'll file a separate bug on that issue--if/when we fix that, that would probably make this easier, as we would have to start tracking which text nodes are intended to be parsed for entities and which ones aren't.

@uptownnickbrown

This comment has been minimized.

Show comment
Hide comment
@uptownnickbrown

uptownnickbrown Sep 19, 2013

Just noticed pull request #5251, enabling Live Preview of SVG files but not LiveHTML updating of SVG.

Is it worth enabling that level of "Live" support for XHTML files? Or do you think it's necessary to get LiveHTML in place first?

Just noticed pull request #5251, enabling Live Preview of SVG files but not LiveHTML updating of SVG.

Is it worth enabling that level of "Live" support for XHTML files? Or do you think it's necessary to get LiveHTML in place first?

@njx

This comment has been minimized.

Show comment
Hide comment
@njx

njx Sep 19, 2013

Member

I think I might have misunderstood the original request here. We could easily enable preview of XHTML files, but without live-as-you-type editing. That's basically what we're doing for SVG. Doing the live-as-you-type editing for XHTML would require us to deal with CDATA and possibly namespaces, so that's what makes that hard.

Would it be valuable for you if we just made it so you could easily preview XHTML files without them updating as you type? (They would refresh on save.)

Member

njx commented Sep 19, 2013

I think I might have misunderstood the original request here. We could easily enable preview of XHTML files, but without live-as-you-type editing. That's basically what we're doing for SVG. Doing the live-as-you-type editing for XHTML would require us to deal with CDATA and possibly namespaces, so that's what makes that hard.

Would it be valuable for you if we just made it so you could easily preview XHTML files without them updating as you type? (They would refresh on save.)

@uptownnickbrown

This comment has been minimized.

Show comment
Hide comment
@uptownnickbrown

uptownnickbrown Sep 19, 2013

It is valuable for me - I'm in fact doing that locally already. I made the exact same tweak as the SVG support tweak, editing FileUtils.js to include this line:

var _staticHtmlFileExts = ["htm", "html", "xhtml"],

Live-as-you-type editing would obviously be nice, but updating on refresh is a useful stop-gap solution.

It is valuable for me - I'm in fact doing that locally already. I made the exact same tweak as the SVG support tweak, editing FileUtils.js to include this line:

var _staticHtmlFileExts = ["htm", "html", "xhtml"],

Live-as-you-type editing would obviously be nice, but updating on refresh is a useful stop-gap solution.

@njx

This comment has been minimized.

Show comment
Hide comment
@njx

njx Sep 20, 2013

Member

Got it. It's not quite that simple, I think, because from looking at the code it seems like we would try to do live-as-you-type editing any file whose extension is in _staticHtmlFileExts. But it should be straightforward to fix that.

Member

njx commented Sep 20, 2013

Got it. It's not quite that simple, I think, because from looking at the code it seems like we would try to do live-as-you-type editing any file whose extension is in _staticHtmlFileExts. But it should be straightforward to fix that.

@ghost ghost assigned njx Sep 20, 2013

@njx

This comment has been minimized.

Show comment
Hide comment
@njx

njx Sep 20, 2013

Member

Medium pri to me

Member

njx commented Sep 20, 2013

Medium pri to me

@uptownnickbrown

This comment has been minimized.

Show comment
Hide comment
@uptownnickbrown

uptownnickbrown Sep 20, 2013

You're right - in my local tweaked version, Brackets does "try" to do live-as-you-type editing with some very buggy behavior that I detailed earlier in the thread.

And I agree with you 100% - it's useful for me to be able to turn on Live Preview even with broken live-as-you-type editing, but in the big picture I don't think any file format should be able to Live Preview with really broken LiveHTML behavior.

You're right - in my local tweaked version, Brackets does "try" to do live-as-you-type editing with some very buggy behavior that I detailed earlier in the thread.

And I agree with you 100% - it's useful for me to be able to turn on Live Preview even with broken live-as-you-type editing, but in the big picture I don't think any file format should be able to Live Preview with really broken LiveHTML behavior.

@shorshe

This comment has been minimized.

Show comment
Hide comment
@shorshe

shorshe Feb 13, 2014

I'm using Brackets for EPUB editing. xhtml Preview would be highly appreciated.... ;)
Especially as sigil will fade into nonexistence sooner or later...

shorshe commented Feb 13, 2014

I'm using Brackets for EPUB editing. xhtml Preview would be highly appreciated.... ;)
Especially as sigil will fade into nonexistence sooner or later...

@takahashim

This comment has been minimized.

Show comment
Hide comment
@takahashim

takahashim Mar 16, 2014

I'm also trying to use editing EPUB content files with a patch var _staticHtmlFileExts = ["htm", "html", "xhtml"],.
With this patch, it seems very comfortable for me.

I'm also trying to use editing EPUB content files with a patch var _staticHtmlFileExts = ["htm", "html", "xhtml"],.
With this patch, it seems very comfortable for me.

@shorshe

This comment has been minimized.

Show comment
Hide comment
@shorshe

shorshe Mar 17, 2014

Hi takahashim!
Were you able to insert this patch in a distribution build or did you build it yourself?
In the distribution I can't find FileUtils.js...

shorshe commented Mar 17, 2014

Hi takahashim!
Were you able to insert this patch in a distribution build or did you build it yourself?
In the distribution I can't find FileUtils.js...

@takahashim

This comment has been minimized.

Show comment
Hide comment
@takahashim

takahashim Mar 17, 2014

@shorshe FileUtils.js is in src/file/.

$ git diff
diff --git a/src/file/FileUtils.js b/src/file/FileUtils.js
index 55c418f..d1fb8ad 100644
--- a/src/file/FileUtils.js
+++ b/src/file/FileUtils.js
@@ -330,7 +330,7 @@ define(function (require, exports, module) {
     }

     /** @const - hard-coded for now, but may want to make these preferences */
-    var _staticHtmlFileExts = ["htm", "html"],
+    var _staticHtmlFileExts = ["htm", "html", "xhtml"],
         _serverHtmlFileExts = ["php", "php3", "php4", "php5", "phtm", "phtml", "cfm", "cfml", "asp", "aspx", "jsp", "jspx", "shtm", "shtml"];

     /**

@shorshe FileUtils.js is in src/file/.

$ git diff
diff --git a/src/file/FileUtils.js b/src/file/FileUtils.js
index 55c418f..d1fb8ad 100644
--- a/src/file/FileUtils.js
+++ b/src/file/FileUtils.js
@@ -330,7 +330,7 @@ define(function (require, exports, module) {
     }

     /** @const - hard-coded for now, but may want to make these preferences */
-    var _staticHtmlFileExts = ["htm", "html"],
+    var _staticHtmlFileExts = ["htm", "html", "xhtml"],
         _serverHtmlFileExts = ["php", "php3", "php4", "php5", "phtm", "phtml", "cfm", "cfml", "asp", "aspx", "jsp", "jspx", "shtm", "shtml"];

     /**
@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Mar 17, 2014

Contributor

@shorshe In a cloned build, the file is src/file/FileUtils.js as @takahashim stated. In an installed build, many of the files are concatenated in www/main.js.

Contributor

redmunds commented Mar 17, 2014

@shorshe In a cloned build, the file is src/file/FileUtils.js as @takahashim stated. In an installed build, many of the files are concatenated in www/main.js.

@shorshe

This comment has been minimized.

Show comment
Hide comment
@shorshe

shorshe Mar 18, 2014

Thanks for the tip @redmunds !
I found and changed the variable in main.js in brackets sprint 36. It breaks the code signature obviously, but works like a charm afterwards.

shorshe commented Mar 18, 2014

Thanks for the tip @redmunds !
I found and changed the variable in main.js in brackets sprint 36. It breaks the code signature obviously, but works like a charm afterwards.

@njx njx removed their assignment Jul 12, 2014

@redmunds redmunds self-assigned this Jul 31, 2014

@redmunds redmunds added this to the Release 0.43 milestone Jul 31, 2014

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Aug 13, 2014

Contributor

@uptownnickbrown This has been fixed in release 0.43. Closing.

Contributor

redmunds commented Aug 13, 2014

@uptownnickbrown This has been fixed in release 0.43. Closing.

@redmunds redmunds closed this Aug 13, 2014

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