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: do not allow the window to grab focus when tabbing / shift+tabbing #16042

Merged
merged 2 commits into from Dec 19, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions atom/browser/common_web_contents_delegate.cc
Expand Up @@ -346,6 +346,19 @@ blink::WebSecurityStyle CommonWebContentsDelegate::GetSecurityStyle(
security_style_explanations);
}

bool CommonWebContentsDelegate::TakeFocus(content::WebContents* source,
bool reverse) {
if (source && source->GetOutermostWebContents() == source) {
// If this is the outermost web contents and the user has tabbed or
// shift + tabbed through all the elements, reset the focus back to
// the first or last element so that it doesn't stay in the body.
source->FocusThroughTabTraversal(reverse);
return true;
}

return false;
}

void CommonWebContentsDelegate::DevToolsSaveToFile(const std::string& url,
const std::string& content,
bool save_as) {
Expand Down
1 change: 1 addition & 0 deletions atom/browser/common_web_contents_delegate.h
Expand Up @@ -98,6 +98,7 @@ class CommonWebContentsDelegate : public content::WebContentsDelegate,
blink::WebSecurityStyle GetSecurityStyle(
content::WebContents* web_contents,
content::SecurityStyleExplanations* explanations) override;
bool TakeFocus(content::WebContents* source, bool reverse) override;
void HandleKeyboardEvent(
content::WebContents* source,
const content::NativeWebKeyboardEvent& event) override;
Expand Down
105 changes: 105 additions & 0 deletions spec/chromium-spec.js
Expand Up @@ -1411,6 +1411,111 @@ describe('chromium feature', () => {
await new Promise((resolve) => { utter.onend = resolve })
})
})

describe('focus handling', () => {
ppontes marked this conversation as resolved.
Show resolved Hide resolved
let webviewContents = null

beforeEach(async () => {
w = new BrowserWindow({
show: true
})

const webviewReady = emittedOnce(w.webContents, 'did-attach-webview')
await w.loadFile(path.join(fixtures, 'pages', 'tab-focus-loop-elements.html'))
const [, wvContents] = await webviewReady
webviewContents = wvContents
await emittedOnce(webviewContents, 'did-finish-load')
w.focus()
})

afterEach(() => {
webviewContents = null
})

const expectFocusChange = async () => {
const [, focusedElementId] = await emittedOnce(ipcMain, 'focus-changed')
return focusedElementId
}

describe('a TAB press', () => {
const tabPressEvent = {
type: 'keyDown',
keyCode: 'Tab'
}

it('moves focus to the next focusable item', async () => {
let focusChange = expectFocusChange()
w.webContents.sendInputEvent(tabPressEvent)
let focusedElementId = await focusChange
assert.strictEqual(focusedElementId, 'BUTTON-element-1', `should start focused in element-1, it's instead in ${focusedElementId}`)

focusChange = expectFocusChange()
w.webContents.sendInputEvent(tabPressEvent)
focusedElementId = await focusChange
assert.strictEqual(focusedElementId, 'BUTTON-element-2', `focus should've moved to element-2, it's instead in ${focusedElementId}`)

focusChange = expectFocusChange()
w.webContents.sendInputEvent(tabPressEvent)
focusedElementId = await focusChange
assert.strictEqual(focusedElementId, 'BUTTON-wv-element-1', `focus should've moved to the webview's element-1, it's instead in ${focusedElementId}`)

focusChange = expectFocusChange()
webviewContents.sendInputEvent(tabPressEvent)
focusedElementId = await focusChange
assert.strictEqual(focusedElementId, 'BUTTON-wv-element-2', `focus should've moved to the webview's element-2, it's instead in ${focusedElementId}`)

focusChange = expectFocusChange()
webviewContents.sendInputEvent(tabPressEvent)
focusedElementId = await focusChange
assert.strictEqual(focusedElementId, 'BUTTON-element-3', `focus should've moved to element-3, it's instead in ${focusedElementId}`)

focusChange = expectFocusChange()
w.webContents.sendInputEvent(tabPressEvent)
focusedElementId = await focusChange
assert.strictEqual(focusedElementId, 'BUTTON-element-1', `focus should've looped back to element-1, it's instead in ${focusedElementId}`)
})
})

describe('a SHIFT + TAB press', () => {
const shiftTabPressEvent = {
type: 'keyDown',
modifiers: ['Shift'],
keyCode: 'Tab'
}

it('moves focus to the previous focusable item', async () => {
let focusChange = expectFocusChange()
w.webContents.sendInputEvent(shiftTabPressEvent)
let focusedElementId = await focusChange
assert.strictEqual(focusedElementId, 'BUTTON-element-3', `should start focused in element-3, it's instead in ${focusedElementId}`)

focusChange = expectFocusChange()
w.webContents.sendInputEvent(shiftTabPressEvent)
focusedElementId = await focusChange
assert.strictEqual(focusedElementId, 'BUTTON-wv-element-2', `focus should've moved to the webview's element-2, it's instead in ${focusedElementId}`)

focusChange = expectFocusChange()
webviewContents.sendInputEvent(shiftTabPressEvent)
focusedElementId = await focusChange
assert.strictEqual(focusedElementId, 'BUTTON-wv-element-1', `focus should've moved to the webview's element-1, it's instead in ${focusedElementId}`)

focusChange = expectFocusChange()
webviewContents.sendInputEvent(shiftTabPressEvent)
focusedElementId = await focusChange
assert.strictEqual(focusedElementId, 'BUTTON-element-2', `focus should've moved to element-2, it's instead in ${focusedElementId}`)

focusChange = expectFocusChange()
w.webContents.sendInputEvent(shiftTabPressEvent)
focusedElementId = await focusChange
assert.strictEqual(focusedElementId, 'BUTTON-element-1', `focus should've moved to element-1, it's instead in ${focusedElementId}`)

focusChange = expectFocusChange()
w.webContents.sendInputEvent(shiftTabPressEvent)
focusedElementId = await focusChange
assert.strictEqual(focusedElementId, 'BUTTON-element-3', `focus should've looped back to element-3, it's instead in ${focusedElementId}`)
})
})
})
})

describe('font fallback', () => {
Expand Down
25 changes: 25 additions & 0 deletions spec/fixtures/pages/tab-focus-loop-elements-wv.html
@@ -0,0 +1,25 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title></title>
<body>
<div>
<button id="wv-element-1">Button 1</button>
<button id="wv-element-2">Button 2</button>
</div>
<script>
const { ipcRenderer } = require('electron')

function handleFocusChange(event) {
if (event.target.tagName) {
const elementId = event.target.id ? `-${event.target.id}` : ''
const elementIdentifier = `${event.target.tagName}${elementId}`
ipcRenderer.send('focus-changed', elementIdentifier)
}
}

addEventListener('focus', handleFocusChange, true)
</script>
</body>
</html>
27 changes: 27 additions & 0 deletions spec/fixtures/pages/tab-focus-loop-elements.html
@@ -0,0 +1,27 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title></title>
<script>
const { ipcRenderer } = require('electron')

function handleFocusChange(event) {
if (event.target.tagName && event.target.tagName !== 'WEBVIEW') {
const elementId = event.target.id ? `-${event.target.id}` : ''
const elementIdentifier = `${event.target.tagName}${elementId}`
ipcRenderer.send('focus-changed', elementIdentifier)
}
}

addEventListener('focus', handleFocusChange, true)
</script>
<body>
<div>
<button id="element-1">Button 1</button>
<button id="element-2">Button 2</button>
<webview src="tab-focus-loop-elements-wv.html" nodeintegration></webview>
<button id="element-3">Button 3</button>
</div>
</body>
</html>