-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
adding all the other WebDriver commands #1465
Conversation
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. |
There was a problem hiding this 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.
webdriver/commands/Accept_Alert.json
Outdated
"version_added": false | ||
}, | ||
"ie": { | ||
"version_added": "6" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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).
- 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
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! |
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 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+
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.