Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Images with style=background-image: url("img.png"); aren't loaded (close #1212) #1293

Merged
merged 4 commits into from
Sep 6, 2017

Conversation

LavrovArtem
Copy link
Contributor

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 30c4782 have failed. See details:

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit 34557f8 have passed. See details:

@@ -129,6 +129,23 @@ export default class PropertyAccessorsInstrumentation extends SandboxBase {
return HTML_ELEMENT_TEXT_PROPERTIES[prop];
}

static _createStyleInstrumentation (property) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_createForStyleProperty (property)

@@ -193,6 +193,22 @@ class NativeMethods {
if (textAreaValueDescriptor && typeof textAreaValueDescriptor.set === 'function')
this.textAreaValueSetter = textAreaValueDescriptor.set;

// stylesheets
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Stylesheets

@@ -68,6 +68,29 @@ export default class WindowSandbox extends SandboxBase {
return String(reason);
}

static _wrapCSSGetPropertyValueIfNecessary (constructor, nativeGetPropertyValue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename function parameter to the nativeGetPropertyValueFn

}
}

static _wrapCSSSetPropertyIfNecessary (constructor, nativeSetProperty) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nativeSetPropertyFn

cssProperties.forEach(function (prop) {
var value = 'url(' + url + ')';

// NOTE: We cannot get url through 'borderImage' or 'border-image' properties
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: If we setup borderImageorborder-imageproperty then it affects aborderImageSource` property.


el.style[prop] = value;

var controlValue = el.style[propForChecking];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var nativeValue

var value = 'url(' + url + ')';

// NOTE: We cannot get url through 'borderImage' or 'border-image' properties
var propForChecking = prop === 'borderImage' || prop === 'border-image' ? 'borderImageSource' : prop;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var affectedProp = ...

var url = 'http://some.domain.com/image.png';
var proxyUrl = urlUtils.getProxyUrl(url);

div.style.setProperty('background', 'url(' + url + ')', '');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test for non-string url value case
For example:
div.style.setProperty('background', null)

@@ -375,3 +375,15 @@ test('the constructor field of a function should return a wrapped Function objec
Function.prototype.toString = nativeToString;
/*eslint-enable no-extend-native*/
});

test('getPropertyValue and setProperty methods of css object should be overridden (GH-1212)', function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also check style.removeProperty.
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration/removeProperty

It should returns non-proxied value.

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit 797af00 have passed. See details:

@LavrovArtem
Copy link
Contributor Author

FPR

@LavrovArtem
Copy link
Contributor Author

ping @helen-dikareva

Copy link
Collaborator

@helen-dikareva helen-dikareva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@LavrovArtem LavrovArtem merged commit f81cb16 into DevExpress:master Sep 6, 2017
@LavrovArtem LavrovArtem deleted the i1212 branch September 6, 2017 10:31
AndreyBelym pushed a commit to AndreyBelym/testcafe-hammerhead that referenced this pull request Feb 28, 2019
…d` (close DevExpress#1212) (DevExpress#1293)

* fix `Images with style=background-image: url("img.png"); aren't loaded` (close DevExpress#1212)

* renaming

* fix tests

* fix review's issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants