-
Notifications
You must be signed in to change notification settings - Fork 363
Add new audit rule that warns about bad page title #187
base: master
Are you sure you want to change the base?
Conversation
…s, add tests for audit rule
}, | ||
test: function(scope) { | ||
// test length of title | ||
if (document.title.length >= 66) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document
may not be what you expect here.
scope
is an element which relevantElementMatcher
returns true on (see https://github.com/GoogleChrome/accessibility-developer-tools/blob/master/src/js/AuditRule.js#L243) and may be inside an iframe, in which case its owner document will be something other than document
which will be the owner document of the context where this script is running. For example, if I inject this script into my page, document
will be the top level document for my page, but scope
may be inside an iframe which has its own <title>
, which would be what I would want to check.
Since the element being passed in will be a <title>
, you may want to rename scope
to title
or titleEl
(like how https://github.com/GoogleChrome/accessibility-developer-tools/blob/master/src/audits/ImageWithoutAltText.js calls the argument to its test()
function image
). Then you can check the textContent
of the title element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used https://github.com/GoogleChrome/accessibility-developer-tools/blob/master/src/audits/PageWithoutTitle.js
as a reference material. Although I see now what you meant by 'test' calling the functions 'argument'. It calls the scope and performs a query, therefore finding the title across the scope.
I will try to fix the script as you suggested.
test: function(title) { | ||
var titleText = title.textContent; | ||
var badChars = /([-/\\]|[.]$)/; | ||
return titleText.length > 65 || badChars.test(titleText); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this length and not something else?
Thanks for the PR @kristapsmelderis. I think there is a big open question here about whether there is any corresponding standard or widely accepted convention associated with this rule. I understand the spirit of the rule, but the specifics seem a little arbitrary. Can you fill us in more? |
Hello @ckundo Apologies, it has been a while;
As for standards. Nothing concrete, however: "The title should ideally be less than 64 characters in length." - the focus is on technical aspects. Nothing about accessibility. The examples show presence of symbols such as '.' - I can only assume that the use case: "what if titles are read by screen readers" has been left out from standard considerations when they were written. P.S. I do not wish to pollute ADT with unclosed PRs. This has been left out of my memory for a long time. I am ok if the PR is not accepted and simply closed. It would require a "fresh" attempt if such rules would be accepted, in my opinion. |
No description provided.