brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] [Live HTML] Bad things happen when adding an attribute #4355

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by dangoor Friday Aug 09, 2013 at 03:05 GMT Originally opened as https://github.com/adobe/brackets/issues/4713


I saw both of these scenarios in the Getting Started Guide.

Bad Thing 1:

  1. Navigate just about anywhere in the document
  2. Type <p>
  3. End tag added automatically
  4. Type hello.
  5. See nice new paragraph in document.
  6. Move cursor to just before the > in the open tag for the paragraph
  7. Type style="color: red"

The "hello" is gone, and there is likely more than that messed up. I did this after the <h2> tag and the paragraph that follows that tag disappeared.

Bad Thing 2:

  1. I reloaded and navigated to that same spot (the line after <h2>)
  2. Type <p style="color:red">
  3. The paragraph that follows disappeared. The paragraph after that one is red.
  4. If you type text, it appears at the end of the "Brackets is a different type of editor" paragraph.
  5. I then deleted the style attribute and the paragraph turned black again
  6. whenever I typed anything, each character was added as a new block of text.
core-ai-bot commented 3 years ago

Comment by dangoor Friday Aug 09, 2013 at 03:06 GMT


I'm marking this sprint 29 just to track the live HTML blockers together.

core-ai-bot commented 3 years ago

Comment by dangoor Friday Aug 09, 2013 at 14:28 GMT


Looking at Bad Thing 1, things start going awry when you type: style=". Once you type that double quote, here are the edits:

