core(link-text): localize link text audit #14927

raffaelj · 2023-03-24T18:49:51Z

Summary

The link-text audit produces false positives, because it is not localized. I changed the anchor-elements gatherer and the link-text audit to check for the current language.

Related Issues/PRs

Fixes #14845

also added German strings to block list

google-cla · 2023-03-24T18:49:56Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

no functional change, but less if/else soup and more descriptive function name

if opening html tag is the only place with a lang attribute

connorjclark · 2023-05-03T19:28:47Z

cli/test/fixtures/seo/seo-failure-cases.html

@@ -5,7 +5,7 @@
 * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
 -->

-<html>
+<html lang="en">


It'd be good to update this smoke test to include cases that exercise the new logic (getLangOfInnerText). The expectations for this smoke tests are found in seo-failing.js. If the current smoke expectations were more explicit, it would have this for the link-text audit (currently it just checks for 4 failures, but it'd be good to be more explicit now):

'link-text': { score: 0, displayValue: '4 links found', details: { items: [ {text: 'click this'}, {text: 'click this'}, {text: 'CLICK THIS'}, {text: 'click this'}, ], }, }

Can you devise some cases here to make sure getLangOfInnerText works as expected? If you need to create a new file HTML in fixtures/seo and a new accompanying expectation file in test-definitions/seo-mixed-language.js, that's OK.

I changed the smoke test seo-failing to be more explicit, including the language attribute and a German example. I also added a new smoke test seo-mixed-language with many edge cases (German and English examples).

connorjclark · 2023-05-03T19:36:58Z

core/gather/gatherers/anchor-elements.js

+  const langElements = getElementsInDocument('[lang]'); // eslint-disable-line no-undef
+  const docHasOnlyHtmlLangAttr = (langElements.length === 1) &&
+    langElements[0].nodeName === 'HTML';
+  const lang = !docHasOnlyHtmlLangAttr ? null : langElements[0].getAttribute('lang');


this needs to be normalized some how. Consider the valid language code en-US.

I don't recall how we could actually normalize to the base language code. In shared/localization/locales.js we form a mapping explicitly. I suppose here we could just split by - and return the first part to use as the loclae.

I wouldn't normalize here. The gatherer just collects the lang code and passes it to the audit. If it should get normalized, then the last tool in the chain (the audit) should do that.

See also my response below to @adamraine about keeping the loop over language code segments.

connorjclark · 2023-05-03T19:37:24Z

cli/test/fixtures/seo/seo-tester.html

@@ -5,7 +5,7 @@
 * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
 -->

-<html>
+<html lang="en">


I'm a little uneasy how this is now required for the audit to function at all.

We discussed this, and we think it'd be best to do the current behavior of the audit when the document does not declare a language. I think that's what the PR is currently doing, but some tests in the audit would be good to verify.

Just to be sure - What do you mean with "current behavior"? The current behavior before this PR or the current behavior of this PR?

My implementation ignores links if the language is not declared. I also added a new test case to core/test/audits/seo/link-text-test.js to reflect that new behavior.

Just to be sure - What do you mean with "current behavior"? The current behavior before this PR or the current behavior of this PR?

The current behavior before this PR is what we want to fall back to if a lang is not specified.

raffaelj · 2023-05-08T12:24:18Z

@connorjclark @adamraine Thanks for the review. The easy ones are fixed. I wrote a smoke test, but I don't have more time today. I'll do some cleanup and push the changes tomorrow.

I'll have a look at the other suggestions this week (or weekend).

connorjclark · 2023-05-08T22:07:07Z

core/test/audits/seo/link-text-test.js

