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.

Sign up for GitHub

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

Jump to bottom

Centralize checks for <a>, <area>, <form>, and <link> #2613

Merged
merged 5 commits into from
Sep 22, 2017
Merged

Centralize checks for <a>, <area>, <form>, and <link> #2613

merged 5 commits into from
Sep 22, 2017

Conversation

annevk
Copy link
Member

@annevk annevk commented May 2, 2017

@annevk annevk force-pushed the annevk/simplify-a-area-link branch from 6f5078f to 3becdd1 Compare May 2, 2017 14:18
annevk added a commit to web-platform-tests/wpt that referenced this pull request May 2, 2017
@annevk
Disconnected <a> should still navigate
ba67ac5 
Follow-up test for whatwg/html#2613.
@annevk annevk mentioned this pull request May 2, 2017
Merged
@annevk
Copy link
Member Author

annevk commented May 2, 2017

Sigh. This is wrong, because we'd also have to add the check to "download the hyperlink". The whole setup seems rather broken though.

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label May 2, 2017
@annevk
Copy link
Member Author

annevk commented May 2, 2017

And with broken I mean that "download the hyperlink" shouldn't have to do its own URL processing and such.

@annevk annevk force-pushed the annevk/simplify-a-area-link branch from 3becdd1 to 5cd556f Compare May 3, 2017 08:35
@annevk annevk changed the title Centralize active document checks for <a>, <area>, and <link> Centralize checks for <a>, <area>, <form>, and <link> May 3, 2017
@annevk annevk added normative change topic: forms topic: navigation and removed do not merge yet Pull request must not be merged per rationale in comment labels May 3, 2017
@annevk
Copy link
Member Author

annevk commented May 3, 2017

I updated the commit to address #2615 per how I think it should be addressed and also addressed my own review feedback above. I will also update the three test PRs to align with the proposed change.

@annevk annevk mentioned this pull request May 3, 2017
Merged
@annevk annevk force-pushed the annevk/simplify-a-area-link branch from 5cd556f to 475c12a Compare May 9, 2017 07:38
@domenic domenic added the do not merge yet Pull request must not be merged per rationale in comment label May 10, 2017
domenic
domenic approved these changes May 10, 2017
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM with nit, but we'll mark "do not merge yet" pending compat analysis in #2615.

source Outdated
</ol>
<!-- c.f. <link> and <a>'s similar section -->
<p>The <span>activation behavior</span> of <code>area</code> elements is to <span
data-x="following hyperlinks">Follow the hyperlink</span> or <span data-x="downloading
Copy link
Member

Choose a reason for hiding this comment

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

Nit: lowercase "follow"

@annevk
Centralize checks for <a>, <area>, <form>, and <link>
dd32b3d 
Require that they are all connected to a document that is fully
active. There is enough difference between implementations that this
appears to be web compatible.

Tests:

* web-platform-tests/wpt#5758
* web-platform-tests/wpt#5759
* web-platform-tests/wpt#5761

Fixes #2615.
@annevk annevk force-pushed the annevk/simplify-a-area-link branch from 475c12a to dd32b3d Compare August 23, 2017 13:20
@annevk annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Aug 23, 2017
annevk added a commit to web-platform-tests/wpt that referenced this pull request Aug 23, 2017
@annevk
Disconnected <a> should still navigate
544ff63 
Follow-up test for whatwg/html#2613.
@annevk
Copy link
Member Author

annevk commented Aug 23, 2017

Updated PR and tests per findings in #2615 and #2708 (has a list of browser bugs that need updating upon this landing).

domenic
domenic reviewed Aug 24, 2017
View reviewed changes
source Outdated
<li><p>If <var>continue</var> is false, then abort these steps.
<li><p>If <var>continue</var> is false, then return.</p></li>

<li><p>If <var>form</var> <span>cannot navigate</span>, then return.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

Should we expand the HTML comment here into a visible note?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we could.

source
@@ -22935,11 +22922,23 @@ interface <dfn>HTMLHyperlinkElementUtils</dfn> {

<h4>Following hyperlinks</h4>

<p>An element <var>element</var> <dfn>cannot navigate</dfn> if one of the following is true:</p>
Copy link
Member

@domenic domenic Aug 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This section is full of double negatives ("an element cannot navigate if it's node document is not fully active"), but I think it's pretty nice at the call sites, so I'm not sure what to do. I guess you could define "can navigate" and then say "cannot navigate = the negation of can navigate".

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather just leave it as-is unless we get more complaints. Or until we turn everything into abstract operations, if ever.

source
@@ -22935,11 +22922,23 @@ interface <dfn>HTMLHyperlinkElementUtils</dfn> {

<h4>Following hyperlinks</h4>

<p>An element <var>element</var> <dfn>cannot navigate</dfn> if one of the following is true:</p>
Copy link
Member

Choose a reason for hiding this comment

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

FYI https://english.stackexchange.com/questions/204006/cannot-vs-can-not since my brain momentarily started thinking "cannot" is weird. (TLDR: it's correct.)

@domenic
Copy link
Member

domenic commented Aug 24, 2017

Can we also get tests for both locations in the form algorithm?

@annevk
Copy link
Member Author

annevk commented Sep 15, 2017

Made the note visible.

@annevk
Copy link
Member Author

annevk commented Sep 15, 2017

Updated tests too.

@annevk
Copy link
Member Author

annevk commented Sep 20, 2017

Updated tests are at web-platform-tests/wpt#5759 and web-platform-tests/wpt#5761 btw.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Sep 22, 2017
@annevk
Disconnected <a> should not navigate
055df51 
Follow-up tests for whatwg/html#2613.
@annevk annevk merged commit f3c354a into master Sep 22, 2017
@annevk annevk deleted the annevk/simplify-a-area-link branch September 22, 2017 12:05
@annevk
Copy link
Member Author

annevk commented Sep 22, 2017

Bugs for <form>:

For <a>/<area>:

rachelandrew pushed a commit to rachelandrew/web-platform-tests that referenced this pull request Nov 8, 2017
@annevk @rachelandrew
Disconnected <a> should not navigate
70fcd99 
Follow-up tests for whatwg/html#2613.
jakearchibald pushed a commit to jakearchibald/web-platform-tests that referenced this pull request Nov 16, 2017
@annevk @jakearchibald
Disconnected <a> should not navigate
275ebb4 
Follow-up tests for whatwg/html#2613.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domenic domenic domenic approved these changes

Assignees

@domenic domenic

Labels
normative change topic: forms topic: navigation
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@annevk @domenic