[
  {
    "type": "attrChange",
    "tagID": 59,
    "attribute": "style",
    "value": ">Hello</p>\n        <!--\n            MADE WITH <3 AND JAVASCRIPT\n        -->\n        \n        <p>\n            Welcome to an early preview of Brackets, a new open-source editor for the next generation of\n            the web. We're big fans of standards and want to build better tooling for JavaScript, HTML and CSS \n            and related open web technologies. This is our humble beginning.\n        </p>\n        \n        <!--\n            WHAT IS BRACKETS?\n        -->\n        <p>\n            <em>Brackets is a different type of editor.</em>\n            One notable difference is that this editor is written in JavaScript, HTML and CSS.\n            This means that most of you using Brackets have the skills necessary to modify and extend the editor.\n            In fact, we use Brackets every day to build Brackets. It also has some unique features like Quick Edit,\n            Live Preview and others that you may not find in other editors.\n            To learn more about how to use those features, read on.\n        </p>\n        \n        \n        <h2>We're trying out a few new things</h2>\n        \n        <!--\n            THE RELATIONSHIP BETWEEN HTML, CSS AND JAVASCRIPT\n        -->\n        <h3>Quick Edit for CSS and JavaScript</h3>\n        <p>\n            No more switching between documents and losing your context. When editing HTML, use the\n            <kbd>Cmd/Ctrl + E</kbd> shortcut to open a quick inline editor that displays all the related CSS.\n            Make a tweak to your CSS, hit <kbd>ESC</kbd> and you're back to editing HTML, or just leave the\n            CSS rules open and they'll become part of your HTML editor. If you hit <kbd>ESC</kbd> outside of\n            a quick inline editor, they'll all collapse.\n        </p>\n        \n        <samp>\n            Want to see it in action? Place your cursor on the <!-- <samp> --> tag above and press\n            <kbd>Cmd/Ctrl + E</kbd>. You should see a CSS quick editor appear above. On the right you will see\n            a list of the CSS rules that are related to this tag. Simply scroll the rules with \n            <kbd>Alt + Up/Down</kbd> to find the one you want to edit.\n        </samp>\n        \n        <a href="
  },
  {
    "type": "attrAdd",
    "tagID": 59,
    "attribute": "screenshots",
    "value": ""
  },
  {
    "type": "attrChange",
    "tagID": 59,
    "attribute": "style",
    "value": ">Hello</p>\n        <!--\n            MADE WITH <3 AND JAVASCRIPT\n        -->\n        \n        <p>\n            Welcome to an early preview of Brackets, a new open-source editor for the next generation of\n            the web. We're big fans of standards and want to build better tooling for JavaScript, HTML and CSS \n            and related open web technologies. This is our humble beginning.\n        </p>\n        \n        <!--\n            WHAT IS BRACKETS?\n        -->\n        <p>\n            <em>Brackets is a different type of editor.</em>\n            One notable difference is that this editor is written in JavaScript, HTML and CSS.\n            This means that most of you using Brackets have the skills necessary to modify and extend the editor.\n            In fact, we use Brackets every day to build Brackets. It also has some unique features like Quick Edit,\n            Live Preview and others that you may not find in other editors.\n            To learn more about how to use those features, read on.\n        </p>\n        \n        \n        <h2>We're trying out a few new things</h2>\n        \n        <!--\n            THE RELATIONSHIP BETWEEN HTML, CSS AND JAVASCRIPT\n        -->\n        <h3>Quick Edit for CSS and JavaScript</h3>\n        <p>\n            No more switching between documents and losing your context. When editing HTML, use the\n            <kbd>Cmd/Ctrl + E</kbd> shortcut to open a quick inline editor that displays all the related CSS.\n            Make a tweak to your CSS, hit <kbd>ESC</kbd> and you're back to editing HTML, or just leave the\n            CSS rules open and they'll become part of your HTML editor. If you hit <kbd>ESC</kbd> outside of\n            a quick inline editor, they'll all collapse.\n        </p>\n        \n        <samp>\n            Want to see it in action? Place your cursor on the <!-- <samp> --> tag above and press\n            <kbd>Cmd/Ctrl + E</kbd>. You should see a CSS quick editor appear above. On the right you will see\n            a list of the CSS rules that are related to this tag. Simply scroll the rules with \n            <kbd>Alt + Up/Down</kbd> to find the one you want to edit.\n        </samp>\n        \n        <a href="
  },
  {
    "type": "attrAdd",
    "tagID": 59,
    "attribute": "screenshots",
    "value": ""
  },
  {
    "type": "attrChange",
    "tagID": 59,
    "attribute": "style",
    "value": ">Hello</p>\n        <!--\n            MADE WITH <3 AND JAVASCRIPT\n        -->\n        \n        <p>\n            Welcome to an early preview of Brackets, a new open-source editor for the next generation of\n            the web. We're big fans of standards and want to build better tooling for JavaScript, HTML and CSS \n            and related open web technologies. This is our humble beginning.\n        </p>\n        \n        <!--\n            WHAT IS BRACKETS?\n        -->\n        <p>\n            <em>Brackets is a different type of editor.</em>\n            One notable difference is that this editor is written in JavaScript, HTML and CSS.\n            This means that most of you using Brackets have the skills necessary to modify and extend the editor.\n            In fact, we use Brackets every day to build Brackets. It also has some unique features like Quick Edit,\n            Live Preview and others that you may not find in other editors.\n            To learn more about how to use those features, read on.\n        </p>\n        \n        \n        <h2>We're trying out a few new things</h2>\n        \n        <!--\n            THE RELATIONSHIP BETWEEN HTML, CSS AND JAVASCRIPT\n        -->\n        <h3>Quick Edit for CSS and JavaScript</h3>\n        <p>\n            No more switching between documents and losing your context. When editing HTML, use the\n            <kbd>Cmd/Ctrl + E</kbd> shortcut to open a quick inline editor that displays all the related CSS.\n            Make a tweak to your CSS, hit <kbd>ESC</kbd> and you're back to editing HTML, or just leave the\n            CSS rules open and they'll become part of your HTML editor. If you hit <kbd>ESC</kbd> outside of\n            a quick inline editor, they'll all collapse.\n        </p>\n        \n        <samp>\n            Want to see it in action? Place your cursor on the <!-- <samp> --> tag above and press\n            <kbd>Cmd/Ctrl + E</kbd>. You should see a CSS quick editor appear above. On the right you will see\n            a list of the CSS rules that are related to this tag. Simply scroll the rules with \n            <kbd>Alt + Up/Down</kbd> to find the one you want to edit.\n        </samp>\n        \n        <a href="
  },
  {
    "type": "attrAdd",
    "tagID": 59,
    "attribute": "screenshots",
    "value": ""
  },
  {
    "type": "textDelete",
    "parentID": 8,
    "afterID": 59,
    "beforeID": 11
  },
  {
    "type": "elementDelete",
    "tagID": 11
  },
  {
    "type": "textDelete",
    "parentID": 8,
    "afterID": 11,
    "beforeID": 12
  },
  {
    "type": "elementDelete",
    "tagID": 12
  },
  {
    "type": "textDelete",
    "parentID": 8,
    "afterID": 12,
    "beforeID": 14
  },
  {
    "type": "elementDelete",
    "tagID": 14
  },
  {
    "type": "textDelete",
    "parentID": 8,
    "afterID": 14,
    "beforeID": 15
  },
  {
    "type": "elementDelete",
    "tagID": 15
  },
  {
    "type": "textDelete",
    "parentID": 8,
    "afterID": 15,
    "beforeID": 16
  },
  {
    "type": "elementDelete",
    "tagID": 16
  },
  {
    "type": "textDelete",
    "parentID": 8,
    "afterID": 16,
    "beforeID": 20
  },
  {
    "type": "elementDelete",
    "tagID": 20
  },
  {
    "type": "textDelete",
    "parentID": 8,
    "afterID": 20,
    "beforeID": 23
  },
  {
    "type": "elementDelete",
    "tagID": 23
  },
  {
    "type": "textDelete",
    "parentID": 8,
    "afterID": 23,
    "beforeID": 25
  },
  {
    "type": "textInsert",
    "content": "\n        \n        ",
    "parentID": 59,
    "afterID": 24
  },
  {
    "type": "textReplace",
    "content": "\n            ",
    "parentID": 59,
    "beforeID": 24
  }
] 

Apparently, the tokenizer/parser does not kick this out as invalid so it actually applies this to the browser. We might consider tossing out unmatched quotes as an invalid case.

What happens if we close the quotes? We get these edits:

[
  {
    "type": "attrAdd",
    "tagID": 59,
    "attribute": "href",
    "value": "screenshots/quick-edit.png"
  },
  {
    "type": "attrDel",
    "tagID": 59,
    "attribute": "style"
  },
  {
    "type": "attrDel",
    "tagID": 59,
    "attribute": "screenshots"
  },
  {
    "type": "textDelete",
    "parentID": 59,
    "afterID": 24
  }
] 

So, we're clearly not in a good state. At this point, it looks like we have two things to do:

  1. look for unmatched quotes as a way to avoid huge flashes of change in the browser while the user edits
  2. look into why we get this pair of edits, given that you'd expect the second edits to undo the first

Update: I just pasted the edits in without looking to closely at them. The shape of the edits was clearly off. Looking at that first set of edits, it's obvious that something is wrong with them at the very least because we're getting multiple attrChanges for "style" on tagID 59.

core-ai-bot commented 3 years ago

Comment by dangoor Friday Aug 09, 2013 at 14:48 GMT


It seems like the reason the second set of edits doesn't undo the first is that the incremental editing logic would see the second change to be entirely within the confines of the one tag, even though it's a change that affects a greater area.

I will note that it's not enough to detect a > character before quotes are closed because > is a legal character in attribute values.

core-ai-bot commented 3 years ago

Comment by dangoor Friday Aug 09, 2013 at 15:03 GMT


I need to run for now, but I wanted to add the note that an error is returned for a simple sample like <p style=">foo</p>, but it appears that the entire document is being sent through the dom builder.

core-ai-bot commented 3 years ago

Comment by njx Friday Aug 09, 2013 at 16:38 GMT


Yup, that's nasty. I had assumed that if the user had unbalanced quotes, we would either end up with an unbalanced tag close, or we would be in a "collecting attribute" state at the end of the document, which is supposed to cause an error (since we return an error if the tokenizer ends in a state other than TEXT). I can understand why the first case isn't happening (because basically the attribute value would end up including the > of the current tag and the < of the next tag that has a quoted attribute), but I don't know why we aren't getting the second case. I'll look into that further today.

I think you're also probably right that incremental reparses need to expand their bounds more than they are in some cases today. In general, there's no harm (other than performance) in reparsing more, so we should feel free to expand the bounds if there's any danger that we're missing something. I'll look at that as well.

core-ai-bot commented 3 years ago

Comment by njx Friday Aug 09, 2013 at 18:53 GMT


The above pull request fixes the tokenization issue (which fixes all the symptoms in this bug).

Regarding the incremental parse issue, it sounds like the problem there was that because the first edit was so destructive, the marked ranges for everything got really confused. Or, to be more precise, the marked ranges changed radically between the first and second edits (because of the tree structure, not the edit itself); so when we look up the range after the second edit (where the tree is more correct), it doesn't really correspond meaningfully to the actual changed portion of the document structure.

It sounds like we need some heuristic where, if we detect that a big enough change has happened to the document structure, we shouldn't try to do an incremental edit. But it's a catch-22, because we might not be able to determine that unless we reparse the whole document...Needs more thought.

core-ai-bot commented 3 years ago

Comment by dangoor Friday Aug 09, 2013 at 20:17 GMT


This issue is fixed well by the pull request. Thanks for the quick work!

Closing...