search

seiyria / bootstrap-slider

A slider control for Bootstrap 3 & 4.
http://seiyria.github.io/bootstrap-slider/
Other
3k stars 1.14k forks source link

Don't access null result from getComputedStyle #869

Closed xTVaser closed 5 years ago

xTVaser commented 5 years ago

Hello, when using this library in a front-end that was exposed via an iframe I ran into a long standing firefox bug where getComputedStyle would return null. https://bugzilla.mozilla.org/show_bug.cgi?id=548397

This is the workaround I implemented to work-around this issue. Very recently, this bug has finally been resolved in version 62, but I still figured it would be worth contributing considering the bug could still be hit on the last ~9 years of firefox releases.

Let me know if unit-tests / a JSFiddle is required for this fix and I'll see what I can do, I'm not quite sure if I could reproduce the same iframe-dependent behavior in a JSFiddle at this point.

Pull Requests

Please accompany all pull requests with the following (where appropriate):

seiyria commented 5 years ago

Well-researched and fairly simple. I don't know that it would be easy to demo this like you said so I'll leave this one to @rovolution. He might have some thoughts on it. Looks good to me though!

xTVaser commented 5 years ago

@rovolution I tired to add a spec / tests using the snippet you posted but I don't think that will work. Since jasmine is running it's tests in PhantomJS it seems to override the window object back to normal and not return null. One idea would be to pull the window object access into a function that could be mocked/spied on:

function getComputedStyleResult(element) {
    return window.getComputedStyle(element);
}
// Check options.rtl
if(this.options.rtl==='auto'){
    var computedStyle = getComputedStyleResult(this.element);
    // ....
}

However to be honest I'm not the most familiar with JS, any idea how to refer to that function? Jasmine expects spyOn(<object>, <func name>) and there is no obvious object structure in bootstrap-slider.js, just nested functions.

rovolution commented 5 years ago

@xTVaser ah dont worry about it. thanks for at least trying!

jespirit commented 5 years ago

Current version: v10.2.3

It seems if you run grunt test on the command line, the tests will pass because somehow window.getComputedStyle() is still valid, when it should be null.

However, if you run grunt jasmine --filter=filename the tests will actually fail.

Output:

$ grunt jasmine --filter=firefox
(node:6324) [DEP0022] DeprecationWarning: os.tmpDir() is deprecated. Use os.tmpdir() instead.
Running "jasmine:src" (jasmine) task
Testing jasmine specs via PhantomJS

 getComputedStyle() returns null in Firefox
   - computes orientation properly in an iframe - 'rtl' should be false......×
     TypeError: null is not a constructor (evaluating 'window.getComputedStyle(this.element)') in <path>/bootstrap-slider/temp/bootstrap-slider.js (line 384) (1)
   - computes orientation properly in an iframe - 'rtl' should be true......×
     TypeError: null is not a constructor (evaluating 'window.getComputedStyle(this.element)') in <path>/bootstrap-slider/temp/bootstrap-slider.js (line 384) (1)

2 specs in 0.007s.
>> 2 failures
Fatal error: Callback must be a function

This is the unit test that I wrote:

FirefoxGetComputedStyleSpec.js

describe("getComputedStyle() returns null in Firefox", function() {
    var getComputedStyle;
    var testSlider;

    beforeEach(function() {
        // temporarily reassign to a variable and set to null
        getComputedStyle = window.getComputedStyle;
        window.getComputedStyle = null;
    });

    it("computes orientation properly in an iframe - 'rtl' should be false", function() {
        testSlider = new Slider(document.getElementById("testSlider1"), {
            rtl: 'auto'
        });
        expect(testSlider.getAttribute('rtl')).toBe(false);
    });

    it("computes orientation properly in an iframe - 'rtl' should be true", function() {
        testSlider = new Slider(document.getElementById("rtlSlider"));
        expect(testSlider.getAttribute('rtl')).toBe(true);
    });

    afterEach(function() {
        // restore
        window.getComputedStyle = getComputedStyle;
        if (testSlider) {
            if (testSlider instanceof Slider) { testSlider.destroy(); }
            testSlider = null;
        }
    });
});

Edit: Shouldn't it be window.getComputedStyle = function() { return null; } ?

rovolution commented 5 years ago

Shouldn't it be window.getComputedStyle = function() { return null; }

ah yea you are correct 😄 try that and see if it works

xTVaser commented 5 years ago

@jespirit Yeah that test is similar to what I attempted, I also overrode window.getComputedStyle to be a function that returns null rather than equal to null; but it exhibits the same issue:

If logs are added to see if the lines that we are interested in are getting hit, we can see that they are not:

if(this.options.rtl==='auto'){
    var computedStyle = window.getComputedStyle(this.element);
    console.log("getComputedStyle result in slider creation - " + window.getComputedStyle(this.element))
    if (computedStyle != null) {
        console.log("getComputedStyle is not null")
        this.options.rtl = computedStyle.direction === 'rtl';
    } else {
        console.log("getComputedStyle is null")
        // Fix for Firefox bug in versions less than 62:
        // https://bugzilla.mozilla.org/show_bug.cgi?id=548397
        // https://bugzilla.mozilla.org/show_bug.cgi?id=1467722
        this.options.rtl = this.element.style.direction === 'rtl';
    }
}

output:

getComputedStyle() returns null in Firefox
   - computes orientation properly in an iframe - 'rtl' should be false...
log: getComputedStyle result after overriding - null

log: getComputedStyle result in slider creation - [object CSSStyleDeclaration]

log: getComputedStyle is not null

It's because the tests are ran in PhantomJS so the headless browser restores the window object and such (I presume). The test passes, but it doesn't add coverage to what we are interested in unfortunately.

I also noticed its a little risky to globally override window.getComputedStyle, it causes some side-effects in some of the existing RtlOptionsSpec tests where underlying jquery code depends on getComputedStyle.