Parse the whitespace surrounding equals in attribute correctly #503

ajaybhargavb · 2015-08-29T00:29:57Z

Issue - #123

Will add/update tests once the fix is accepted.

NTaylorMullen · 2015-08-30T22:24:06Z

src/Microsoft.AspNet.Razor/Parser/HtmlMarkupParser.Block.cs

+                // capture whitespace after attribute name (if any)
+                whitespaceAfterAttributeName = ReadWhile(
+                    sym => sym.Type == HtmlSymbolType.WhiteSpace ||
+                    sym.Type == HtmlSymbolType.NewLine);


I'm pretty sure this is just fine but we should validate that this newline and the previous whitespace symbol types can handle all the valid whitespace/newline characters defined by the spec for attributes:

- U+0020 SPACE - U+0009 CHARACTER TABULATION (tab) - U+000A LINE FEED (LF) - U+000C FORM FEED (FF) - U+000D CARRIAGE RETURN (CR)

Just checked and yes, it handles all characters in list. I'll add them to some tests.

Please unwrap this. Hard to read with expression split up but lambda parameter mixed in with one part.

Missing a change here?

NTaylorMullen · 2015-08-30T22:27:11Z

⌚ just for verification that the symbol types can handle the spec defined whtiespace.

NTaylorMullen · 2015-08-31T18:52:53Z

Design PR looks good then.

ajaybhargavb · 2015-09-02T00:14:43Z

Updated.

NTaylorMullen · 2015-09-02T02:26:19Z

src/Microsoft.AspNet.Razor/Parser/HtmlMarkupParser.Block.cs

-                while (!EndOfFile && !IsEndOfAttributeValue(quote, CurrentSymbol))
+                // Read the attribute value only if the value is quoted
+                // or if there is no whitespace between '=' and the unquoted value.
+                if (quote != HtmlSymbolType.Unknown || !whitespaceAfterEquals.Any())


What happens now with <input name=@value /> ?

This condition will return true. No change in the behavior.

NTaylorMullen · 2015-09-02T02:35:32Z

Need a few more tests:

Add a dynamic value one (attrName = @attrValue)
Add a data- variation with and without a dynamic value.
TagHelper test for bound/unbound variation of attribute. (Did the TagHelper bits just work?)

NTaylorMullen · 2015-09-02T02:35:37Z

⌚

ajaybhargavb · 2015-09-02T23:25:14Z

Updated.

NTaylorMullen · 2015-09-02T23:55:26Z

src/Microsoft.AspNet.Razor/Parser/HtmlMarkupParser.Block.cs

@@ -557,6 +587,11 @@ private void AttributePrefix(IEnumerable<HtmlSymbol> whitespace, IEnumerable<Htm
                // Output the attribute name, the equals and optional quote. Ex: foo="
                Output(SpanKind.Markup);

+                if (quote == HtmlSymbolType.Unknown && whitespaceAfterEquals.Any())


What text input makes this evaluate to true?

<p data-foo= blah />

When would this evaluate to false? You're in an else block for quote != HtmlSymbolType.Unknown || !whitespaceAfterEquals.Any(). Do a bit of Boolean algebra on !(A != B || !C) and you get A == B && C.

~~Looks line line 594 is unreachable. What is lost if that code is never evaluated?~~

Got it. @dougbu you may be misreading the diff. This is the else of if (attributeCanBeConditional)

@NTaylorMullen yup, thought I expanded everything in between but noooo...

NTaylorMullen · 2015-09-03T00:06:12Z

⌚

Need to add tests to validate following chars:

U+0009 CHARACTER TABULATION (tab)
U+000C FORM FEED (FF)

ajaybhargavb · 2015-09-03T01:04:22Z

Updated.

dougbu · 2015-09-03T02:31:20Z

src/Microsoft.AspNet.Razor/Parser/TagHelpers/TagHelperBlockRewriter.cs


                    // TODO: Handle malformed tags, if there's an '=' then there MUST be a value.
                    // https://github.com/aspnet/Razor/issues/104

                    SourceLocation symbolStartLocation;

+                    // Skip the whitespace preceding the start of the attribute value.
+                    var valueStartIndex = i + 1;


Add a code comment explaining + 1 skips over the = sign.

dougbu · 2015-09-03T02:48:14Z

Need to update tests with source locations to account for single-character Environment.NewLine before Travis will succeed.

dougbu · 2015-09-03T02:48:21Z

⌚

NTaylorMullen · 2015-09-03T03:14:16Z

once travis tests pass by addressing @dougbu's comment.

dougbu · 2015-09-03T04:21:18Z

test/Microsoft.AspNet.Razor.Test/Parser/Html/HtmlAttributeTest.cs

@@ -29,6 +29,39 @@ public void SimpleLiteralAttribute()
        }

        [Fact]
+        public void SimpleLiteralAttributeWithWhitespaceSurroundingEquals()
+        {
+            ParseBlockTest($"<a href \f{Environment.NewLine}= \t{Environment.NewLine}'Foo' />",


Use of Environment.NewLine instead of \r\n means you're not testing '\r' on Linux. I.e. I suggest being explicit about the characters and dropping Environment.NewLine. That should also fix the problems testing on Linux because the source locations will be identical.

I agree with @dougbu because:

\r\n is Windows., \n is Unix and \r is Mac

ajaybhargavb · 2015-09-03T18:14:45Z

Updated.

dougbu · 2015-09-03T20:55:56Z

⌚ for remaining unaddressed feedback.

ajaybhargavb · 2015-09-04T17:51:35Z

Updated.

dougbu · 2015-09-08T04:39:17Z

- #123 - Handled the corresponding cases in tag helper scenarios - Added unit and code generation tests

dnfclas added the cla-already-signed label Aug 29, 2015

ajaybhargavb force-pushed the ajbaaska/attributewhitespace.123 branch 2 times, most recently from fc23774 to 8c9a5b8 Compare August 29, 2015 00:49

NTaylorMullen reviewed Aug 30, 2015
View reviewed changes

ajaybhargavb force-pushed the ajbaaska/attributewhitespace.123 branch from 05a4705 to bdf36c6 Compare September 2, 2015 00:14

ajaybhargavb changed the title ~~[Design] Parse the whitespace surrounding equals in attribute correctly~~ Parse the whitespace surrounding equals in attribute correctly Sep 2, 2015

NTaylorMullen reviewed Sep 2, 2015
View reviewed changes

ajaybhargavb force-pushed the ajbaaska/attributewhitespace.123 branch from ab4b550 to 79f4b66 Compare September 3, 2015 01:06

dougbu reviewed Sep 3, 2015
View reviewed changes

ajaybhargavb force-pushed the ajbaaska/attributewhitespace.123 branch 2 times, most recently from a66a17c to b02e63c Compare September 8, 2015 18:17

Parse the whitespace surrounding equals in attribute correctly

08c8f9f

- #123 - Handled the corresponding cases in tag helper scenarios - Added unit and code generation tests

ajaybhargavb force-pushed the ajbaaska/attributewhitespace.123 branch from b02e63c to 08c8f9f Compare September 8, 2015 18:43

ajaybhargavb merged commit 08c8f9f into dev Sep 8, 2015

ajaybhargavb deleted the ajbaaska/attributewhitespace.123 branch September 8, 2015 18:56

Parse the whitespace surrounding equals in attribute correctly #503

Parse the whitespace surrounding equals in attribute correctly #503

Uh oh!

Conversation

ajaybhargavb commented Aug 29, 2015

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!

NTaylorMullen commented Aug 30, 2015

Uh oh!

NTaylorMullen commented Aug 31, 2015

Uh oh!

ajaybhargavb commented Sep 2, 2015

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!

NTaylorMullen commented Sep 2, 2015

Uh oh!

NTaylorMullen commented Sep 2, 2015

Uh oh!

ajaybhargavb commented Sep 2, 2015

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!

NTaylorMullen commented Sep 3, 2015

Uh oh!

ajaybhargavb commented Sep 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu commented Sep 3, 2015

Uh oh!

dougbu commented Sep 3, 2015

Uh oh!

NTaylorMullen commented Sep 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaybhargavb commented Sep 3, 2015

Uh oh!

dougbu commented Sep 3, 2015

Uh oh!

ajaybhargavb commented Sep 4, 2015

Uh oh!

dougbu commented Sep 8, 2015

Uh oh!

Uh oh!