[WIP] Adding slowparse for HTML validation #876

flukeout · 2017-09-14T22:24:11Z

Currently in Progress

Auditing the rules in strings.js one-by-one
- Filing issues in https://github.com/mozilla/slowparse/issues
- Making sure we pass a highlight object into the error messages instead of relying on the token which not all the errors have access to.

Tracking issue: https://github.com/mozilla/thimble.mozilla.org/issues/2456

Related PR in Slowparse

[WIP] Slowparse Updates slowparse#87

Priority Todo

~~Sometimes the error highlight isn't cleared after an error is resolved~~
Sometimes slowparse doesn't fire when the editor first loads, must be some kind of timing thing

Secondary Todo

Cannot right-click links in the error message copy.
Add a hover treatment to the emoji to make it clickable? Give it a little animation!? Something?
Adapt the emoji/icon styling to that it works with long lines of code
- Not sure how to handle this. We'd have to estimate if the code on a line fills up enough space to make it difficult to show the icon.
Update the box title to show the error type and not just "We Found an Error"
- I've just commented it out for now.
Update the color of the highlights when hovering over the message and make sure that if it strobes, it works with the strobing of the underline underneath the error.
Consider: do we need to make the emoji more clickable / discoverable?
Finalize error message presentation & formatting
- Then, re-write all of the descriptions to match, dang it.
- Probably ditching the "here" text for the hover effect is the main change.

Links

Currently looks like this...

Latest updates

Removed CodeMirror's native syntax highlighting for errors (all the pink highlights)
- I just overwrite the cm-error class basically
- Removed live-sync error line-highlight in the gutter
Testing out turning the document grey after the error
- Not sold on this yet

humphd · 2017-09-15T13:41:58Z

This is amazing. I love the playfulness, combined with how useful it is at the same time.

Pomax · 2017-09-19T20:19:56Z

How much work would it be to flag the "error" bit in red and the rest in dull greys? Having the </x> in grey makes it fall off the mental map a little.

flukeout · 2017-09-19T20:25:22Z

@Pomax - I agree, the error is really within that </x> so we shouldn't be hiding it in the grey. Currently the parser gives us the cursor position of the error, which I'm using to make the grey highlight. In the parser's estimation, the error started right before </x>. We'd need to extract some additional data to know where to end the red highlight we want to add.

We know that info is in there somwhere, since the expanded error message lets us hover to highlight </.

Pomax · 2017-09-19T20:51:56Z

Looks like slowparse itself might be flagging a bad case, too:

Rather than the error "the closing tag </p is missing the final >", it seems to "see" a tag </p> despite that final > not existing, and somehow relates it to the <body> tag.

… Also added a random "good" emoji when you fix your problem.

…tion when it opens.

Pomax · 2017-09-21T21:26:40Z

super sweet ♫ although of course I did just find another quirk:

intuitively this should be "following background:green;"

flukeout · 2017-09-21T21:27:55Z

I think at that point Slowparse should just delete your project. Can we do that?

Pomax · 2017-09-21T21:28:13Z

yes. yes we can.

humphd

Doing a first pass over the code--I realize you're not done. Left a few things to do. Looking good.

humphd · 2017-09-22T14:10:24Z

src/extensions/bramble-extensions.json

A few things you need to do here:

Move this to src/extensions/default instead of ../extra. Extensions in ../extra don't get statically cached.

Copy the images over:

"copy": [ "extensions/default/HTMLHinter/images/*.*" ],

Build the CSS from your LESS:

"less": { "dist/extensions/default/HTMLHinter/main.css": [ "dist/extensions/default/HTMLHinter/main.less" ] }

And then try and do npm run build and loading Brackets from dist/ vs. src/, and make sure everything loads properly.

humphd · 2017-09-22T14:11:42Z

src/extensions/extra/HTMLHinter/errorDisplay.js

+        // for future use you fucken idiots.
+
+        var goodEmojis = [
+            "biceps","halo","heart","peace","sunglasses","wink","horns","thumbs"


Spaces after , please

humphd · 2017-09-22T14:12:40Z

src/extensions/extra/HTMLHinter/errorDisplay.js

+                    return function() {
+                        el.classList.add(randomEmoji);
+                    };
+                }(errorToggle), 400);


Can you pull these numbers 400, 1300 out and define them above as ALL_CAPS variables that are better documented?

humphd · 2017-09-22T14:15:45Z

src/extensions/extra/HTMLHinter/errorDisplay.js

+        errorToggle.classList.add("nerd");
+
        var description = document.createElement("div");
        description.className = "errorPanel";


Why not move this divinto the template instead of defining here in code?

humphd · 2017-09-22T14:23:59Z

src/extensions/extra/HTMLHinter/inlineWidget.html

+    <!--
    <div class="regex-prop-summary">
-        <h4><u>HTML Error</u></h4>
+        <h4>We found a problem</h4>


l10n this string.

humphd · 2017-09-22T14:25:22Z

src/extensions/extra/HTMLHinter/main.js

+
+            }, errorDisplayTimeoutMS);
+        } else {
+            clearAllErrors();


Invert this logic so you don't have to indent:

if(!error) { clearAllErrors(); return; } // Do all that other code here...

humphd · 2017-09-22T14:27:00Z

src/extensions/extra/HTMLHinter/nls/root/strings.js

    "INVALID_TAG_NAME": "<p>The <code>&lt;</code> character <em data-highlight='[[openTag.start]],[[openTag.end]]'>here</em> appears to be the beginning of a tag, but is not followed by a valid tag name.</p> <p>If you just want a <code>&lt;</code> to appear on your Web page, try using <code>&amp;lt;</code> instead.</p> <p>Or, see a <a href='https://developer.mozilla.org/en/docs/Web/Guide/HTML/HTML5/HTML5_element_list'>list of HTML5 tags</a>.</p>",
    "JAVASCRIPT_URL_NOT_ALLOWED": "<p>Sorry, but security restrictions on this site prevent you from using the <code>javascript:</code> URL <em data-highlight='[[value.start]],[[value.end]]'>here</em>. If you really need to use JavaScript, consider using <a href='http://jsbin.com/'>jsbin</a> or <a href='http://jsfiddle.net/'>jsfiddle</a>.</p>",
-    "MISMATCHED_CLOSE_TAG": "<p>The closing <code>&lt;/[[closeTag.name]]&gt;</code> tag <em data-highlight='[[closeTag.start]],[[closeTag.end]]'>here</em> doesn't pair with the opening <code>&lt;[[openTag.name]]&gt;</code> tag <em data-highlight='[[openTag.start]],[[openTag.end]]'>here</em>. This is likely due to a missing <code>&lt;/[[openTag.name]]&gt;</code> tag.",
+    "MISMATCHED_CLOSE_TAG": "<p>The closing <code data-highlight='[[closeTag.start]],[[closeTag.end]]'>&lt;/[[closeTag.name]]&gt;</code> tag doesn't pair with the opening <code data-highlight='[[openTag.start]],[[openTag.end]]'>&lt;[[openTag.name]]&gt;</code> tag. This is likely due to a missing <code>&lt;/[[openTag.name]]&gt;</code> tag.",


We should figure out the right way to do these strings. It isn't to have them in here. cc @gideonthomas

All of this needs to be in the thimble repo in the /locales/en-US/editor.properties file. The tricky thing here is that there's a lot of code in the strings and I'm not sure how to handle that (it might be ok). Let's open up a PR for these strings in Thimble and get Theo to weigh in

out of curiosity, I thought all these strings would be in slowparse? Are we managing them in brackets instead?

slowparse has its own template messages, but those are pretty much "tailored for nothing", so we decided to only tap into Slowparse for the actual error + parse information, and have the "messages" that present that information in brackets, since that's the product that needs human language presentation, rather than slowparse itself.

…e helpful error.

…ngs.js file

flukeout · 2017-12-05T21:28:17Z

OK—I'm going to pick this up again and see if i can get it running. I don't want to let this die, so maybe we'll try to roll it out one rule at a time or something like that instead of all-at-once.

…three rules for now.

Pomax · 2018-01-18T18:47:57Z

@flukeout @humphd do we want to keep this PR open, or land it to its own branch that others can perhaps pick up on to finish? I don't think that we'd be finishing this work on our own, unfortunately =(

humphd · 2018-01-18T19:27:40Z

@Pomax how much more work is left on this? I haven't followed it closely enough to understand the cost of continuing. What I saw of the live demo stuff looked really good. I'll do work to finish it if it's close.

humphd · 2018-02-02T17:11:04Z

@Pomax, @gideonthomas I'd like to see this get landed. I'll try to finish it with my students this term.

Pomax · 2019-03-21T17:57:43Z

@humphd I'm going through my mentioned list to see if I can close anything out, this feels like it's probably a passed station by now? (Especially given Thimble's retirement)

humphd · 2019-03-21T18:05:13Z

Yes, we can close.

Beginning of slowparse work

dc962fe

flukeout added 5 commits September 19, 2017 10:56

Added a delay before showing a parsing error

e758608

Removed red highlighting from the Editor

f57e493

Removed an important rule that overrides stuff.

60d4f48

Makes text dark after an error

2877913

Indent fixes

95616ad

flukeout added 7 commits September 19, 2017 15:34

Error highlighting changes & styling

4b9cd3f

Added a little to-do note

e726468

Added an animation when we clear the parsing error.

b2fc9ca

This should make the emoji appear / disappear animations work better.…

f02bf02

… Also added a random "good" emoji when you fix your problem.

Made the nerd show the errors & hints.

b17cdac

Made it so the error speech bubble animates in with a bit of a transi…

686ad5c

…tion when it opens.

Now works with CSS pages and has some updated styling!

be29af8

flukeout added 2 commits September 21, 2017 18:01

Added arrow indicators to the hover highlights.

4830bb5

Some minor cleanup.

c3769e1

humphd reviewed Sep 22, 2017

View reviewed changes

Adds error titles to all messages

7312d18

flukeout mentioned this pull request Sep 26, 2017

added </option> as an optional ommited end tag mozilla/slowparse#81

Open

flukeout added 5 commits September 27, 2017 16:55

Introduced a new case for the mismatched_closing_tag to provide a mor…

035a849

…e helpful error.

More updates

9724e20

Updating the error reporting messages, leaving notes in the root/stri…

87f39dd

…ngs.js file

Updated notes

38e0783

A few more audits and notes in the strings file.

1e77c96

flukeout added 2 commits October 3, 2017 13:26

[WIP] - invalid child error.

78cb0db

Continued updates

0804b98

flukeout added 8 commits December 5, 2017 16:39

Fixed up highlights not disappearing when they should and limited to …

ba72063

…three rules for now.

Audited and enabled a few more rules.

9565073

Updating some rules

4e536ae

Added UNCLOSED_TAG rule

567c517

Added a couple more rules.

f962095

Added block in inline rule.

4de7d8c

Added additional reporting for testing

b3ff43b

submodule update

ee31758

humphd closed this Mar 21, 2019

[WIP] Adding slowparse for HTML validation #876

[WIP] Adding slowparse for HTML validation #876

Uh oh!

Conversation

flukeout commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

humphd commented Sep 15, 2017

Uh oh!

Pomax commented Sep 19, 2017

Uh oh!

flukeout commented Sep 19, 2017

Uh oh!

Pomax commented Sep 19, 2017

Uh oh!

Pomax commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flukeout commented Sep 21, 2017

Uh oh!

Pomax commented Sep 21, 2017

Uh oh!

humphd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pomax Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flukeout commented Dec 5, 2017

Uh oh!

Pomax commented Jan 18, 2018

Uh oh!

humphd commented Jan 18, 2018

Uh oh!

humphd commented Feb 2, 2018

Uh oh!

Pomax commented Mar 21, 2019

Uh oh!

humphd commented Mar 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

flukeout commented Sep 14, 2017 •

edited

Loading

Pomax commented Sep 21, 2017 •

edited

Loading

Pomax Sep 26, 2017 •

edited

Loading