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

Add html-validation to detect malformed markup #15433

Closed
wants to merge 11 commits into from

Conversation

octopusinvitro
Copy link
Contributor

@octopusinvitro octopusinvitro commented Sep 2, 2016

Firefox was showing the source code with the heading tags in red. I discovered that it was due to a missing link-closing tag, so I added to the main layout heading. But it is much better if we have an automated way to detect and correct those things. Hence this addition. The gem uses the tidy-html5 validator, which can be installed locally optionally.

I ran the validator against each page and did a separate commit for each, including the reported errors for that page.

@zarino , I had to remove some of your code (basically attributes in the main page's select) and add code on some other places (mostly alt attributes to images), so I would like you to take a look at this as well if you please.

@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15433 September 2, 2016 12:09 Inactive
@tmtmtmtm
Copy link
Contributor

tmtmtmtm commented Sep 2, 2016

A bugfix without a regression test… surely not? ;)

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

octopusinvitro commented Sep 2, 2016

@tmtmtmtm should I research into adding something that runs a check against the W3C validator when running the web tests

@tmtmtmtm
Copy link
Contributor

tmtmtmtm commented Sep 2, 2016

I don't think you need to run against an external validator. Nokogiri in strict mode is probably all we need.

Changing the subject to Nokogiri::HTML(last_response.body) { |config| config.strict } lets us then run a test later like

it "has no HTML errors" do
   subject.errors.must_be_empty
end

@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15433 September 19, 2016 15:17 Inactive
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15433 September 20, 2016 16:16 Inactive
@octopusinvitro octopusinvitro changed the title Add missing closing tag for link in site header Add ruby-vnu to detect malformed markup Sep 20, 2016
@zarino
Copy link
Member

zarino commented Sep 20, 2016

@octopusinvitro – This looks fine to me, apart from the “Search by country name” hint text now being missing from the search input on the homepage.

Before:

screen shot 2016-09-20 at 17 24 35

After:

screen shot 2016-09-20 at 17 24 45

The text input (generated by selectToAutocomplete) uses copy-attributes-to-text-field to soak up whatever HTML attributes the source <select> element had. I guess, a satisfactory solution if we still want W3C compliant markup, would be to add the placeholder attribute back in with JavaScript, immediately before we call . selectToAutocomplete() in main.js.

@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15433 September 20, 2016 16:47 Inactive
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15433 September 20, 2016 16:50 Inactive
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15433 September 20, 2016 16:54 Inactive
Errors corrected:
- An "img" element must have an "alt" attribute, except under certain
conditions. For details, consult guidance on providing text alternatives
for images. (All occurrences)
Errors corrected:
- An "img" element must have an "alt" attribute, except under certain
conditions. For details, consult guidance on providing text alternatives
for images. At line 254, column 69.
- End tag "div" seen, but there were open elements. At line 256, column 12.
- Unclosed element "a". At line 252, column 77.
Errors corrected:
 - No errors
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
- & did not start a character reference. (& probably should have been escaped as &amp;.) At line 1140, column 93
- & did not start a character reference. (& probably should have been escaped as &amp;.) At line 1140, column 123
- & did not start a character reference. (& probably should have been escaped as &amp;.) At line 1140, column 139
- & did not start a character reference. (& probably should have been escaped as &amp;.) At line 1152, column 90
- & did not start a character reference. (& probably should have been escaped as &amp;.) At line 1152, column 114
- & did not start a character reference. (& probably should have been escaped as &amp;.) At line 1152, column 130
- & did not start a character reference. (& probably should have been escaped as &amp;.) At line 1159, column 219
- & did not start a character reference. (& probably should have been escaped as &amp;.) At line 1159, column 256
- & did not start a character reference. (& probably should have been escaped as &amp;.) At line 1159, column 272
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15433 September 22, 2016 11:54 Inactive
@octopusinvitro
Copy link
Contributor Author

octopusinvitro commented Sep 22, 2016

@tmtmtmtm I rebased all commits in the PR to:

  • Remove the commits that were adding the old nu-validator
  • Remove the commit that validates the home page, and leave that to another PR where Make the hint text visible again #15562 is also addressed
  • Moved the tests to their dedicated pages and remove the html_validation test file
  • I had to add a t/web/countries.rb page because it didn't exist.
  • It puts back the placeholder text in the select element through JavaScript. @zarino might want to check this commit out (1994b19)

placeholder

Closes #15562

@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15433 September 22, 2016 13:34 Inactive
@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15433 September 22, 2016 14:11 Inactive
@@ -305,6 +305,11 @@ $(function(){
});

// http://baymard.com/labs/country-selector
$('.js-select-to-autocomplete').attr({
Copy link
Member

@zarino zarino Sep 22, 2016

Choose a reason for hiding this comment

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

A small thing, but it’s generally good practice to avoid querying the DOM more often than is necessary. So, you might query the DOM once and then cache the result in a variable to be reused again later:

var $select = $('.js-select-to-autocomplete');
$select.attr({ … });
$select.selectToAutoComplete();
$select.on('change', function(){ … });

Or, since you have a fairly small number of methods to call, you could save the overhead of another variable, and just use jQuery chaining:

$('.js-select-to-autocomplete')
  .attr({ … })
  .selectToAutocomplete()
  .on('change', function(){ … });

@tmtmtmtm tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15433 September 22, 2016 14:26 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.

Generally looking good, but the repetition of the

skip if `which tidy`.empty?

seems unnecessary and error-prone. I think it would be a lot cleaner and simpler to move that check inside the last_response_must_be_valid check.

@octopusinvitro
Copy link
Contributor Author

@tmtmtmtm feedback addressed

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.

NB: this still seems like it's doing way too many things at once. Not being able to fix any of the problems until we've fixed all of the problems in a monster PR seems like completely the wrong approach.

class="person-card__image">
<noscript><img src="<%= person.proxy_image %>" class="person-card__image"></noscript>
class="person-card__image"
alt="Member headshot">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a plain "Member headshot" sufficient, when there might be hundreds of these across the page? Perhaps we should include the member's name here?

<% else %>
<img src="/images/person-placeholder-108px.png" class="person-card__image">
<img src="/images/person-placeholder-108px.png" class="person-card__image"
alt="Placeholder image">
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto here. "Placeholder image for Freddy McFreedy" seems like it might be more useful.

@octopusinvitro
Copy link
Contributor Author

octopusinvitro commented Oct 11, 2016

Closing this as it was handled in separate PRs for every page and the only ones pending are #15587 and #15594

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

Successfully merging this pull request may close these issues.

4 participants