-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
added get dom property and get dom attribute method information #2529
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
Open
rpallavisharma
wants to merge
7
commits into
trunk
Choose a base branch
from
2493DomAttribute
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
c960ee9
added get dom property and get dom attribute method information
rpallavisharma b64fb0e
Merge branch 'trunk' into 2493DomAttribute
rpallavisharma 150d9d9
Merge branch 'trunk' into 2493DomAttribute
rpallavisharma 108047b
Merge branch 'trunk' into 2493DomAttribute
rpallavisharma 662cac6
Merge branch 'trunk' into 2493DomAttribute
rpallavisharma 14a2ad7
Merge branch 'trunk' into 2493DomAttribute
harsha509 c93e12b
Merge branch 'trunk' into 2493DomAttribute
rpallavisharma File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
May be we need to be more precise now with the difeerences between
getAtrribute,getDomAttributeandgetDomPropertyexample:
Returns the element's property value if present, otherwise falls back to the attribute value. This convenience method handles the common case but can be ambiguous. For precise control, use getDomProperty() or getDomAttribute() instead.@diemol any thoughts !
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.
We are showing in here how to use it. Let me know if any changes are needed at code level?
Uh oh!
There was an error while loading. Please reload this page.
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.
@harsha509 getAttribute() is actually deprecated for just that reason.
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.
Including a synopsis of this information would be helpful: What's the difference between an "attribute" and a "property" in the DOM? | Quiz Interview Questions with Solutions https://share.google/lB3bUx9KDK28LOx9B
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.
Hi @sbabcoc,
I’m already aware of the distinction, this is exactly why I asked for the documentation to clearly outline the difference in a way users can understand.
Also
getAtrributeis not deprecated yet in selenium see https://github.com/SeleniumHQ/selenium/blob/trunk/java/CHANGELOGThere 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 am unable to understand the discussion and reasoning around the new information added about new methods at document and code level, and it seems to me i am repeating myself again. if this PR isn't correct then don't add it. i don't know what to do in here further. im finding this exhausting.
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 is the regular process of contributing to a project to send a PR. Sometimes people don't understand the context. They ask questions, change the context, and then follow up by asking more questions. Some PRs stay open and in discussions for weeks, sometimes months. It's not ideal, but that's just the way it is.
Sometimes, it's very quick. Sometimes it takes a bit longer. I'm very happy that you're taking the time to contribute. I hope you understand that sometimes it doesn't go through at the first try. You get feedback, and then we iterate on it. It's not perfect, but it's a collaborative process. This fosters a sense of working as a team, and without feedback, there would be no point in open source.
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.
Hi @rpallavisharma ,
There is nothing wrong in this PR and I totally understand the intention behind the PR and appreciate the additions.
My point is that introducing
getDomAttribute()andgetDomProperty()without first clarifying the ambiguity ofgetAttribute()may confuse users even more.The docs should reflect the real behavior of
getAtrribute, since ambiguity affects users in practice.So, as a final suggestion from my review, please update the
getAttribute()explanation to something like:getAttribute Returns the element's property value if present, otherwise falls back to the attribute value. This convenience method handles the common case but can be ambiguous. For precise control, use getDomProperty() or getDomAttribute() instead.Once that is clearly stated, the existing code example can be adjusted accordingly, and we won’t need separate sections for
getDomAttribute()andgetDomProperty().Also, here is the additional info, regarding the ambiguity for your reference:
Hope this clarifies everything!
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.
@harsha509 you don't want to have section for the new commands added separately in document?
Only get attribute which already exists should be there? And modified to what you wrote?
Then the code for the new commands where does that go?
In that case this PR doesn't do it.
It added information for new commands with code examples.
@diemol sure we can continue this discussion , as much as necessary. Fine with me.
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 are three methods get attribute get dom property get dom attribute . these are three different methods. we kept get attribute for backward compatibility, and added two new.
i created that PR to showcase two new and code examples for it. didn't change get attribute.
this blog - https://saucelabs.com/resources/blog/selenium-4-new-element-attribute-and-property-methods
the problem now is that you are saying we update get attribute definition as well? or we add the information of these two new methods in same method?