Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

FIX: Creating a resizable panel using an HTML code fragment starting … #13574

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

khatastroffik
Copy link

…with a comment will break

As mentioned in #6252, the original error msg isn't really helpful.
The modified msg could be improved as well, though:
Since the content of the panel may be a HTML fragment (usually starting with a DIV node), mentioning that the first (root) DOM node must have an ID that will be used as a pref key would help.

Nevertheless, the mangled code in WorkspaceManager.js isn't reliable since it will break when the panel content starts with a html comment. Such a comment may be necessary when a plugin like HTMLhint is used. On a "panel" code fragment, this plugin attests an error due to a missing doctype. To disable this error, one may put a comment at the top of the html code: <!--htmlhint doctype-first: false-->. WorkspaceManager.js will fail creating the panel, then. The proposed modification fixes such an issue.

sample test code for a resizable panel

<!--htmlhint doctype-first: false-->
<!--This comment is of no use but for testing purpose-->
<div id="my-very-special-testing-panel"><span style="color: red">Elvis</span> was here!</div>

…with a comment will break

As mentioned in adobe#6252, the original error msg isn't really helpful. 
The modified msg could be improved as well, though:
Since the content of the panel may be a **HTML fragment** (usually starting with a DIV node), mentioning that ```the **first** (root) DOM node must have an ID that will be used as a pref key``` would help.

Nevertheless, the mangled code in WorkspaceManager.js isn't reliable since it will _break_ when the panel content **starts with a html comment**. Such a comment may be necessary when  a plugin like HTMLhint is used. On a "panel" code fragment, this plugin attests an error due to a missing doctype. To disable this error, one may put a comment at the top of the html code: ```<!--htmlhint doctype-first: false-->```. WorkspaceManager.js will fail creating the panel, then. The proposed modification fixes such an issue.

**sample test code for a resizable panel**
```
<!--htmlhint doctype-first: false-->
<!--This comment is of no use but for testing purpose-->
<div id="my-very-special-testing-panel"><span style="color: red">Elvis</span> was here!</div>
```
Copy link
Collaborator

@petetnt petetnt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lbegue! Thanks for this contribution!

LGTM, maybe a inline comment on the change would be nice as it's not immediately obvious why the root node might not be what you expect it to be 👍

@khatastroffik
Copy link
Author

my pleasure :-)

The function "normalizeKeyDescriptorString(origDescriptor)" is not declared as private (also apply to the API documentation), though it wasn't exported.
The function should be made available, since it may be useful to develop extensions e.g. to check if a key is already bound by looking up in the key map for the corresponding (normalized) descriptor.
See: http://brackets.io/docs/current/modules/command/KeyBindingManager.html
@ficristo
Copy link
Collaborator

ficristo commented Aug 5, 2017

This change seems to break my extension when I add a comment at the start of the panel's html.
The following change seem to fix it. @lbegue could you double check?

// Since the panel source code may start with a html comment, 
// the 'root' DOM node must be targeted explicitly using a filter removing all comments.
this.$panel = $panel.filter("*");
var $rootNodeWithID = this.panel[0];

PS: In future I would prefer if you don't misk unrelated things in the same PR. So the normalizedKey thing should have been in another PR, but not a big deal in this case.

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

Successfully merging this pull request may close these issues.

5 participants