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.

Sign up for GitHub

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

Jump to bottom

event should not work on iframe content (closes #1791, closes #1822 in testcafe) #1789

Merged
merged 6 commits into from Oct 15, 2018
Merged

event should not work on iframe content (closes #1791, closes #1822 in testcafe) #1789

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aa68f5e
event should not work on iframe content (closes #1822)
AlexKamaev Oct 11, 2018
b4d81fa
fix test
AlexKamaev Oct 11, 2018
a6e2559
fix safari tests
AlexKamaev Oct 11, 2018
b310334
focus should work while click should not
AlexKamaev Oct 12, 2018
1352847
changes
AlexKamaev Oct 12, 2018
b2775b9
improve
AlexKamaev Oct 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/client/sandbox/event/hover.js
View file
Open in desktop
Expand Up @@ -2,6 +2,8 @@ import INTERNAL_ATTRS from '../../../processing/dom/internal-attributes';
import SandboxBase from '../base';
import nativeMethods from '../native-methods';
import * as domUtils from '../../utils/dom';
import * as positionUtils from '../../utils/position';
import * as browserUtils from '../../utils/browser';

export default class HoverSandbox extends SandboxBase {
constructor (listeners) {
Expand Down Expand Up @@ -56,6 +58,14 @@ export default class HoverSandbox extends SandboxBase {
return jointParent;
}

_onHover ({ target, clientX, clientY }) {
const isIframeContent = domUtils.getTagName(target) === 'iframe' && positionUtils.isPositionInsideElement(target, clientX, clientY);
Copy link
Contributor

Choose a reason for hiding this comment

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

1)Convert expression
domUtils.getTagName(target) === 'iframe' && positionUtils.isPositionInsideElement(target, clientX, clientY)
to utility function isPositionInsideIframe(el, {x, y}) and move this to the src/client/utils/position.js file.

2)Merge onHover and _hover function together. And rewrite using fast forward paradigm

_hover() {
 const isPositionIsInIframe = postionUtils.isInIframe(el, x, y);
 const shouldMarkElement = browserUtils.isIE && isPositionIsInIframe;

if (!shouldMarkElement)
  return;

....
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. ok
  2. I've separated them intentionally, because _onHover method takes x,y params while _hover is just api method without any extra params. Besides _hover is used in our tests

const hoverIsDisabled = browserUtils.isIE && isIframeContent;

if (!hoverIsDisabled)
this._hover(target);
}

_hover (el) {
if (!this.hoverElementFixed && !domUtils.isShadowUIElement(el)) {
const jointParent = this._clearHoverMarkerUntilJointParent(el);
Expand All @@ -77,6 +87,6 @@ export default class HoverSandbox extends SandboxBase {
attach (window) {
super.attach(window);

this.listeners.addInternalEventListener(window, ['mouseover', 'touchstart'], e => this._hover(e.target));
this.listeners.addInternalEventListener(window, ['mouseover', 'touchstart'], e => this._onHover(e));
}
}
7 changes: 6 additions & 1 deletion src/client/sandbox/event/simulator.js
View file
Open in desktop
Expand Up @@ -3,7 +3,7 @@ import nativeMethods from '../native-methods';
import * as browserUtils from '../../utils/browser';
import * as domUtils from '../../utils/dom';
import * as eventUtils from '../../utils/event';
import { getOffsetPosition, offsetToClientCoords } from '../../utils/position';
import { getOffsetPosition, offsetToClientCoords, isPositionInsideElement } from '../../utils/position';
import { getBordersWidth } from '../../utils/style';

const TOUCH_EVENT_RADIUS = 25;
Expand Down Expand Up @@ -529,6 +529,11 @@ export default class EventSimulator {
}

_dispatchMouseRelatedEvents (el, args, dataTransfer) {
const isIframeContent = domUtils.getTagName(el) === 'iframe' && isPositionInsideElement(el, args.clientX, args.clientY);

if (isIframeContent && args.type !== 'mouseover' && args.type !== 'mouseenter')
return true;

const pointerRegExp = /mouse(down|up|move|over|out)/;

// NOTE: In IE, submit doesn't work if a click is simulated for some submit button's children (for example,
Expand Down
13 changes: 13 additions & 0 deletions src/client/utils/position.js
View file
Open in desktop
Expand Up @@ -226,6 +226,19 @@ export function getElementRectangle (el) {
return rectangle;
}

export function isPositionInsideElement (el, x, y) {
Copy link
Contributor

Choose a reason for hiding this comment

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

export function shouldRaiseMouseoverInsideIframe (iframe, { x, y })

const rect = getElementRectangle(el);
const borders = styleUtils.getBordersWidth(el);
const padding = styleUtils.getElementPadding(el);

const left = rect.left + borders.left + padding.left;
const top = rect.top + borders.top + padding.top;
const right = rect.left + rect.width - borders.right - padding.right;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment for this line

const bottom = rect.top + rect.height - borders.bottom - padding.bottom;

return x >= left && x <= right && y >= top && y <= bottom;
}

function calcOffsetPosition (el, borders, offsetPosition) {
const isSvg = domUtils.isSVGElementOrChild(el);

Expand Down
141 changes: 141 additions & 0 deletions test/client/fixtures/sandbox/event/event-test.js
View file
Open in desktop
@@ -1,4 +1,5 @@
var browserUtils = hammerhead.utils.browser;
var eventUtils = hammerhead.utils.event;
var nativeMethods = hammerhead.nativeMethods;
var listeners = hammerhead.sandbox.event.listeners;
var focusBlur = hammerhead.sandbox.event.focusBlur;
Expand Down Expand Up @@ -314,6 +315,15 @@ test('should not wrap invalid event handlers (GH-1251)', function () {
});

strictEqual(listeningCtx.getEventCtx(target, 'click').wrappers.length, storedHandlerWrappersCount);

handlers.forEach(function (handler) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a try/catch statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a test for invalid event handlers. See the line above: var handlers = [void 0, 1, null, 'str', {}];. The test adds some wrong event handlers, but does not remove them, so it leads to problems with next texts. I've added code which removes handlers in the same manner as code which adds handlers

try {
nativeMethods.windowRemoveEventListener.call(target, 'click', handler);
}
catch (e) {
//
}
});
};

testHandlers(window);
Expand All @@ -336,3 +346,134 @@ test('event.preventDefault call should change the event.defaultPrevented propert

eventSimulator.keydown(input);
});

asyncTest('mouse events in iframe', function () {
return createTestIframe()
.then(function (iframe) {
var $div = $('<div>').appendTo('body');
var $iframe = $(iframe);

$div.css('display', 'inline-block');

$iframe.css({
width: 300,
height: 100,
border: '5px solid black',
padding: 20,
backgroundColor: 'yellow'
});

$iframe.appendTo($div);

var actualEvents = [];

var simulatorMethods = [
'mousedown',
'mouseup',
'mousemove',
'mouseover',
'mouseenter',
'click',
'dblclick',
'contextmenu'
];

var allEvents = [
'pointerdown',
'mousedown',
'pointerup',
'mouseup',
'pointermove',
'mousemove',
'pointerover',
'mouseover',
'mouseenter',
'click',
'dblclick',
'contextmenu'
];

var eventsInsideFrame = ['pointerover', 'mouseover', 'mouseenter'];

if (!eventUtils.hasPointerEvents) {
const pointerRegExp = /pointer(down|up|move|over)/;

allEvents = allEvents.filter(function (eventName) {
return !pointerRegExp.test(eventName);
});

eventsInsideFrame = eventsInsideFrame.filter(function (eventName) {
return !pointerRegExp.test(eventName);
});
}

var getHandler = function (i) {
return function () {
actualEvents.push(allEvents[i]);
};
};

for (var i = 0; i < allEvents.length; i++)
iframe.addEventListener(allEvents[i], getHandler(i));

for (i = 0; i < simulatorMethods.length; i++)
eventSimulator[simulatorMethods[i]](iframe, { clientX: 190, clientY: 130 });

deepEqual(actualEvents, eventsInsideFrame);

actualEvents = [];

for (i = 0; i < simulatorMethods.length; i++)
eventSimulator[simulatorMethods[i]](iframe, { clientX: 190, clientY: 70 });

deepEqual(actualEvents, allEvents);

$div.remove();

start();
});

});

asyncTest('hover style in iframe', function () {
return createTestIframe()
.then(function (iframe) {
var $style = $('<style>').appendTo('body');
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use jquery for elements and property manipulation.

var $div = $('<div>').appendTo('body');
var $iframe = $(iframe);

$style.html('iframe: hover { background-color: blue!important; }');
$div.css('display', 'inline-block');

$iframe.css({
width: 300,
height: 100,
border: '5px solid black',
padding: 20,
backgroundColor: 'yellow'
});

$iframe.appendTo($div);

var initialBackgroundColor = $iframe.css('backgroundColor');

eventSimulator.mouseover(iframe, { clientX: 190, clientY: 130 });

if (browserUtils.isIE)
equal($iframe.css('backgroundColor'), initialBackgroundColor);
else
notEqual($iframe.css('backgroundColor'), initialBackgroundColor);

eventSimulator.mouseover($div[0], { clientX: 0, clientY: 0 });
equal($iframe.css('backgroundColor'), initialBackgroundColor);

eventSimulator.mouseover(iframe, { clientX: 190, clientY: 70 });
notEqual($iframe.css('backgroundColor'), initialBackgroundColor);

$div.remove();
$style.remove();

start();
});

});
46 changes: 46 additions & 0 deletions test/client/fixtures/utils/position-test.js
View file
Open in desktop
Expand Up @@ -39,3 +39,49 @@ test('`getOffsetPosition` with `roundFn` arg', function () {
notDeepEqual(offsetPositionCeil, offsetPositionRound);
strictEqual(elementInPoint, child);
});

test('`isPositionInsideElement`', function () {
var testDiv = document.querySelector(TEST_DIV_SELECTOR);
var div = document.createElement('div');

div.style.width = '300px';
div.style.height = '100px';

var borderWidth = 5;
var paddingWidth = 10;

div.style.border = borderWidth + 'px solid black';
div.style.padding = paddingWidth + 'px';

testDiv.appendChild(div);

var rect = positionUtils.getElementRectangle(div);

rect.right = rect.left + rect.width;
rect.bottom = rect.top + rect.height;

notOk(positionUtils.isPositionInsideElement(div, rect.left, rect.top));
notOk(positionUtils.isPositionInsideElement(div, rect.left, rect.bottom));
notOk(positionUtils.isPositionInsideElement(div, rect.right, rect.top));
notOk(positionUtils.isPositionInsideElement(div, rect.right, rect.bottom));

rect.left += borderWidth + paddingWidth;
rect.right -= borderWidth + paddingWidth;
rect.top += borderWidth + paddingWidth;
rect.bottom -= borderWidth + paddingWidth;

ok(positionUtils.isPositionInsideElement(div, rect.left, rect.top));
ok(positionUtils.isPositionInsideElement(div, rect.left, rect.bottom));
ok(positionUtils.isPositionInsideElement(div, rect.right, rect.top));
ok(positionUtils.isPositionInsideElement(div, rect.right, rect.bottom));

rect.left -= 1;
rect.right += 1;
rect.top -= 1;
rect.bottom += 1;

notOk(positionUtils.isPositionInsideElement(div, rect.left, rect.top));
notOk(positionUtils.isPositionInsideElement(div, rect.left, rect.bottom));
notOk(positionUtils.isPositionInsideElement(div, rect.right, rect.top));
notOk(positionUtils.isPositionInsideElement(div, rect.right, rect.bottom));
});