@@ -10,15 +10,15 @@ import LinkTextAudit from '../../../audits/seo/link-text.js';

 describe('SEO: link text audit', () => {
  it('fails when link with non descriptive text is found', () => {
-    const invalidLink = {href: 'https://example.com/otherpage.html', text: 'click here', rel: ''};


I think adding a new test or two that has non-english text that is bad would be clearer. All these existing test cases could leave off lang: 'en' since it doesn't modify the thing they are asserting; plus that gives us coverage for the common case of there being no document language.

I cleaned it up and added more tests.

raffaelj · 2023-05-11T10:32:41Z

I think, I addressed all annotations.

If you really want to change the audit to use only the first segment of a language code (e. g. en from en-US), I could do it. But I still think, it's better to keep my current implementation.

A few questions are still open from #14845 (comment):

headline "Fallbacks"
- fallback to pragma-set-default-language or HTTP header
headline "Other thoughts"
- more exceptions
- change weight of audit

raffaelj · 2023-06-06T11:18:50Z

@connorjclark @adamraine Can I do anything to finish this PR?

adamraine

Sorry for the delay @raffaelj, and thanks for being patient

adamraine · 2023-08-23T22:56:03Z

cli/test/fixtures/seo/seo-tester.html

@@ -5,7 +5,7 @@
 * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
 -->

-<html>
+<html lang="en">


Just to be sure - What do you mean with "current behavior"? The current behavior before this PR or the current behavior of this PR?

The current behavior before this PR is what we want to fall back to if a lang is not specified.

cli/test/smokehouse/test-definitions/seo-mixed-language.js

adamraine · 2023-08-23T23:54:59Z

core/gather/gatherers/anchor-elements.js

+   * @param {string|null} currentLang
+   * @return {string}
+   */
+  function getLangOfInnerText(node, currentLang = null) {


This function adds a lot of complexity just so we can check the edge case where a child of the anchor defines a new language. We could get most of the benefit if we just check for node.closest('[lang]')?.getAttribute('lang') || '' and set the lang to unknown ('') if any child nodes define a lang. If the language is unknown we just check every language like Connor mentioned.

I think the slightly worse experience for those edge cases is preferable to introducing all of this complexity.

adamraine · 2023-08-23T23:55:19Z

core/audits/seo/link-text.js

+              nonDescriptiveLinkTexts[lang].has(searchTerm)) {
+              return true;
+            }
+            lang = lang.substring(0, lang.lastIndexOf('-'));


Future-proofing sounds fine 👍

…o localize-link-text-audit

raffaelj · 2023-08-29T15:55:13Z

Sorry for the delay, and thanks for being patient

Now I have to add another delay. I see 10 changed files from 3-6 months ago and I have to read them all again before I can answer. I'm not sure, if I find some time during the next weeks. If someone else wants to take over to speed things up - just go ahead.

raffaelj added 9 commits March 4, 2023 18:33

started to implement localiziation for link-text audit

6c67816

also added German strings to block list

update link-text tests for localization

1810a8b

Merge branch 'main' into localize-link-text-audit

fdf520b

reverted xlink:href check (deprecated)

668bab4

dropped checks for xml:lang (deprecated)

08aa8fa

cleanup

d47b871

Merge branch 'main' into localize-link-text-audit

f7029e0

refactored link-text lang detection (edge cases)

60547bd

improved language code check in link-text audit

fad9353

raffaelj requested a review from a team as a code owner March 24, 2023 18:49

raffaelj requested review from connorjclark and removed request for a team March 24, 2023 18:49

vercel bot deployed to Preview March 24, 2023 18:50 View deployment

raffaelj added 3 commits March 25, 2023 14:03

skip if link text is empty

4dfcea5

prevent more than one dom traversal if page has no lang

11957f2

pass some type checks

0584681

vercel bot deployed to Preview March 25, 2023 13:11 View deployment

raffaelj added 2 commits March 25, 2023 16:06

cleanup

74b677a

pass missing type checks

54248cc

vercel bot deployed to Preview March 25, 2023 15:08 View deployment

raffaelj added 3 commits March 26, 2023 12:36

improved code readability

7552a22

no functional change, but less if/else soup and more descriptive function name

avoid unnecessary array copy by calling .filter() twice

5469f11

prevent unnecessary function call

748fd29

if opening html tag is the only place with a lang attribute

vercel bot deployed to Preview March 26, 2023 13:01 View deployment

devtools-bot assigned connorjclark Mar 27, 2023

devtools-bot added the waiting4reviewer label Mar 27, 2023

connorjclark reviewed May 3, 2023

View reviewed changes

vercel bot deployed to Preview May 8, 2023 09:46 View deployment

This comment was marked as spam.

Sign in to view

connorjclark reviewed May 8, 2023

View reviewed changes

raffaelj added 8 commits May 9, 2023 13:46

reverted unneded textLang attr

d027a22

fixed wrong lang/textLang attr

ee417cf

added test cases for non-English and unknown languages

1be0dbc

also return textLang attribute from link-text audit

01bb153

more explicit smoke test (seo-failing) with languages

b8dc49f

cleanup

6dd6d05

added smoke test for mixed languages (link-text audit)

bc1347b

added string to German blocklist (link-text audit)

c3a3796

vercel bot deployed to Preview May 9, 2023 12:43 View deployment

improved language detection for single lang attr (html or body)

e5590aa

vercel bot deployed to Preview May 10, 2023 11:32 View deployment

raffaelj changed the title ~~Localize link text audit~~ core(link-text): localize link text audit May 11, 2023

This comment was marked as spam.

Sign in to view

raffaelj added 2 commits May 12, 2023 16:33

prevent calling getLangOfInnerText if document has no lang

b21e10f

added invalid links with lang-region attributes to mocha test

e479dac

vercel bot deployed to Preview May 12, 2023 14:44 View deployment

Merge branch 'main' into localize-link-text-audit

c9af94d

vercel bot deployed to Preview August 19, 2023 13:17 View deployment

adamraine reviewed Aug 24, 2023

View reviewed changes

Merge branch 'main' of https://github.com/GoogleChrome/lighthouse int…

86f0e50

…o localize-link-text-audit

vercel bot deployed to Preview August 29, 2023 15:40 View deployment

fixed wrong copyright year

8558030

vercel bot deployed to Preview August 29, 2023 15:44 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(link-text): localize link text audit #14927

core(link-text): localize link text audit #14927

raffaelj commented Mar 24, 2023 •

edited

google-cla bot commented Mar 24, 2023

connorjclark May 3, 2023 •

edited

raffaelj May 10, 2023

connorjclark May 3, 2023

raffaelj May 11, 2023

connorjclark May 3, 2023

connorjclark May 8, 2023

raffaelj May 10, 2023

adamraine Aug 23, 2023

raffaelj commented May 8, 2023

This comment was marked as spam.

connorjclark May 8, 2023

raffaelj May 10, 2023

raffaelj commented May 11, 2023

This comment was marked as spam.

raffaelj commented Jun 6, 2023

adamraine left a comment

adamraine Aug 23, 2023

adamraine Aug 23, 2023

adamraine Aug 23, 2023

raffaelj commented Aug 29, 2023

core(link-text): localize link text audit #14927

Are you sure you want to change the base?

core(link-text): localize link text audit #14927

Conversation

raffaelj commented Mar 24, 2023 • edited

google-cla bot commented Mar 24, 2023

connorjclark May 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raffaelj commented May 8, 2023

This comment was marked as spam.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raffaelj commented May 11, 2023

This comment was marked as spam.

raffaelj commented Jun 6, 2023

adamraine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raffaelj commented Aug 29, 2023

raffaelj commented Mar 24, 2023 •

edited

connorjclark May 3, 2023 •

edited