Skip to content
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

Fix #8806: Allow about:blank as reference URL #9421

Merged
merged 1 commit into from Feb 15, 2018

Conversation

Projects
None yet
5 participants
@vedantc98
Copy link
Contributor

vedantc98 commented Feb 7, 2018

Fixes #8806.

This change is Reviewable

@wpt-pr-bot wpt-pr-bot added the infra label Feb 7, 2018

@wpt-pr-bot wpt-pr-bot requested review from gsnedders and jgraham Feb 7, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Feb 7, 2018

Build PASSED

Started: 2018-02-14 20:10:19
Finished: 2018-02-14 20:26:43

View more information about this build on:

@vedantc98 vedantc98 force-pushed the vedantc98:abt_blnk branch 2 times, most recently from 72d01ee to e75a459 Feb 7, 2018

@@ -430,7 +430,7 @@ def check_parsed(repo_root, path, f):
for reftest_node in source_file.reftest_nodes:
href = reftest_node.attrib.get("href", "").strip(space_chars)
parts = urlsplit(href)
if parts.scheme or parts.netloc:
if (parts.scheme or parts.netloc) and not href == "about:blank":

This comment has been minimized.

Copy link
@gsnedders

gsnedders Feb 7, 2018

Contributor

I wonder if we should make this parts == ("about", "", "blank", "", "") or parts == urlsplit("about:blank") to deal with characters being escaped in the path (blank). @jgraham?

This comment has been minimized.

Copy link
@jgraham

jgraham Feb 7, 2018

Contributor

I hope no one ever does that, but I guess the latter form is a good idea.

This comment has been minimized.

Copy link
@vedantc98

vedantc98 Feb 7, 2018

Author Contributor

I'll update the condition, but I'm unsure of how to test this bug.
Since about:blank will be treated as a relative file when I add it to the href attribute in the test file.

This comment has been minimized.

Copy link
@vedantc98

vedantc98 Feb 12, 2018

Author Contributor

@jgraham What changes should I make?

This comment has been minimized.

Copy link
@gsnedders

gsnedders Feb 13, 2018

Contributor

Replace the right hand side of the and here with parts != urlsplit("about:blank") and add a second test with <link rel=match href=about:bl%61nk>.

def test_about_blank_as_ref(caplog):
with _mock_lint("check_path") as mocked_check_path:
with _mock_lint("check_file_contents") as mocked_check_file_contents:
rv = lint(_dummy_repo, ["about:blank"], "normal")

This comment has been minimized.

Copy link
@gsnedders

gsnedders Feb 13, 2018

Contributor

You don't want about:blank here (that'll just try and lint a file called about:blank); you want to add a file to tools/lint/tests/dummy/ref with a <link rel=match href=about:blank> in it (see the other files in that directory), and then you'll need to add a second file for the other test mentioned above.

@@ -430,7 +430,7 @@ def check_parsed(repo_root, path, f):
for reftest_node in source_file.reftest_nodes:
href = reftest_node.attrib.get("href", "").strip(space_chars)
parts = urlsplit(href)
if parts.scheme or parts.netloc:
if (parts.scheme or parts.netloc) and not href == "about:blank":

This comment has been minimized.

Copy link
@gsnedders

gsnedders Feb 13, 2018

Contributor

Replace the right hand side of the and here with parts != urlsplit("about:blank") and add a second test with <link rel=match href=about:bl%61nk>.

@vedantc98 vedantc98 force-pushed the vedantc98:abt_blnk branch 2 times, most recently from 3069f4d to 8d5217f Feb 13, 2018

@vedantc98

This comment has been minimized.

Copy link
Contributor Author

vedantc98 commented Feb 13, 2018

@gsnedders I've made those changes, hope that's good enough.

@vedantc98 vedantc98 force-pushed the vedantc98:abt_blnk branch from fd3df6b to 44a13af Feb 14, 2018

@vedantc98 vedantc98 force-pushed the vedantc98:abt_blnk branch from 44a13af to fe0e905 Feb 14, 2018

@gsnedders gsnedders merged commit c23fdbd into web-platform-tests:master Feb 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.