From d41fc98be643e88e5afa7941ccf7bdaa1e7d02bf Mon Sep 17 00:00:00 2001 From: Simon Lydell Date: Sat, 14 Nov 2015 11:40:38 +0100 Subject: [PATCH] Choose which element to scroll more wisely 'overflow' events _are_ triggered for ``, if it overflows. This means that there is no need to try to detect if `` is scrollable in `commands.scroll` using `.scrollHeight` and `.clientHeight`; we can simply use `vim.state.largestScrollableElement`. The above is not only a code cleanup; it also fixes a bug, mentioned in #605: On some sites (such as groups.google.com), `` might overflow, but not be scrollable because of `overflow: hidden;`. The old `.scrollHeight` vs. `.clientHeight` technique did not catch this, and wrongly tried to scroll `` when there was a more appropriate scrollable element on the page. `overflow: hidden;` _is_ taken care of for `vim.state.scrollableElements` and `vim.state.largestScrollableElement`, though, fixing the problem. This clears the confusion from commit 59cb2a9a: > Revert "Remove unnecessary code in commands.follow{,_focus}" > > This reverts commit 1a77a5d62036ee0d6029b36ee3c2ef33355b0d85. > > Apparently, I must be wrong. I get a hint marker for the entire page (if it is > scrollable) after this commit. So confused now. As noted above, `` (`document.documentElement`) _does_ get 'overflow' events, and thus ends up in `vim.state.scrollableElements`, so we _should_ exclude `document.documentElement` from getting a marker. There is no need to deal with quirks mode here, though. In quirks mode, `` is considered the root element and must be scrolled instead of ``, but 'overflow' events are still triggered for ``, and _not_ for ``. --- extension/lib/commands-frame.coffee | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/extension/lib/commands-frame.coffee b/extension/lib/commands-frame.coffee index 9f10861..a8f13a8 100644 --- a/extension/lib/commands-frame.coffee +++ b/extension/lib/commands-frame.coffee @@ -44,21 +44,25 @@ commands.scroll = (args) -> activeElement = utils.getActiveElement(vim.content) document = activeElement.ownerDocument - if vim.state.scrollableElements.has(activeElement) - element = activeElement - else - element = + element = switch + when vim.state.scrollableElements.has(activeElement) + activeElement + # If the currently focused element isnât scrollable, scroll the largest + # scrollable element instead, which usually means ``. + when vim.state.largestScrollableElement + { largestScrollableElement } = vim.state # In quirks mode (when the page lacks a doctype) `` is considered - # the root element rather than ``. - if document.compatMode == 'BackCompat' and document.body? + # the root element rather than ``. The 'overflow' event is triggered + # for `` though (_not_ ``!). + if largestScrollableElement == document.documentElement and + document.compatMode == 'BackCompat' and document.body? document.body else - document.documentElement - # If the entire page isnât scrollable, scroll the largest scrollable element - # instead (if any). - if element.scrollHeight <= element.clientHeight and - vim.state.largestScrollableElement - element = vim.state.largestScrollableElement + largestScrollableElement + else + # This point should never be reached, but itâs better to be safe than + # sorry. Not being able to scroll is very annoying. Use the best bet. + document.documentElement options = {} options[direction] = switch type -- 2.39.3