Skip to content

Revamp coords parsing to be more compatible and less insane #514

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

Merged
merged 1 commit into from
Jan 21, 2016

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Jan 14, 2016

The old parser tried to mimic IE as close as possible. Now Edge is instead interested in aligning with Gecko/WebKit. This new algorithm was designed by studying implementations as well as invalid Web content.

At the same time, support parsing of floating point numbers, as suggested by @travisleithead in the bug below.

Fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=28148.

@zcorpan zcorpan force-pushed the revamp-coords-parsing branch from 5a1c99a to c2eb90e Compare January 14, 2016 09:27
@annevk
Copy link
Member

annevk commented Jan 14, 2016

Do we have tests for how many browsers this new algorithm matches?

@zcorpan
Copy link
Member Author

zcorpan commented Jan 14, 2016

I don't know how to test this in an automated fashion :( since the parsed list is not exposed and the only way to tell what happens, AFAIK, is with hit testing or looking at focus rings.

@zcorpan
Copy link
Member Author

zcorpan commented Jan 14, 2016

But as noted in the bug, the new algorithm doesn't exactly match any one browser, but is more of a mix of the different approaches that seems to give good results for actual Web content.

@zcorpan
Copy link
Member Author

zcorpan commented Jan 14, 2016

It could be tested using elementFromPoint although this Gecko bug prevents it from working in Gecko.

@zcorpan
Copy link
Member Author

zcorpan commented Jan 14, 2016

@bzbarsky @jchaffraix @hober is there interest in aligning coords parsing? I'd be happy to write some tests in web-platform-tests.

@fsoder
Copy link

fsoder commented Jan 14, 2016

If there's sufficient interest for this from other parties, I can work on aligning the parser in Blink.

@Ms2ger
Copy link
Member

Ms2ger commented Jan 14, 2016

I'm definitely interested in having tests by the time we implement image maps in Servo :)

@zcorpan zcorpan force-pushed the revamp-coords-parsing branch from c2eb90e to ca8c695 Compare January 14, 2016 13:44
zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Jan 14, 2016
@bzbarsky
Copy link
Contributor

I'm interested in aligning parsing here at least in theory. This is not code that's well-owned in Gecko and hasn't been changed much in a long time; I don't feel like I have a good understanding of what's likely to be compatible here and what's not. But if there's a simple enough and compatible spec I would certainly support it getting implemented.

@annevk
Copy link
Member

annevk commented Jan 15, 2016

I would be okay with merging this given that the old algorithm aligned with IE and nobody wants to implement that anymore, and given that we now have tests and bugs filed.

@zcorpan zcorpan force-pushed the revamp-coords-parsing branch from ca8c695 to 14a0201 Compare January 18, 2016 08:35
@zcorpan
Copy link
Member Author

zcorpan commented Jan 20, 2016

Tests are reviewed now and @fsoder has an implementation that passes the tests. Can we merge, or does someone else want to review also?


<li>Let <var>negated</var> be false.</li>
<li><p><span>Collect a sequence of characters</span> that are not <span data-x="space
character">space characters</span>, U+002C COMMA, U+003B SEMICOLON characters, and let
Copy link
Member

Choose a reason for hiding this comment

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

"or" before U+003B

@domenic
Copy link
Member

domenic commented Jan 20, 2016

A couple minor editorial nits. LGTM once fixed. Very nice to just reuse the float algorithm instead of the existing complicated thing.

@domenic domenic assigned zcorpan and unassigned domenic Jan 20, 2016
The old parser tried to mimic IE as close as possible. Now Edge
is instead interested in aligning with Gecko/WebKit. This new
algorithm was designed by studying implementations as well as
invalid Web content.

At the same time, support parsing of floating point numbers, as
suggested by Travis Leithead in the bug below.

Fixes https://www.w3.org/Bugs/Public/show_bug.cgi?id=28148.
@zcorpan zcorpan force-pushed the revamp-coords-parsing branch from 14a0201 to ead6cfe Compare January 21, 2016 10:09
@zcorpan
Copy link
Member Author

zcorpan commented Jan 21, 2016

@@ -4557,13 +4557,13 @@ a.setAttribute('href', 'http://example.com/'); // change the content attribute d
      U+002E FULL STOP, or U+002D HYPHEN-MINUS characters. This skips past leading garbage.</p></li>

      <li><p><span>Collect a sequence of characters</span> that are not <span data-x="space
-     character">space characters</span>, U+002C COMMA, U+003B SEMICOLON characters, and let
+     character">space characters</span>, U+002C COMMA, or U+003B SEMICOLON characters, and let
      <var>unparsed number</var> be the result.</p></li>

-     <li><p>Let <var>number</var> be the result of using the <span>rules for parsing floating-point
-     number values</span> for <var>unparsed number</var>.</p></li>
+     <li><p>Let <var>number</var> be the result of parsing <var>unparsed number</var> using the
+     <span>rules for parsing floating-point number values</span>.</p></li>

-     <li><p>If <var>number</var> is an error, let <var>number</var> be zero.</p></li>
+     <li><p>If <var>number</var> is an error, set <var>number</var> to zero.</p></li>

      <li><p>Append <var>number</var> to <var>numbers</var>.</p></li>

@zcorpan zcorpan merged commit ead6cfe into master Jan 21, 2016
@zcorpan zcorpan deleted the revamp-coords-parsing branch January 21, 2016 10:10
@zcorpan
Copy link
Member Author

zcorpan commented Jan 21, 2016

Thanks for the review!

zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Jan 21, 2016
@zcorpan
Copy link
Member Author

zcorpan commented Jan 28, 2016

FYI, the relevant fixes have now landed in Blink. @fsoder++

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

Successfully merging this pull request may close these issues.

6 participants