From 94a32b9ddcec58439a05179d9b6aeec135b46fb2 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Sun, 15 Mar 2015 12:13:52 +0000 Subject: Fix frame-focus detection. In b05276ed8264e5a71f20a7068690ba2a414ee6d8, I inadvertently moved the focus handler from window to document. As a consequence, detectFocus (now registerFocus) wasn't being called when expected. --- content_scripts/vimium_frontend.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee index bc56e175..429657fc 100644 --- a/content_scripts/vimium_frontend.coffee +++ b/content_scripts/vimium_frontend.coffee @@ -209,7 +209,7 @@ window.initializeWhenEnabled = -> for type in [ "keydown", "keypress", "keyup", "click", "focus", "blur", "mousedown" ] do (type) -> installListener window, type, (event) -> handlerStack.bubbleEvent type, event installListener document, "DOMActivate", (event) -> handlerStack.bubbleEvent 'DOMActivate', event - installListener document, "focus", detectFocus + installListener window, "focus", registerFocus installedListeners = true FindModeHistory.init() @@ -229,7 +229,7 @@ getActiveState = -> # # The backend needs to know which frame has focus. # -detectFocus = -> +registerFocus = -> # settings may have changed since the frame last had focus settings.load() chrome.runtime.sendMessage({ handler: "frameFocused", frameId: frameId }) -- cgit v1.2.3 From 3876a4f06de77d190fdaecc6cf99eb57134d0372 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Sun, 15 Mar 2015 12:32:42 +0000 Subject: Populate popup with frame's URL. Previously, we have been populating the suggested exclusion URL in the page popup with the tab's URL. This uses the active frame's URL instead: - because this is the URL which will affect the current frame (without this, a user can naively add an exclusion rule and it has no effect), and - because, without this, the user has no reasonable way to add exclusion rules for frames such as the hangouts frame on gmail (for which, the URL is not displayed in the address bar). --- background_scripts/main.coffee | 3 +++ content_scripts/vimium_frontend.coffee | 5 +++-- pages/options.coffee | 8 ++++++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee index b390fe1c..ecf18bef 100644 --- a/background_scripts/main.coffee +++ b/background_scripts/main.coffee @@ -26,6 +26,7 @@ validFirstKeys = {} singleKeyCommands = [] focusedFrame = null frameIdsForTab = {} +root.urlForTab = {} # Keys are either literal characters, or "named" - for example (alt+b), (left arrow) or # This regular expression captures two groups: the first is a named key, the second is the remainder of @@ -462,6 +463,7 @@ chrome.tabs.onRemoved.addListener (tabId) -> tabInfoMap.deletor = -> delete tabInfoMap[tabId] setTimeout tabInfoMap.deletor, 1000 delete frameIdsForTab[tabId] + delete urlForTab[tabId] chrome.tabs.onActiveChanged.addListener (tabId, selectInfo) -> updateActiveState(tabId) @@ -647,6 +649,7 @@ unregisterFrame = (request, sender) -> handleFrameFocused = (request, sender) -> tabId = sender.tab.id + urlForTab[tabId] = request.url if frameIdsForTab[tabId]? frameIdsForTab[tabId] = [request.frameId, (frameIdsForTab[tabId].filter (id) -> id != request.frameId)...] diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee index 429657fc..757cc0ad 100644 --- a/content_scripts/vimium_frontend.coffee +++ b/content_scripts/vimium_frontend.coffee @@ -190,6 +190,7 @@ initializePreDomReady = -> # Ensure the sendResponse callback is freed. false + # Wrapper to install event listeners. Syntactic sugar. installListener = (element, event, callback) -> element.addEventListener(event, -> @@ -227,12 +228,12 @@ getActiveState = -> return { enabled: isEnabledForUrl, passKeys: passKeys } # -# The backend needs to know which frame has focus. +# The backend needs to know which frame has focus, and the active URL. # registerFocus = -> # settings may have changed since the frame last had focus settings.load() - chrome.runtime.sendMessage({ handler: "frameFocused", frameId: frameId }) + chrome.runtime.sendMessage handler: "frameFocused", frameId: frameId, url: window.location.toString() # # Initialization tasks that must wait for the document to be ready. diff --git a/pages/options.coffee b/pages/options.coffee index d2950348..61b055c2 100644 --- a/pages/options.coffee +++ b/pages/options.coffee @@ -271,8 +271,12 @@ initPopupPage = -> exclusions = null document.getElementById("optionsLink").setAttribute "href", chrome.runtime.getURL("pages/options.html") + # As the active URL, we choose the most recently registered URL from a frame in the tab, or the tab's own + # URL. + url = chrome.extension.getBackgroundPage().urlForTab[tab.id] || tab.url + updateState = -> - rule = bgExclusions.getRule tab.url, exclusions.readValueFromElement() + rule = bgExclusions.getRule url, exclusions.readValueFromElement() $("state").innerHTML = "Vimium will " + if rule and rule.passKeys "exclude #{rule.passKeys}" @@ -302,7 +306,7 @@ initPopupPage = -> window.close() # Populate options. Just one, here. - exclusions = new ExclusionRulesOnPopupOption(tab.url, "exclusionRules", onUpdated) + exclusions = new ExclusionRulesOnPopupOption(url, "exclusionRules", onUpdated) updateState() document.addEventListener "keyup", updateState -- cgit v1.2.3 From ea05de3f114dcc81a91bf863538f20754641cadd Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Sun, 15 Mar 2015 13:14:26 +0000 Subject: Propagate exclusion rules to all frames. When the active tab changes, we call updateActiveState to update the icon and propagate any changed exclusion-rule state to the tab. All frames received the request. However, only one response is received by the background page. Therefore, new exclusion rules are only propagated to one frame. Here's what can go wrong... On gmail, open the hangouts frame. Add an exclusion rule sepcific to the hangouts frame. Save it. The update is propagated only to the main frame. The new exclusion rule is not in effect in the hangouts frame. That's wierd and obviously wrong. In this commit, every frame receiving the getActiveState request also calls checkIfEnabledForUrl to ensure that any new exclusion-rule state is propagated. This is overkill, to some extent. We should really move settings to chrome.storage and have each frame check locally for changes affecting it. --- background_scripts/main.coffee | 3 ++- content_scripts/vimium_frontend.coffee | 13 +++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee index ecf18bef..a5d7ac31 100644 --- a/background_scripts/main.coffee +++ b/background_scripts/main.coffee @@ -398,7 +398,8 @@ root.updateActiveState = updateActiveState = (tabId) -> if response isCurrentlyEnabled = response.enabled currentPasskeys = response.passKeys - config = isEnabledForUrl { url: tab.url }, { tab: tab } + currentURL = response.url + config = isEnabledForUrl { url: currentURL }, { tab: tab } enabled = config.isEnabledForUrl passKeys = config.passKeys if (enabled and passKeys) diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee index 757cc0ad..3577e6f3 100644 --- a/content_scripts/vimium_frontend.coffee +++ b/content_scripts/vimium_frontend.coffee @@ -150,7 +150,7 @@ initializePreDomReady = -> settings.load() initializeModes() - checkIfEnabledForUrl() + checkIfEnabledForUrl true # true means checkIfEnabledForUrl is being called on start up. refreshCompletionKeys() # Send the key to the key handler in the background page. @@ -225,7 +225,12 @@ setState = (request) -> getActiveState = -> Mode.updateBadge() - return { enabled: isEnabledForUrl, passKeys: passKeys } + # getActiveState is called in each frame within the tab. However, only the response from the first frame is + # handled on the background page. Therefore, exclusion rule changes are not propagated to other frames. + # So, we force a state update for this frame, just in case. + # FIXME(smblott): This could be avoided if settings were propagated via chrome.storage. + checkIfEnabledForUrl() + return enabled: isEnabledForUrl, passKeys: passKeys, url: window.location.toString() # # The backend needs to know which frame has focus, and the active URL. @@ -557,7 +562,7 @@ onKeyup = (event) -> DomUtils.suppressPropagation(event) @stopBubblingAndTrue -checkIfEnabledForUrl = -> +checkIfEnabledForUrl = (onStartUp = false) -> url = window.location.toString() chrome.runtime.sendMessage { handler: "isEnabledForUrl", url: url }, (response) -> @@ -566,7 +571,7 @@ checkIfEnabledForUrl = -> isIncognitoMode = response.incognito if isEnabledForUrl initializeWhenEnabled() - else if (HUD.isReady()) + else if onStartUp and HUD.isReady() # Quickly hide any HUD we might already be showing, e.g. if we entered insert mode on page load. HUD.hide() handlerStack.bubbleEvent "registerStateChange", -- cgit v1.2.3 From fb24c770645ce1b8178729d4be612d58497ffa5f Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Sun, 15 Mar 2015 15:04:23 +0000 Subject: Rework page icon handling. Setting the page icon is now driven from the corrently-active frame. - Eliminates a race condition. - Icon matches actual frame state (not tab URL state). - Exclusion-rule changes propagate to all frames. --- background_scripts/main.coffee | 54 ++++++++++++---------------------- content_scripts/vimium_frontend.coffee | 48 +++++++++++++----------------- pages/options.coffee | 2 -- 3 files changed, 40 insertions(+), 64 deletions(-) diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee index a5d7ac31..a6e5a4eb 100644 --- a/background_scripts/main.coffee +++ b/background_scripts/main.coffee @@ -362,9 +362,6 @@ updateOpenTabs = (tab, deleteFrames = false) -> # Frames are recreated on refresh delete frameIdsForTab[tab.id] if deleteFrames -setBrowserActionIcon = (tabId,path) -> - chrome.browserAction.setIcon({ tabId: tabId, path: path }) - chrome.browserAction.setBadgeBackgroundColor # This is Vimium blue (from the icon). # color: [102, 176, 226, 255] @@ -384,35 +381,24 @@ setBadge = do -> # We wait a few moments. This avoids badge flicker when there are rapid changes. timer = setTimeout updateBadge(badge, sender.tab.id), 50 -# Updates the browserAction icon to indicate whether Vimium is enabled or disabled on the current page. -# Also propagates new enabled/disabled/passkeys state to active window, if necessary. -# This lets you disable Vimium on a page without needing to reload. -# Exported via root because it's called from the page popup. -root.updateActiveState = updateActiveState = (tabId) -> - enabledIcon = "icons/browser_action_enabled.png" - disabledIcon = "icons/browser_action_disabled.png" - partialIcon = "icons/browser_action_partial.png" - chrome.tabs.get tabId, (tab) -> - setBadge { badge: "" }, tab: { id: tabId } - chrome.tabs.sendMessage tabId, { name: "getActiveState" }, (response) -> - if response - isCurrentlyEnabled = response.enabled - currentPasskeys = response.passKeys - currentURL = response.url - config = isEnabledForUrl { url: currentURL }, { tab: tab } - enabled = config.isEnabledForUrl - passKeys = config.passKeys - if (enabled and passKeys) - setBrowserActionIcon(tabId,partialIcon) - else if (enabled) - setBrowserActionIcon(tabId,enabledIcon) - else - setBrowserActionIcon(tabId,disabledIcon) - # Propagate the new state only if it has changed. - if (isCurrentlyEnabled != enabled || currentPasskeys != passKeys) - chrome.tabs.sendMessage(tabId, { name: "setState", enabled: enabled, passKeys: passKeys, incognito: tab.incognito }) - else - setBrowserActionIcon tabId, disabledIcon +# Here's how we set the page icon. The default is "disabled", so if we do nothing else, then we get the +# grey-out disabled icon. Thereafter, we only set tab-specific icons, so there's no need to update the icon +# when we visit a tab on which Vimium isn't running. +# +# For active tabs, when a frame starts, it requests its active state via isEnabledForUrl. We also check the +# state every time a frame gets the focus. Once the frame learns its active state, it updates the current +# tab's badge (but only if that frame has the focus). +# +# Exclusion rule changes (from either the options page or the page popup) propagate via the subsequent focus +# change. In particular, whenever a frame next gets the focus, it requests its new state and sets the icon +# accordingly. +# +setIcon = (request, sender) -> + path = switch request.icon + when "enabled" then "icons/browser_action_enabled.png" + when "partial" then "icons/browser_action_partial.png" + when "disabled" then "icons/browser_action_disabled.png" + chrome.browserAction.setIcon tabId: sender.tab.id, path: path handleUpdateScrollPosition = (request, sender) -> updateScrollPosition(sender.tab, request.scrollX, request.scrollY) @@ -429,7 +415,6 @@ chrome.tabs.onUpdated.addListener (tabId, changeInfo, tab) -> runAt: "document_start" chrome.tabs.insertCSS tabId, cssConf, -> chrome.runtime.lastError updateOpenTabs(tab) if changeInfo.url? - updateActiveState(tabId) chrome.tabs.onAttached.addListener (tabId, attachedInfo) -> # We should update all the tabs in the old window and the new window. @@ -466,8 +451,6 @@ chrome.tabs.onRemoved.addListener (tabId) -> delete frameIdsForTab[tabId] delete urlForTab[tabId] -chrome.tabs.onActiveChanged.addListener (tabId, selectInfo) -> updateActiveState(tabId) - unless chrome.sessions chrome.windows.onRemoved.addListener (windowId) -> delete tabQueue[windowId] @@ -681,6 +664,7 @@ sendRequestHandlers = refreshCompleter: refreshCompleter createMark: Marks.create.bind(Marks) gotoMark: Marks.goto.bind(Marks) + setIcon: setIcon setBadge: setBadge # We always remove chrome.storage.local/findModeRawQueryListIncognito on startup. diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee index 3577e6f3..b96157c1 100644 --- a/content_scripts/vimium_frontend.coffee +++ b/content_scripts/vimium_frontend.coffee @@ -162,6 +162,7 @@ initializePreDomReady = -> isEnabledForUrl = false chrome.runtime.sendMessage = -> chrome.runtime.connect = -> + window.removeEventListener "focus", onFocus requestHandlers = hideUpgradeNotification: -> HUD.hideUpgradeNotification() @@ -173,8 +174,6 @@ initializePreDomReady = -> getScrollPosition: -> scrollX: window.scrollX, scrollY: window.scrollY setScrollPosition: (request) -> setScrollPosition request.scrollX, request.scrollY executePageCommand: executePageCommand - getActiveState: getActiveState - setState: setState currentKeyQueue: (request) -> keyQueue = request.keyQueue handlerStack.bubbleEvent "registerKeyQueue", { keyQueue: keyQueue } @@ -183,7 +182,7 @@ initializePreDomReady = -> # In the options page, we will receive requests from both content and background scripts. ignore those # from the former. return if sender.tab and not sender.tab.url.startsWith 'chrome-extension://' - return unless isEnabledForUrl or request.name == 'getActiveState' or request.name == 'setState' + return unless isEnabledForUrl # These requests are delivered to the options page, but there are no handlers there. return if request.handler in [ "registerFrame", "frameFocused", "unregisterFrame" ] sendResponse requestHandlers[request.name](request, sender) @@ -210,35 +209,21 @@ window.initializeWhenEnabled = -> for type in [ "keydown", "keypress", "keyup", "click", "focus", "blur", "mousedown" ] do (type) -> installListener window, type, (event) -> handlerStack.bubbleEvent type, event installListener document, "DOMActivate", (event) -> handlerStack.bubbleEvent 'DOMActivate', event - installListener window, "focus", registerFocus installedListeners = true FindModeHistory.init() -setState = (request) -> - isEnabledForUrl = request.enabled - passKeys = request.passKeys - isIncognitoMode = request.incognito - initializeWhenEnabled() if isEnabledForUrl - handlerStack.bubbleEvent "registerStateChange", - enabled: isEnabledForUrl - passKeys: passKeys - -getActiveState = -> - Mode.updateBadge() - # getActiveState is called in each frame within the tab. However, only the response from the first frame is - # handled on the background page. Therefore, exclusion rule changes are not propagated to other frames. - # So, we force a state update for this frame, just in case. - # FIXME(smblott): This could be avoided if settings were propagated via chrome.storage. - checkIfEnabledForUrl() - return enabled: isEnabledForUrl, passKeys: passKeys, url: window.location.toString() - # -# The backend needs to know which frame has focus, and the active URL. +# Whenever we get the focus: +# - Reload settings (they may have changed). +# - Tell the background page this frame's URL. +# - Check if we should be enabled. # -registerFocus = -> - # settings may have changed since the frame last had focus - settings.load() - chrome.runtime.sendMessage handler: "frameFocused", frameId: frameId, url: window.location.toString() +onFocus = (event) -> + if event.target == window + settings.load() + chrome.runtime.sendMessage handler: "frameFocused", frameId: frameId, url: window.location.toString() + checkIfEnabledForUrl() +window.addEventListener "focus", onFocus # # Initialization tasks that must wait for the document to be ready. @@ -577,6 +562,15 @@ checkIfEnabledForUrl = (onStartUp = false) -> handlerStack.bubbleEvent "registerStateChange", enabled: isEnabledForUrl passKeys: passKeys + # Update the page icon, if necessary. + if document.hasFocus() + chrome.runtime.sendMessage + handler: "setIcon" + icon: + if isEnabledForUrl and not passKeys then "enabled" + else if isEnabledForUrl then "partial" + else "disabled" + # Exported to window, but only for DOM tests. window.refreshCompletionKeys = (response) -> diff --git a/pages/options.coffee b/pages/options.coffee index 61b055c2..f60f3bb4 100644 --- a/pages/options.coffee +++ b/pages/options.coffee @@ -295,8 +295,6 @@ initPopupPage = -> Option.saveOptions() $("saveOptions").innerHTML = "Saved" $("saveOptions").disabled = true - chrome.tabs.query { windowId: chrome.windows.WINDOW_ID_CURRENT, active: true }, (tabs) -> - chrome.extension.getBackgroundPage().updateActiveState(tabs[0].id) $("saveOptions").addEventListener "click", saveOptions -- cgit v1.2.3 From 5b84f5cf94d32b6e8be75448f9c06aa7f2068582 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Sun, 15 Mar 2015 15:38:36 +0000 Subject: Tidy up exclusion rules/propagation. --- content_scripts/vimium_frontend.coffee | 6 +++--- pages/options.coffee | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee index b96157c1..d0fc158c 100644 --- a/content_scripts/vimium_frontend.coffee +++ b/content_scripts/vimium_frontend.coffee @@ -150,7 +150,7 @@ initializePreDomReady = -> settings.load() initializeModes() - checkIfEnabledForUrl true # true means checkIfEnabledForUrl is being called on start up. + checkIfEnabledForUrl() refreshCompletionKeys() # Send the key to the key handler in the background page. @@ -547,7 +547,7 @@ onKeyup = (event) -> DomUtils.suppressPropagation(event) @stopBubblingAndTrue -checkIfEnabledForUrl = (onStartUp = false) -> +checkIfEnabledForUrl = -> url = window.location.toString() chrome.runtime.sendMessage { handler: "isEnabledForUrl", url: url }, (response) -> @@ -556,7 +556,7 @@ checkIfEnabledForUrl = (onStartUp = false) -> isIncognitoMode = response.incognito if isEnabledForUrl initializeWhenEnabled() - else if onStartUp and HUD.isReady() + else if HUD.isReady() # Quickly hide any HUD we might already be showing, e.g. if we entered insert mode on page load. HUD.hide() handlerStack.bubbleEvent "registerStateChange", diff --git a/pages/options.coffee b/pages/options.coffee index f60f3bb4..6545189b 100644 --- a/pages/options.coffee +++ b/pages/options.coffee @@ -304,7 +304,7 @@ initPopupPage = -> window.close() # Populate options. Just one, here. - exclusions = new ExclusionRulesOnPopupOption(url, "exclusionRules", onUpdated) + exclusions = new ExclusionRulesOnPopupOption url, "exclusionRules", onUpdated updateState() document.addEventListener "keyup", updateState -- cgit v1.2.3