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

adding all the other WebDriver commands #1465

Merged
merged 4 commits into from
Apr 12, 2018
Merged

adding all the other WebDriver commands #1465

merged 4 commits into from
Apr 12, 2018

Conversation

chrisdavidmills
Copy link
Contributor

This PR adds all the WebDriver commands to the repo, a total of 54.

Andreas has assured me that to his knowledge, support of all the commands is the same. The idea here is to get these in the repo, and then we can make adjustments if any differences are encountered.

@chrisdavidmills
Copy link
Contributor Author

So basically, a review would just consist of making sure the name and the URL slug inside each file match the file name, in each case.

@teoli2003 teoli2003 added data:webdriver Compat data for WebDriver features. https://developer.mozilla.org/docs/Web/WebDriver HackOnMDNParis2018 Issues or pull requests at the Hack on MDN event in Paris in March 2018 labels Mar 17, 2018
Copy link
Contributor

@teoli2003 teoli2003 left a comment

Choose a reason for hiding this comment

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

I will review the rest once the IE6 question is answered.

"version_added": false
},
"ie": {
"version_added": "6"
Copy link
Contributor

Choose a reason for hiding this comment

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

This bit seems dubious. @andreastt, can you confirm IE never implemented WebDriver?

Copy link
Contributor

Choose a reason for hiding this comment

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

There exists a community-supported spec-conforming implementation for IE >= 7. According to https://github.com/SeleniumHQ/selenium/wiki/InternetExplorerDriver it seems it does no longer support IE 6 as I told @chrisdavidmills.

By contrast, the vendor-supported implementation for Edge is not spec-conforming so we cannot reasonably claim that it is compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @andreastt
I think we should do the following:

  1. Mark IE support as a "partial_implementation": true and add a "notes": "Support for WebDriver is obtained via the InternetExplorerDriver tool [https://github.com/SeleniumHQ/selenium/wiki/InternetExplorerDriver]" (or similar).
  2. Keep the false value for Edge but add a note: "The vendor-supported implementation for Edge is not spec-conforming and isn't compatible."

@Elchi3 what do you think of this?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me. We could also reach out to the Microsoft folks to see what say think here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Support for IE isn’t partial, it is full. I don’t think we ought to discriminate against non-vendor implementations here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we should update all of the IE support versions to 7. I don't think we need to point to all of the driver tools for the different browsers. Use of those is expected by anyone trying to use WebDriver.

I'd also be happy to add a note about non-conforming support for browsers, if @andreastt thinks it would be useful. I think it is useful to know there is support there, but it is non-standard, rather than just no support at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there any other cases similar to Edge that we also need to report?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me try to elaborate on the current situation. Certain browsers’ implementations are based on an earlier open source protocol called the Selenium JSON wire protocol. This protocol is being replaced by the W3C WebDriver protocol, and is so far implemented and shipped in Firefox and IE.

Through various shims, a wide range of clients offer forward- and backward compatibility with both protocols. A user might plausibly not observe any difference using WebDriver with a conforming remote end vs. a non-conforming remote end.

Whilst there is ongoing work with transitioning Chrome, Edge, and Safari to the new protocol, they do not ship with this new behaviour enabled by default. It is because of this that I think it is problematic for BCD to claim they are compatible when that compatibility is only a happenstance due to diligent client maintainers.

MDN documents the web platform, not arbitrary tools for interacting with it. I would be fine with adding a note saying that there exists an implementation but that it is non-conforming, but I would be anxious if that meant they turn up as green in the matrix chart.

I hope this helps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does help - thanks! So to make it clear, specifying support added false with a note about the non-conforming implementation means it will turn up as red in the matrix. No worries there.

From what you've said above, it looks like I need to add the note about a non-conforming implementation being present to Chrome/Opera/Samsung, Edge, and Safari? Correct?

As well as updating IE support to 7, of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct.

@chrisdavidmills
Copy link
Contributor Author

I've updated a single JSON file for now with the new information — Accept_Alert.json. Can I just get a review on this one to confirm we think the data looks OK, before I update all the others? @Elchi3 @teoli2003 @andreastt

Thanks!

@Elchi3
Copy link
Member

Elchi3 commented Apr 11, 2018

Looks good to me and I think it summarizes well what @andreastt said above. I would go ahead and add this everywhere so that this can be merged :)

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

r+

@Elchi3 Elchi3 dismissed teoli2003’s stale review April 12, 2018 07:54

comments addressed

@Elchi3 Elchi3 merged commit f70a85c into mdn:master Apr 12, 2018
@chrisdavidmills chrisdavidmills deleted the webdriver-standard-commands branch April 12, 2018 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:webdriver Compat data for WebDriver features. https://developer.mozilla.org/docs/Web/WebDriver HackOnMDNParis2018 Issues or pull requests at the Hack on MDN event in Paris in March 2018
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants