Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Commit

Permalink
Implement spell check
Browse files Browse the repository at this point in the history
This implements highlighting of misspelled words.  It also implements a context menu when right clicking on a misspelled word for up to the top 3 suggestions. You can also learn the word, in which case it will be suggested in future similar lookups, or ignore it so it won't be included in future lookups but will not be underlined

Fix #859

Auditors: @diracdeltas
  • Loading branch information
bbondy committed May 8, 2016
1 parent a2731ea commit 9343742
Show file tree
Hide file tree
Showing 21 changed files with 276 additions and 43 deletions.
30 changes: 27 additions & 3 deletions app/extensions/brave/brave-default.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,12 @@ if (typeof KeyEvent === 'undefined') {
xhttp.send()
}

ipcRenderer.on('init-spell-check', function (e, lang) {
chrome.webFrame.setSpellCheckProvider(lang, true, {

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas May 11, 2016

Member

not a blocker, but this commit causes Flow errors in brave-default.js

spellCheck: (word) => !ipcRenderer.sendSync('is-misspelled', word)
})
})

// Fires when the browser has ad replacement information to give
ipcRenderer.on('set-ad-div-candidates', function (e, adDivCandidates, placeholderUrl) {
// Keep a lookup for skipped common elements
Expand Down Expand Up @@ -538,8 +544,8 @@ if (typeof KeyEvent === 'undefined') {
return passwordNodes
}

function hasSelection (node) {
return window.getSelection().toString().length > 0
function getSelection () {
return window.getSelection().toString()
}

/**
Expand All @@ -563,6 +569,10 @@ if (typeof KeyEvent === 'undefined') {
return window.navigator.platform.includes('Mac')
}

function hasWhitespace (text) {
return /\s/g.test(text);
}

document.addEventListener('contextmenu', (e/*: Event*/) => {
window.setTimeout(() => {
if (!(e instanceof MouseEvent)) {
Expand Down Expand Up @@ -593,12 +603,26 @@ if (typeof KeyEvent === 'undefined') {
maybeLink = maybeLink.parentNode
}

const selection = getSelection()
let suggestions = []
let isMisspelled = false
if (selection.length > 0 && !hasWhitespace(selection)) {
// This is not very taxing, it only happens once on right click and only
// if it is on one word, and the check and result set are returned very fast.
const info = ipcRenderer.sendSync('get-misspelling-info', selection)
suggestions = info.suggestions
isMisspelled = info.isMisspelled
}

var nodeProps = {
name: name,
href: href,
isContentEditable: e.target.isContentEditable || false,
src: e.target.getAttribute ? e.target.getAttribute('src') : undefined,
hasSelection: hasSelection(e.target),
selection,
suggestions,
isMisspelled,
hasSelection: selection.length > 0,
offsetX: e.pageX,
offsetY: e.pageY
}
Expand Down
2 changes: 2 additions & 0 deletions app/extensions/brave/locales/en-US/menu.properties
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,5 @@ inspectElement=Inspect Element
downloadsManager=Downloads Manager...
zoom=Zoom
new=New
learnSpelling=Learn Spelling
ignoreSpelling=Ignore Spelling
2 changes: 2 additions & 0 deletions app/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const CryptoUtil = require('../js/lib/cryptoUtil')
const keytar = require('keytar')
const settings = require('../js/constants/settings')
const siteSettings = require('../js/state/siteSettings')
const spellCheck = require('./spellCheck')

// Used to collect the per window state when shutting down the application
let perWindowState = []
Expand Down Expand Up @@ -353,6 +354,7 @@ app.on('ready', () => {
AdBlock.init()
SiteHacks.init()
NoScript.init()
spellCheck.init()

ipcMain.on(messages.UPDATE_REQUESTED, () => {
Updater.updateNowRequested()
Expand Down
2 changes: 2 additions & 0 deletions app/locale.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ var rendererIdentifiers = function () {
'blockPopups',
'noScript',
'httpsEverywhere',
'learnSpelling',
'ignoreSpelling',
// Other identifiers
'urlCopied'
]
Expand Down
6 changes: 5 additions & 1 deletion app/sessionStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,10 @@ module.exports.defaultAppState = () => {
siteSettings: {},
passwords: [],
notifications: [],
temporarySiteSettings: {}
temporarySiteSettings: {},
dictionary: {
addedWords: [],
ignoredWords: []
}
}
}
62 changes: 62 additions & 0 deletions app/spellCheck.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// @flow
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

'use strict'

const spellchecker = require('spellchecker')
const messages = require('../js/constants/messages')
const electron = require('electron')
const ipcMain = electron.ipcMain
const app = electron.app
const appStore = require('../js/stores/appStore')
const appActions = require('../js/actions/appActions')

// Stores a reference to the last added immutable words
let lastAddedWords

const isMisspelled = (word) =>
!appStore.getState().getIn(['dictionary', 'ignoredWords']).includes(word) &&
!appStore.getState().getIn(['dictionary', 'addedWords']).includes(word) &&
spellchecker.isMisspelled(word)

module.exports.init = () => {
ipcMain.on(messages.IS_MISSPELLED, (e, word) => {
e.returnValue = isMisspelled(word)
})
ipcMain.on(messages.GET_MISSPELLING_INFO, (e, word) => {
const misspelled = isMisspelled(word)
e.returnValue = {
isMisspelled: misspelled,
suggestions: !misspelled ? [] : spellchecker.getCorrectionsForMisspelling(word)
}
})

appStore.addChangeListener(() => {
let addedWords = appStore.getState().getIn(['dictionary', 'addedWords'])
if (lastAddedWords !== addedWords) {
if (lastAddedWords) {
addedWords = addedWords.splice(lastAddedWords.size)
}
addedWords.forEach(spellchecker.add.bind(spellchecker))

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas May 11, 2016

Member

documentation at https://github.com/atom/node-spellchecker says When using Hunspell, this will not modify the .dic file; new words must be added each time the spellchecker is created. Does that mean that adding words won't work on windows <8 and linux?

This comment has been minimized.

Copy link
@bbondy

bbondy May 11, 2016

Author Member

We add the words each time when the app is restarted.

lastAddedWords = appStore.getState().getIn(['dictionary', 'addedWords'])
}
})

const availableDictionaries = spellchecker.getAvailableDictionaries()
let dict = app.getLocale().replace('-', '_')
let dictionaryLocale
if (availableDictionaries.includes(dict)) {
dictionaryLocale = dict
} else {
dict = app.getLocale().split('-')[0]
if (availableDictionaries.includes(dict)) {
dictionaryLocale = dict
}
}

if (dictionaryLocale) {
appActions.setDictionary(dictionaryLocale)
}
}
22 changes: 22 additions & 0 deletions docs/appActions.md
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,28 @@ Hides a message box in the notification bar



### addWord(word, learn)

Adds a word to the dictionary

**Parameters**

**word**: `string`, The word to add

**learn**: `boolean`, true if the word should be learned, false if ignored



### setDictionary(locale)

Adds a word to the dictionary

**Parameters**

**locale**: `string`, The locale to set for the dictionary




* * *

Expand Down
7 changes: 6 additions & 1 deletion docs/state.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,12 @@ AppStore
'privacy.block-canvas-fingerprinting': boolean, // Canvas fingerprinting defense
'security.passwords.manager-enabled': boolean, // whether to use default password manager
'general.downloads.defaultSavePath': string, // The default path to store files, this will be updated on each save until another pref is added to control that.
}]
}],
dictionary: {
locale: string, // en_US, en, or any other locale string
ignoredWords: Array<string>, // List of words to ignore
addedWords: Array<string> // List of words to add to the dictionary
}
}
```

Expand Down
12 changes: 0 additions & 12 deletions docs/windowActions.md
Original file line number Diff line number Diff line change
Expand Up @@ -617,18 +617,6 @@ Sets whether the noscript icon is visible.



### inspectElement(x, y)

Inspect the element for the active webview at the x, y content position

**Parameters**

**x**: `number`, horizontal position of the element to inspect

**y**: `number`, vertical position of the element to inspect




* * *

Expand Down
24 changes: 24 additions & 0 deletions js/actions/appActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,30 @@ const appActions = {
actionType: AppConstants.APP_HIDE_MESSAGE_BOX,
message
})
},

/**
* Adds a word to the dictionary
* @param {string} word - The word to add
* @param {boolean} learn - true if the word should be learned, false if ignored
*/
addWord: function (word, learn) {
AppDispatcher.dispatch({
actionType: AppConstants.APP_ADD_WORD,
word,
learn
})
},

/**
* Adds a word to the dictionary
* @param {string} locale - The locale to set for the dictionary
*/
setDictionary: function (locale) {
AppDispatcher.dispatch({
actionType: AppConstants.APP_SET_DICTIONARY,
locale
})
}
}

Expand Down
45 changes: 45 additions & 0 deletions js/actions/webviewActions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

'use strict'

const getWebview = () =>
document.querySelector('.frameWrapper.isActive webview')

const webviewActions = {
/**
* Puts the webview in focus
*/
setWebviewFocused: function () {
const webview = getWebview()
if (webview) {
webview.focus()
}
},

/**
* Inspect the element for the active webview at the x, y content position
* @param {number} x - horizontal position of the element to inspect
* @param {number} y - vertical position of the element to inspect
*/
inspectElement: function (x, y) {
const webview = getWebview()
if (webview) {
webview.inspectElement(x, y)
}
},

/**
* Repalces the selected text in an editable
* @param {string} text - The text to replace with
*/
replaceMisspelling: function (text) {
const webview = getWebview()
if (webview) {
webview.replaceMisspelling(text)

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas May 11, 2016

Member

somehow webview.replaceMisspelling does nothing for me; webview.replace works as expected here though.

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas May 11, 2016

Member

actually i think what's happening is that i'm trying to replace misspellings on fields with spellcheck=false. confusingly, the replacement spellings appear in the context menu when i highlight those words, but clicking on them does nothing because webview.replaceMisspelling does not work on spellcheck=false fields. i think we should either (1) not show the replacement spellings when spellcheck is false or (2) use replace instead of replaceMisspelling.

This comment has been minimized.

Copy link
@bbondy

bbondy May 11, 2016

Author Member

I'll just use replace

}
}
}

module.exports = webviewActions
19 changes: 0 additions & 19 deletions js/actions/windowActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -279,13 +279,6 @@ const windowActions = {
})
},

setWebviewFocused: function () {
const webview = document.querySelector('.frameWrapper.isActive webview')
if (webview) {
webview.focus()
}
},

/**
* Dispatches a message to the store to create a new frame
*
Expand Down Expand Up @@ -863,18 +856,6 @@ const windowActions = {
actionType: WindowConstants.WINDOW_SET_NOSCRIPT_VISIBLE,
isVisible
})
},

/**
* Inspect the element for the active webview at the x, y content position
* @param {number} x - horizontal position of the element to inspect
* @param {number} y - vertical position of the element to inspect
*/
inspectElement: function (x, y) {
const webview = document.querySelector('.frameWrapper.isActive webview')
if (webview) {
webview.inspectElement(x, y)
}
}
}

Expand Down
7 changes: 7 additions & 0 deletions js/components/frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,9 @@ class Frame extends ImmutableComponent {
if (this.props.enableAds) {
this.insertAds(this.webview.getURL())
}
if (this.props.dictionaryLocale) {
this.initSpellCheck()
}
this.webview.send(messages.POST_PAGE_LOAD_RUN)
if (getSetting(settings.PASSWORD_MANAGER_ENABLED)) {
this.webview.send(messages.AUTOFILL_PASSWORD)
Expand Down Expand Up @@ -487,6 +490,10 @@ class Frame extends ImmutableComponent {
this.webview.send(messages.SET_AD_DIV_CANDIDATES, adDivCandidates, config.vault.replacementUrl)
}

initSpellCheck () {
this.webview.send(messages.INIT_SPELL_CHECK, this.props.dictionaryLocale)
}

goBack () {
this.webview.goBack()
}
Expand Down
4 changes: 3 additions & 1 deletion js/components/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const remote = electron.remote

// Actions
const windowActions = require('../actions/windowActions')
const webviewActions = require('../actions/webviewActions')
const loadOpenSearch = require('../lib/openSearch').loadOpenSearch
const contextMenus = require('../contextMenus')
const getSetting = require('../settings').getSetting
Expand Down Expand Up @@ -260,7 +261,7 @@ class Main extends ImmutableComponent {
window.addEventListener('focus', () => {
// For whatever reason other elements are preserved but webviews are not.
if (document.activeElement && document.activeElement.tagName === 'BODY') {
windowActions.setWebviewFocused()
webviewActions.setWebviewFocused()
}
})
const activeFrame = FrameStateUtil.getActiveFrame(self.props.windowState)
Expand Down Expand Up @@ -580,6 +581,7 @@ class Main extends ImmutableComponent {
.filter((site) => site.get('tags')
.includes(siteTags.BOOKMARK_FOLDER)) || new Immutable.Map()
: null}
dictionaryLocale={this.props.appState.getIn(['dictionary', 'locale'])}
passwords={this.props.appState.get('passwords')}
siteSettings={this.props.appState.get('siteSettings')}
temporarySiteSettings={this.props.appState.get('temporarySiteSettings')}
Expand Down
4 changes: 3 additions & 1 deletion js/constants/appConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ const AppConstants = {
APP_CHANGE_SETTING: _,
APP_CHANGE_SITE_SETTING: _,
APP_SHOW_MESSAGE_BOX: _, /** @param {Object} detail */
APP_HIDE_MESSAGE_BOX: _ /** @param {string} message */
APP_HIDE_MESSAGE_BOX: _, /** @param {string} message */
APP_ADD_WORD: _, /** @param {string} word, @param {boolean} learn */
APP_SET_DICTIONARY: _ /** @param {string} locale */
}

module.exports = mapValuesByKeys(AppConstants)
Loading

1 comment on commit 9343742

@diracdeltas
Copy link
Member

Choose a reason for hiding this comment

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

lgtm other than comments above

Please sign in to comment.