From 46fd6b2e23e0e7d238092078586ba9b4d52aeebd Mon Sep 17 00:00:00 2001 From: Simon Lydell Date: Sun, 22 Nov 2015 11:03:59 +0100 Subject: [PATCH] Improve largest scrollable element detection MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit In commit f405054c a special case was added for the root element: > Consider the entire page as largest if scrollable > > Its area may technically be less than other elements, but if it is scrollable > that's what you expect to scroll. However, if you embed such a page in a frame, the page won’t be scrollable (because it is not the uppermost root element). This made me realize that it makes the most sense to consider the _uppermost scrollable element_ the largest. In other words, if a scrollable element contains another scrollable element (or a frame containing one), the parent should be considered largest even if the child has greater area. --- documentation/commands.md | 13 ++++++++++--- extension/lib/commands-frame.coffee | 6 +++--- extension/lib/help.coffee | 2 +- extension/lib/scrollable-elements.coffee | 17 ++++++++--------- extension/lib/utils.coffee | 13 ++++++++++++- 5 files changed, 34 insertions(+), 17 deletions(-) diff --git a/documentation/commands.md b/documentation/commands.md index 5e2eaf1..2bed874 100644 --- a/documentation/commands.md +++ b/documentation/commands.md @@ -96,14 +96,21 @@ space by default. VimFx provides similar scrolling commands (and actually overrides ``), but they work a little bit differently. They scroll _the currently focused element._ If the currently focused element -isn’t scrollable, or there is no (apparent) currently focused element, the -entire page is scrolled. Finally, if the entire page isn’t scrollable, the -largest scrollable element on the page (if any) is scrolled. +isn’t scrollable, the largest scrollable element on the page (if any, and +including the entire page itself) is scrolled. You can focus scrollable elements using the `zf` command (or the `f` command). The right border of hint markers for scrollable elements is styled to remind of a scroll bar, making them easier to recognize among hints for links. +Note that `zf` and `f` do _not_ add a hint marker for the _largest_ scrollable +element (such as the entire page). There’s no need to focus that element, since +it is scrolled by default if no other scrollable element is focused, as +explained above. (This prevents the largest scrollable element from likely +eating your best hint char on most pages; see [The `f` commands]). + +[The `f` commands]: #the-f-commands-1 + ## `gi` diff --git a/extension/lib/commands-frame.coffee b/extension/lib/commands-frame.coffee index 8591748..74c2baa 100644 --- a/extension/lib/commands-frame.coffee +++ b/extension/lib/commands-frame.coffee @@ -57,7 +57,7 @@ commands.scroll = ({vim, method, type, direction, amount, property, smooth}) -> # point) instead because we cannot be 100% sure that nothing is scrollable # (for example, if VimFx is updated in the middle of a session). Not being # able to scroll is very annoying. - vim.state.scrollableElements.root() + vim.state.scrollableElements.quirks(document.documentElement) options = {} options[direction] = switch type @@ -101,7 +101,7 @@ commands.follow = ({vim}) -> type = 'clickable' unless isXUL or element.nodeName in ['A', 'INPUT', 'BUTTON'] semantic = false - when element != vim.state.scrollableElements.root(element) and + when element != vim.state.scrollableElements.largest and vim.state.scrollableElements.has(element) type = 'scrollable' when element.hasAttribute('onclick') or @@ -194,7 +194,7 @@ commands.follow_focus = ({vim}) -> type = switch when element.tabIndex > -1 'focusable' - when element != vim.state.scrollableElements.root(element) and + when element != vim.state.scrollableElements.largest and vim.state.scrollableElements.has(element) 'scrollable' return unless type diff --git a/extension/lib/help.coffee b/extension/lib/help.coffee index ae23979..36d4190 100644 --- a/extension/lib/help.coffee +++ b/extension/lib/help.coffee @@ -71,7 +71,7 @@ createHeader = (document, vimfx) -> $('title', mainHeading, translate('help.title')) closeButton = $('close-button', header, '×') - closeButton.onclick = removeHelp.bind(null, document.defaultView) + closeButton.onclick = removeHelp.bind(null, document.ownerGlobal) return header diff --git a/extension/lib/scrollable-elements.coffee b/extension/lib/scrollable-elements.coffee index c4f4c37..9342782 100644 --- a/extension/lib/scrollable-elements.coffee +++ b/extension/lib/scrollable-elements.coffee @@ -41,10 +41,6 @@ class ScrollableElements else return element - root: (element = null) -> - document = if element then element.ownerDocument else @window.document - return @quirks(document.documentElement) - has: (element) -> @elements.has(@quirks(element)) add: (element) -> @@ -67,12 +63,15 @@ class ScrollableElements return element.scrollTopMax >= @MINIMUM_SCROLL or element.scrollLeftMax >= @MINIMUM_SCROLL + # It makes the most sense to consider the uppermost scrollable element the + # largest. In other words, if a scrollable element contains another scrollable + # element (or a frame containing one), the parent should be considered largest + # even if the child has greater area. isLargest: (element) -> - # Always consider the toplevel document the largest scrollable element, if - # it is scrollable. (Its area may be smaller than other elements). - root = @root() - return not @largest or element == root or - (@largest != root and utils.area(element) > utils.area(@largest)) + return true unless @largest + return true if utils.containsDeep(element, @largest) + return false if utils.containsDeep(@largest, element) + return utils.area(element) > utils.area(@largest) updateLargest: -> # Reset `@largest` and find a new largest scrollable element (if there are diff --git a/extension/lib/utils.coffee b/extension/lib/utils.coffee index ac2a243..5e6ddf8 100644 --- a/extension/lib/utils.coffee +++ b/extension/lib/utils.coffee @@ -177,7 +177,7 @@ suppressEvent = (event) -> # elements.) eventSequence = ['mouseover', 'mousedown', 'mouseup', 'click', 'command'] simulateClick = (element) -> - window = element.ownerDocument.defaultView + window = element.ownerGlobal for type in eventSequence mouseEvent = new window.MouseEvent(type, { # Let the event bubble in order to trigger delegated event listeners. @@ -196,6 +196,16 @@ simulateClick = (element) -> area = (element) -> return element.clientWidth * element.clientHeight +containsDeep = (parent, element) -> + parentWindow = parent.ownerGlobal + elementWindow = element.ownerGlobal + + while elementWindow != parentWindow and elementWindow.top != elementWindow + element = elementWindow.frameElement + elementWindow = element.ownerGlobal + + return parent.contains(element) + createBox = (document, className, parent = null, text = null) -> box = document.createElement('box') box.className = className @@ -324,6 +334,7 @@ module.exports = { simulateClick area + containsDeep createBox insertText querySelectorAllDeep -- 2.39.3