Commit 3a173f65 authored by Robert Knight's avatar Robert Knight

Work around `getComputedStyle` bug in Firefox

`<thread-list>` uses `getComputedStyle` to find the nearest scrollable
ancestor element which it needs to know in order to scroll specific
annotations into view and determine which annotations are visible.

In Firefox `getComputedStyle` may return `null` if the iframe is hidden
- which may be the case when this code is called in the sidebar.  This
is considered a bug upstream. Work around it in the meantime by adding a
specific class (`js-scrollable`) to the scrollable element and testing
for that as a fallback.

Fixes #341
parent a48ef194
...@@ -42,12 +42,13 @@ var threadFixtures = immutable({ ...@@ -42,12 +42,13 @@ var threadFixtures = immutable({
var fakeVirtualThread; var fakeVirtualThread;
function FakeVirtualThreadList($scope, $window, rootThread) { function FakeVirtualThreadList($scope, $window, rootThread, options) {
fakeVirtualThread = this; // eslint-disable-line consistent-this fakeVirtualThread = this; // eslint-disable-line consistent-this
var thread = rootThread; var thread = rootThread;
this.options = options;
this.setRootThread = function (_thread) { this.setRootThread = function (_thread) {
thread = _thread; thread = _thread;
}; };
...@@ -81,6 +82,7 @@ describe('threadList', function () { ...@@ -81,6 +82,7 @@ describe('threadList', function () {
// Create a scrollable container for the `<thread-list>` so that scrolling // Create a scrollable container for the `<thread-list>` so that scrolling
// can be tested. // can be tested.
var parentEl = document.createElement('div'); var parentEl = document.createElement('div');
parentEl.classList.add('js-scrollable');
parentEl.style.overflow = 'scroll'; parentEl.style.overflow = 'scroll';
parentEl.style.height = '100px'; parentEl.style.height = '100px';
...@@ -210,4 +212,16 @@ describe('threadList', function () { ...@@ -210,4 +212,16 @@ describe('threadList', function () {
assert.calledWithMatch(inputs.onSelect.callback, assert.calledWithMatch(inputs.onSelect.callback,
sinon.match(annotFixtures.annotation)); sinon.match(annotFixtures.annotation));
}); });
// See https://github.com/hypothesis/client/issues/341
it('finds correct scroll root if `window.getComputedStyle` fails', function () {
sinon.stub(window, 'getComputedStyle').returns(null);
var element = createThreadList();
element.scope.$digest();
var scrollRoot = fakeVirtualThread.options.scrollRoot;
assert.isTrue(scrollRoot.classList.contains('js-scrollable'));
window.getComputedStyle.restore();
});
}); });
...@@ -43,6 +43,23 @@ var virtualThreadOptions = { ...@@ -43,6 +43,23 @@ var virtualThreadOptions = {
}, },
}; };
/**
* Return true if the given element is scrollable.
*/
function isScrollable(el) {
// Check the computed style to see if this element is marked as scrollable.
var computedStyle = window.getComputedStyle(el);
if (computedStyle !== null && computedStyle.overflowY === 'scroll') {
return true;
}
// In Firefox, `getComputedStyle` may return `null` if `window` is inside a
// non-rendered (`display:none`) iframe. In this case, fall back to requiring
// the developer to annotate the element with a special class.
// See https://bugzilla.mozilla.org/show_bug.cgi?id=548397 and
// https://github.com/w3c/csswg-drafts/issues/572
return el.classList.contains('js-scrollable');
}
/** /**
* Return the closest ancestor of `el` which is scrollable. * Return the closest ancestor of `el` which is scrollable.
* *
...@@ -51,8 +68,7 @@ var virtualThreadOptions = { ...@@ -51,8 +68,7 @@ var virtualThreadOptions = {
function closestScrollableAncestor(el) { function closestScrollableAncestor(el) {
var parentEl = el; var parentEl = el;
while (parentEl !== document.body) { while (parentEl !== document.body) {
var computedStyle = window.getComputedStyle(parentEl); if (isScrollable(parentEl)) {
if (computedStyle.overflowY === 'scroll') {
return parentEl; return parentEl;
} }
parentEl = parentEl.parentElement; parentEl = parentEl.parentElement;
......
<div class="app-content-wrapper" h-branding="appBackgroundColor"> <div class="app-content-wrapper js-scrollable" h-branding="appBackgroundColor">
<top-bar <top-bar
auth="vm.auth" auth="vm.auth"
on-login="vm.login()" on-login="vm.login()"
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment