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

fix(interactivity-checker): improve robustness of isTabbable #1950

Merged
merged 5 commits into from
Nov 30, 2016

Conversation

devversion
Copy link
Member

A couple of notes:

  • It's intentional to not nest things like that, because such functions need to be clear.

  • You may have noticed that there are no specs for the Platform. It's just hard to mock the userAgent and stuff like v8BreakIterator without polluting the other tests.

References #1625

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 21, 2016
@devversion devversion added in progress This issue is currently in progress and removed pr: needs review labels Nov 21, 2016
@devversion devversion force-pushed the fix/focus-trap-robustness branch 3 times, most recently from bb310a3 to b575211 Compare November 22, 2016 18:56
@devversion devversion added pr: needs review and removed in progress This issue is currently in progress labels Nov 22, 2016
@devversion
Copy link
Member Author

@jelbourn This should be ready for review now.

@jelbourn
Copy link
Member

@devversion can you rebase?

@devversion
Copy link
Member Author

@jelbourn Done.

@@ -16,6 +17,8 @@ import {Injectable} from '@angular/core';
@Injectable()
export class InteractivityChecker {

constructor(private platform: MdPlatform) {}
Copy link
Member

Choose a reason for hiding this comment

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

_platform

return getComputedStyle(element).getPropertyValue('visibility') == 'visible';
return getComputedStyle(element).getPropertyValue('visibility') === 'visible';

function checkRectangles() {
Copy link
Member

Choose a reason for hiding this comment

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

This should be a module method with a description, similar to the others at the bottom of this file. I'd also call it something like hasGeometry or elementHasRenderedRect

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I really like hasGeometry

// See https://github.com/jquery/jquery/blob/master/src/css/hiddenVisibleSelectors.js#L12
if (!(element.offsetWidth || element.offsetHeight || element.getClientRects().length)) {
// In IE11 audio elements with controls have invalid rectangles, but are still visible.
if (!isControlAudio && !checkRectangles()) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that an audio element with display: none or visibility: hidden will still be marked as visible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't think that's correct. I re-confirmed within IE11 and Edge and this check seems to be unnecessary because we always want to check the rectangles / geometry - Good catch.

Copy link
Contributor

Choose a reason for hiding this comment

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

an <audio> with visibility: hidden should be fine, but I think it does miss the display: none case, maybe special-case TRIDENT/EDGE since other browsers have the correct behavior anyways

@@ -16,6 +17,8 @@ import {Injectable} from '@angular/core';
@Injectable()
export class InteractivityChecker {

constructor(private platform: MdPlatform) {}
Copy link
Member

Choose a reason for hiding this comment

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

Can you also update the class comment to just say

/**
 * Utility for checking the interactivity of an element, such as whether is
 * focusable or tabbable.
 */

And then add another // comment above it that explains how we drew much of this logic from ally.js, omitting checks for platforms and edge-cases we don't support?

@@ -122,3 +214,7 @@ function isPotentiallyFocusable(element: HTMLElement): boolean {
hasValidTabIndex(element);
}


function getWindow(node: HTMLElement): Window {
Copy link
Member

Choose a reason for hiding this comment

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

Needs function description


let nodeName = element.nodeName.toLowerCase();
let frameElement = getWindow(element).frameElement as HTMLElement;
let frameType = frameElement && frameElement.nodeName.toLowerCase();
Copy link
Member

Choose a reason for hiding this comment

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

Declare frameType, nodeName, and tabIndexValue closer to where they're first used

@@ -106,6 +177,27 @@ function hasValidTabIndex(element: HTMLElement): boolean {
return !!(tabIndex && !isNaN(parseInt(tabIndex, 10)));
}

function getTabIndexValue(element: HTMLElement) {
Copy link
Member

Choose a reason for hiding this comment

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

Needs function description that explains what this does beyond reading the tabIndex property directly.

/** Appends elements to the testContainerElement. */
function appendElements(elements: Element[]) {
for (let e of elements) {
testContainerElement.appendChild(e);
}
}

function runIf(condition: boolean, runFn: Function): () => void {
return function() {
Copy link
Member

Choose a reason for hiding this comment

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

return () => {

?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to use a normal function because we want to retrieve the arguments below.

Copy link
Contributor

Choose a reason for hiding this comment

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

you can do (...args) => {

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I will change it - But anyways what's the advantage of an arrow function here? We don't need any access to the this context here

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's really much advantage to one way or the other, feel free to leave it as is unless Jeremy feels strongly

@jelbourn jelbourn changed the title fix(focus-trap): improve robustness fix(interactivity-checked): improve robustness of isTabbable Nov 28, 2016
@jelbourn jelbourn changed the title fix(interactivity-checked): improve robustness of isTabbable fix(interactivity-checker): improve robustness of isTabbable Nov 28, 2016
@jelbourn
Copy link
Member

@mmalerba could you also take a look at this PR? I've spent so much time looking at the tabbable stuff I think a fresh perspective would be good

@mmalerba mmalerba self-assigned this Nov 29, 2016
@@ -0,0 +1,41 @@
import {Injectable, NgModule, ModuleWithProviders} from '@angular/core';

// Declare window with type of any.
Copy link
Contributor

Choose a reason for hiding this comment

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

comment doesn't tell us anything the code doesn't


/** Browsers and Platform Types */
IOS = /iPad|iPhone|iPod/.test(navigator.userAgent) && !window.MSStream;
FIREFOX = /(firefox|minefield)/i.test(navigator.userAgent);
Copy link
Contributor

Choose a reason for hiding this comment

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

why FIREFOX and not a GECKO check above instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's because it would be difficult to detect the plain Gecko layout engine, because actually every browser identifies it self as a Gecko-like browser.

Since we only need to check for Firefox explicit, we can just implement the firefox check instead of the unstable gecko check (http://webaim.org/blog/user-agent-string-history/)

Copy link
Member

Choose a reason for hiding this comment

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

Can you add that explanation as a comment?

const hasV8BreakIterator = (window.Intl && (window.Intl as any).v8BreakIterator);

@Injectable()
export class MdPlatform {
Copy link
Contributor

Choose a reason for hiding this comment

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

document where you're getting these checks from

<p>Is Webkit: {{ platform.WEBKIT }}</p>
<p>Is Trident: {{ platform.TRIDENT }}</p>
<p>Is Edge: {{ platform.EDGE }}</p>

Copy link
Contributor

Choose a reason for hiding this comment

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

extra blank line

})
export class PlatformDemo {

constructor(public platform: MdPlatform) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra blank lines

Copy link
Member Author

@devversion devversion Nov 29, 2016

Choose a reason for hiding this comment

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

I don't think that's a big deal - Is there any specific convention in the Google JavaScript conventions? - Going to change it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's any official convention for it, I just like to keep it consistent throughout the project. Most other files I've looked at seem to not leave space here

// Do not run for Blink, Firefox and iOS because those treat video elements
// with controls different and are covered in other tests.
runIf(!platform.BLINK && !platform.FIREFOX && !platform.IOS, () => {

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need spaces between each line

.toBe(false, `Expected <${el.nodeName} tabindex="-1"> not to be tabbable`);
elements.forEach(el => {
expect(checker.isFocusable(el))
.toBe(true, `Expected <${el.nodeName} tabindex="0"> to be focusable`);
Copy link
Contributor

Choose a reason for hiding this comment

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

indent +4 for continuation (here and elsewhere in this file)


// Wait one tick, because the browser takes some time to update the tabIndex
// according to the contentEditable attribute.
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really necessary? your code checks if the contenteditable attribute is set, shouldn't that work without waiting for the browser to update the tabindex?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it seems like the browser needs some time to validate the new tabindex.

Here is a snippet to test inside of Chrome 54

let div = document.createElement('div'); document.body.appendChild(div); div.contentEditable = true; console.log(div.tabIndex);

// After that you will see `tabindex = -1` and if you now call
console.log(div.tabIndex) // = 0

Copy link
Contributor

Choose a reason for hiding this comment

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

interesting, can we just change the timeout to 0 then, bumping to the end of the queue with no delay should be enough

Copy link
Member Author

Choose a reason for hiding this comment

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

@mmalerba Makes sense. Changed it.

/** Appends elements to the testContainerElement. */
function appendElements(elements: Element[]) {
for (let e of elements) {
testContainerElement.appendChild(e);
}
}

function runIf(condition: boolean, runFn: Function): () => void {
return function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can do (...args) => {

@@ -16,6 +17,8 @@ import {Injectable} from '@angular/core';
@Injectable()
export class InteractivityChecker {

constructor(private platform: MdPlatform) {}

/** Gets whether an element is disabled. */
isDisabled(element: HTMLElement) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can probably improve this too:

return element.hasAttribute('disabled') || element.form && element.form.hasAttribute('disabled');

Copy link
Member Author

Choose a reason for hiding this comment

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

Would rather address things like that in another PR. We should keep that one as small as possible.

if (frameElement) {

// Frame elements inherit their tabindex onto all child elements.
if (getTabIndexValue(frameElement) === -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

so if an element inherits its tabindex through the frame it's definitely disabled? (because you have a few places below where if the user sets tabindex=-1 that doesn't necessarily mean non-tabbable, e.g. blink audio)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah if the user explicitly set the tabindex attribute on the frame element then all child elements are not tabbable.

The checks below for the tabIndex only affect the element and not the frameElement

}

// In iOS the browser only considers some specific elements as tabbable.
if (this.platform.WEBKIT && this.platform.IOS && !isPotentiallyTabbable(element)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe call the method isPotentiallyTabbableOnIOS

@devversion
Copy link
Member Author

@jelbourn @mmalerba Addressed your feedback.

@mmalerba
Copy link
Contributor

LGTM

@jelbourn
Copy link
Member

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Nov 29, 2016
@tinayuangao tinayuangao merged commit 4b7e52d into angular:master Nov 30, 2016
@devversion devversion deleted the fix/focus-trap-robustness branch November 30, 2016 20:01
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants