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

[WIP] Add double click action #675

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

joshgordon
Copy link

This PR adds the ability to trigger double-clicks on elements.

Copy link
Member

@danyalaytekin danyalaytekin left a comment

Choose a reason for hiding this comment

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

Hi @joshgordon, thank you for proposing this new action and for updating the unit tests too.

Was there a particular scenario that you were trying to test, and couldn't... text selection maybe? I'd be interested in hearing about it. Or if this is more just an altruistic effort to expand the portfolio of Pa11y Actions that's great too.

I've left a few low level comments inline - please let me know your thoughts.

Finally, I'm thinking it could be good to add an integration test, the motivation to do this increasing when I spotted this Stack Overflow comment, even if its content no longer applies or came from a misunderstanding.

Here are some example files for our integration test for click:

Thanks again Josh. Any questions please ask.

lib/action.js Outdated Show resolved Hide resolved
lib/action.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
test/unit/lib/action.test.js Outdated Show resolved Hide resolved

});

describe('.run(browser, page, options, matches) double click', () => {
Copy link
Member

Choose a reason for hiding this comment

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

As this is now inconsistent with the sibling above, I think this could all be reorganised into this, or something along these lines:

describe('.run(browser, page, options, matches)', () => { 	// line 233

	// common items

	describe('click', ...);			// new, to wrap items relevant only to click
	describe('double click', ...);		// new, to wrap items relevant only to double click
});

test/unit/lib/action.test.js Outdated Show resolved Hide resolved
clickElementResolvedValue = await clickElementAction.run(puppeteer.mockBrowser, puppeteer.mockPage, {}, clickElementMatches);
});

it('clicks the specified element on the page', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('clicks the specified element on the page', () => {
it('double clicks the specified element on the page', () => {

@joshgordon
Copy link
Author

Was there a particular scenario that you were trying to test, and couldn't... text selection maybe?

We have an ext.js application that expects double-clicks on data-rows to view/change the remainder of the properties of the object described by the row. I played around with clicking multiple times but none of them got the modal to pop up, and finally came to the conclusion that I just needed to patch it in myself.

I went ahead and added all the suggestions made. I'll try to find a couple cycles to put together an integration test - that looks straightforward enough.

@danyalaytekin danyalaytekin added this to the 8.x milestone Mar 18, 2024
@danyalaytekin danyalaytekin marked this pull request as draft March 21, 2024 04:39
@danyalaytekin danyalaytekin changed the title Add double click action [WIP] Add double click action Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants