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

Use tidy-html5 to validate the home page #15580

Closed
wants to merge 2 commits into from

Conversation

octopusinvitro
Copy link
Contributor

@octopusinvitro octopusinvitro commented Sep 26, 2016

What does this do?

HTML-validates the home page

Why was this needed?

HTML was broken

Relevant Issue(s)

everypolitician/everypolitician#505
Closes #15562

Implementation notes

The attributes removed to make the validation pass where added back with JavaScript.

Screenshots

screen shot 2016-09-20 at 17 24 35

(Image by @zarino )

Notes to Reviewer

Errors corrected:

  • Attribute "placeholder" not allowed on element "select" at this point. At line 86, column 189
  • Attribute "autocorrect" not allowed on element "select" at this point. At line 86, column 189
  • Attribute "autocomplete" not allowed on element "select" at this point. At line 86, column 189

Notes to Merger

None

Errors corrected:
- Attribute "placeholder" not allowed on element "select" at this point. At line 86, column 189
- Attribute "autocorrect" not allowed on element "select" at this point. At line 86, column 189
- Attribute "autocomplete" not allowed on element "select" at this point. At line 86, column 189
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15580 September 26, 2016 18:04 Inactive
Copy link
Contributor

@tmtmtmtm tmtmtmtm left a comment

Choose a reason for hiding this comment

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

@octopusinvitro this is changing the behaviour of a visible part of the site, so I think this one does need a screenshot to show that it's still working the same way after the change.

@tmtmtmtm tmtmtmtm assigned octopusinvitro and unassigned tmtmtmtm Sep 28, 2016
@octopusinvitro
Copy link
Contributor Author

@tmtmtmtm I added a screenshot.

@zarino
Copy link
Member

zarino commented Sep 28, 2016

(Image by @zarino )

Maybe I'm missing something, but what's the point of including a screenshot in a PR, if the screenshot was taken before the changes in the PR were even written?

@octopusinvitro
Copy link
Contributor Author

@zarino Because nothing has changed and I am too lazy to take another identical screenshot! 😄

@davewhiteland
Copy link
Contributor

I would like to be convinced that we need to do this: placeholder works as an inbuilt feature of modern browsers, without any requirement for JS (yay). But this way we're only using placeholder if JS is enabled/available, which seems like an accessibility step backwards.

My understanding of this is that we're using JS to add these attributes after validation because the validator will otherwise report them. If so, it feels odd to be introducing a JS dependency to get around a problem with the validator.

@zarino
Copy link
Member

zarino commented Sep 28, 2016

My understanding of this is that we're using JS to add these attributes after validation because the validator will otherwise report them. If so, it feels odd to be introducing a JS dependency to get around a problem with the validator.

We’re adding them because, to the letter of the law, the HTML5 spec[1] doesn't allow placeholder, autocorrect, or autcomplete attributes to be applied to a select element.

If you ask me – it's a classic example of when you can safely ignore the spec. But that's just me.

[1] https://www.w3.org/TR/html5/forms.html#the-select-element

@octopusinvitro
Copy link
Contributor Author

octopusinvitro commented Sep 28, 2016

@davewhiteland The easy fix for that would be to include an actual DOM element in the document just before the select with the placeholder/informative, text, and if JS is enabled, it would hide that element, pick the informative text it contains, and use it as a placeholder in the select.

@zarino what do you think?

@zarino
Copy link
Member

zarino commented Sep 28, 2016

@zarino what do you think?

Including the placeholder text in a <label> or whatever, before the <select>, and then removing the element and applying its content as the placeholder attribute on the <select> before it gets selectToAutocomplete’d—as you suggest—is probably the best idea. Not only does it gracefully degrade if JavaScript doesn't run, but it also avoids the thorny semantic argument over whether a command like “Search by country name” is actually a suitable thing to be put into a placeholder attribute.

Question is: what’s the best way of marking up such a “hint” element. A second label is one option:

<p>
  <label for="country-selector">Find representatives from your country:</label>
  <label for="country-selector">Search by country name</label>
</p>
<%= erb :country_selector %>

A separate element inside the same label is another option:

<p>
  <label for="country-selector">
    Find representatives from your country:
    <span class="js-turn-me-into-a-placeholder">Search by country name</span>
  </label>
</p>
<%= erb :country_selector %>

@octopusinvitro
Copy link
Contributor Author

octopusinvitro commented Sep 29, 2016

@zarino I like the second one better 😃

I would add a third one:
The text "Search by country name" feels more like a proper label for the select. So another option could be to make that be the actual label and "Find representatives in your country" can be a heading or another paragraph:

<p class="fancy-paragraph">Find representatives from your country:</p>
<p class="js-turn-me-into-a-placeholder">
  <label for="country-selector">Search by country name</label>
</p>
<%= erb :country_selector %>

@zarino
Copy link
Member

zarino commented Sep 29, 2016

The text "Search by country name" feels more like a proper label for the select. So another option could be to make that be the actual label and "Find representatives in your country" can be a heading or another paragraph

I'd say that's fine, as long as you can still click the "Find representatives in your country" and it focusses the search box. (That’s something you get for free when you use a <label>.)

@octopusinvitro
Copy link
Contributor Author

@zarino ah yes you are right. I'll go with your second option 👍

@tmtmtmtm
Copy link
Contributor

I think I'm going to put a bit of kibosh on this one, at least for now. I can certainly see some value in having all our HTML be completely valid, but this is already several levels removed from the original trigger for this, which was about detecting mismatched tags.

The value of the changes being discussed here now seem a little too minimal for the amount of effort, when we have so many other important things to do.

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

Successfully merging this pull request may close these issues.

4 participants