♿ Update amp-carousel focus styles for better color contrast (#29564)

[dmanek]

• Used the recommended approach taken by Chrome by providing a dual-colour border/outline combination. Minor caveat: the outline for <button> is Chrome/Firefox is typically blue while the outline on carousel button is black/white.
• Fixes amp-carousel: Insufficient focus indication (outline color contrast) #29564

[dmanek]

/cc @caroqliu

[caroqliu]

Looks good! What do you think about adding a visual-diff test for this?

[caroqliu]

/invite @dmanek

[amp-invite-bot]

An invitation to join ampproject has been sent to @dmanek. I will update this thread when the invitation is accepted.

[amp-invite-bot]

The invitation to @dmanek was accepted!

[zhouyx]

LGTM.
You can add script to the visual diff test page, to mock the click event before capture the screenshot.
@caroqliu I don't think it's worth adding a standalone visual diff test just for this given visual diff test is relatively expensive. Is there an existing test that we can modify?

[dmanek]

I didn't find any existing test that includes amp-carousel (amp-lightbox-gallery.html includes amp-carousel-0.1.js but does not use amp-carousel).

[CLAassistant]

All committers have signed the CLA.

[caroqliu]

I don't think it's worth adding a standalone visual diff test just for this given visual diff test is relatively expensive. Is there an existing test that we can modify?

@zhouyx good point, in that case I'd recommend modifying an existing unit test to check the CSS property values, similar to how @krdwan did here:

amphtml/extensions/amp-accordion/1.0/test/test-amp-accordion.js

Lines 85 to 142 in 2990e68

	it('should have amp specific classes for CSS', () => {
	const sections = element.children;
	const {
	firstElementChild: header0,
	lastElementChild: content0,
	} = sections[0];
	const {
	firstElementChild: header1,
	lastElementChild: content1,
	} = sections[1];
	const {
	firstElementChild: header2,
	lastElementChild: content2,
	} = sections[2];
	// Check classes
	expect(header0.className).to.include('i-amphtml-accordion-header');
	expect(header1.className).to.include('i-amphtml-accordion-header');
	expect(header2.className).to.include('i-amphtml-accordion-header');
	expect(content0.className).to.include('i-amphtml-accordion-content');
	expect(content1.className).to.include('i-amphtml-accordion-content');
	expect(content2.className).to.include('i-amphtml-accordion-content');
	// Check computed styles
	expect(win.getComputedStyle(header0).margin).to.equal('0px');
	expect(win.getComputedStyle(header0).cursor).to.equal('pointer');
	expect(win.getComputedStyle(header0).backgroundColor).to.equal(
	'rgb(239, 239, 239)'
	);
	expect(win.getComputedStyle(header0).paddingRight).to.equal('20px');
	expect(win.getComputedStyle(header0).border).to.equal(
	'1px solid rgb(223, 223, 223)'
	);
	expect(win.getComputedStyle(header1).margin).to.equal('0px');
	expect(win.getComputedStyle(header1).cursor).to.equal('pointer');
	expect(win.getComputedStyle(header1).backgroundColor).to.equal(
	'rgb(239, 239, 239)'
	);
	expect(win.getComputedStyle(header1).paddingRight).to.equal('20px');
	expect(win.getComputedStyle(header1).border).to.equal(
	'1px solid rgb(223, 223, 223)'
	);
	expect(win.getComputedStyle(header2).margin).to.equal('0px');
	expect(win.getComputedStyle(header2).cursor).to.equal('pointer');
	expect(win.getComputedStyle(header2).backgroundColor).to.equal(
	'rgb(239, 239, 239)'
	);
	expect(win.getComputedStyle(header2).paddingRight).to.equal('20px');
	expect(win.getComputedStyle(header2).border).to.equal(
	'1px solid rgb(223, 223, 223)'
	);
	expect(win.getComputedStyle(content0).margin).to.equal('0px');
	expect(win.getComputedStyle(content1).margin).to.equal('0px');
	expect(win.getComputedStyle(content2).margin).to.equal('0px');
	});