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

Add pref-gated methods for testing activation #12343

Closed
Manishearth opened this issue Jul 8, 2016 · 10 comments
Closed

Add pref-gated methods for testing activation #12343

Manishearth opened this issue Jul 8, 2016 · 10 comments

Comments

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Jul 8, 2016

From script, currently you can only trigger synthetic activations with .click(). However, there is a lot of intricacy around activation that is not observable (and thus not testable from JS), which we really should be testing. PRs like #11781 generally add an HTML test, but I'd prefer to have servo-specific WPT tests here. Eventually, webdriver would be the solution, but so far we don't have that up yet.

I'd like to do something similar to #12322 (comment), use a testing pref that enables methods on elements that lets you trigger activation, cancel activation, and check if something is activatable, among any other functionality that tests may find useful. Then, of course, we should write tons of tests for activation corner cases that use these 😄

cc @jdm @Ms2ger

@sjmelia: Would you like to work on this?

@izgzhen
Copy link
Contributor

@izgzhen izgzhen commented Jul 8, 2016

I bumped into a tidy problem saying that I should add spec link comment to our special methods. Should I modify the linter, or there is another workaround?

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jul 8, 2016

Just say "internal testing method" and have the tidy linter skip such strings

@izgzhen
Copy link
Contributor

@izgzhen izgzhen commented Jul 8, 2016

@Manishearth

if "// check-tidy: no specs after this line" in line:
I found this can work: // check-tidy: no specs after this line

bors-servo added a commit that referenced this issue Jul 9, 2016
Add ability to WPT-test file uploads and fetches, fixes #12322

Using `inputElem.selectFiles(["path1", "path2"])` in JavaScript with pref `dom.htmlinputelement.select_files.enabled` = `true` will simulate the effect of `inputElem.click()`.

See #12322  for more, also related to #12343.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12344)
<!-- Reviewable:end -->
@sjmelia
Copy link
Contributor

@sjmelia sjmelia commented Jul 9, 2016

I would absolutely love to work on this!

The end goal w.r.t. PR #11781 would be a test that looks something like this:

element.enter_activation_state(); // only valid when dom.testing.element.activation.enabled:true
assert_true(document.querySelector(':active') === element, 
            'Element was not given pseudoselector');
element.exit_activation_state();

My question would be - why not instead pref-gate a mousedown/mouseup similar to the gecko privileged nsIDOMWindowUtils.sendMouseEvent? Is that crazy talk? Regardless it sounds a lot more challenging - maybe too challenging - so i'll have a go at the above first in any case.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jul 10, 2016

We could do those, too. @jdm thoughts?

I would prefer having specific activation ones too, because there is some logic lodged between activation and mouse clicks that you may want to bypass.

@jdm
Copy link
Member

@jdm jdm commented Jul 11, 2016

We are almost at the point where we can write tests using webdriver that a) don't require adding gated, privileged APIs, b) can be run in multiple browsers, so I don't think that we should spend too much effort on these things right now.

@jdm
Copy link
Member

@jdm jdm commented Jul 11, 2016

Then again, if there are things we would like to test right now we should go ahead and do what's necessary for them. The webdriver stuff is gated on #12223 and then figuring out why the existing webdriver tests fail before starting to write more.

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jul 11, 2016

Oh, we are?

I was under the impression that webdriver was still quite far away.

Anyway, this might be nice to have independent of webdriver's existence, because while webdriver will let you simulate clicks, you can't directly deal with activation -- like I said, I'd like to isolate the activation logic too.

I was also thinking about having a DOM mocking framework that lets us unit test the DOM, with a window/script thread and mockable resource task. But let's wait for webdriver to happen before we look into that.

@sjmelia
Copy link
Contributor

@sjmelia sjmelia commented Jul 19, 2016

I do intend to work on this, just a little busy these last few weeks.

I'll implement manishearth's suggestion as fiddling with the interfaces exposed to JS does sound like fun, and if necessary I can always pick up another case to back it out in favour of, or complement it with; a webdriver test.

Would it be possible to isolate the activation behaviour with a unit test? I.e. in Rust. Of course there are a very complicated set of dependencies?

@Manishearth
Copy link
Member Author

@Manishearth Manishearth commented Jul 19, 2016

We'd have to initiate all the JS/DOM stuff. It's doable, but I'm not sure if we should be doing it 😄

bors-servo added a commit that referenced this issue Aug 8, 2016
Fix Issue 12343: Add pref-gated methods and test for activatable element

<!-- Please describe your changes on the following line: -->
Add methods to activatable elements; gated by a preference, for use in servo-specific WPT tests.

I cargo-culted over the "redundant check" from #12322, I don't really understand the possible exploit.

Obvious flaw is the copypasta for each element. I understand that the webidl codegen will produce a trait for ActivatableElement, but I can't see how I can provide a default implementation for this? I guess otherwise could be an improvement to have each element just delegate to an Enter.../Exit... method on the Activatable trait?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #12343 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

…states

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12703)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 17, 2016
Fix Issue 12343: Add pref-gated methods and test for activatable element

<!-- Please describe your changes on the following line: -->
Add methods to activatable elements; gated by a preference, for use in servo-specific WPT tests.

I cargo-culted over the "redundant check" from #12322, I don't really understand the possible exploit.

Obvious flaw is the copypasta for each element. I understand that the webidl codegen will produce a trait for ActivatableElement, but I can't see how I can provide a default implementation for this? I guess otherwise could be an improvement to have each element just delegate to an Enter.../Exit... method on the Activatable trait?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #12343 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

…states

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12703)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.