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

Remove type union for tilt and angle properties #562

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Conversation

jrandolf-2
Copy link
Contributor

@jrandolf-2 jrandolf-2 commented Sep 29, 2023

3rd Attempt...

{azimuthAngle:0} matches TiltProperties because it only validates whether tiltX and tiltY exist and defaults them otherwise. This implies AngleProperties is not matched.

This PR implements the TiltProperties and AngleProperties verbatim, according to the Webdriver spec.


Preview | Diff

@jrandolf-2 jrandolf-2 requested review from jgraham and OrKoN September 29, 2023 09:59
@jgraham
Copy link
Member

jgraham commented Sep 29, 2023

I think you want actual prose validating that there isn't a tiltX or tiltY property because currently we pass this into the classic command that will use those properties if they exist, and IIRC we can't specify the absence of properties in CDDL.

@jrandolf-2
Copy link
Contributor Author

I think you want actual prose validating that there isn't a tiltX or tiltY property because currently we pass this into the classic command that will use those properties if they exist, and IIRC we can't specify the absence of properties in CDDL.

@jgraham If tiltX or tiltY doesn't exist, then it will be set to 0. If they do exist, they will be validated and throw if any of them are invalid.

Copy link
Contributor

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

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

LGTM

It looks like the union type meant to denote that either angle or tile properties are provided. So I am not sure if it's compatible with the current spec text. Deferring to @jgraham on that.

index.bs Outdated Show resolved Hide resolved
@jrandolf-2 jrandolf-2 merged commit e0ac302 into main Oct 5, 2023
@jrandolf-2 jrandolf-2 deleted the jrandolf/tilt branch October 5, 2023 11:49
github-actions bot added a commit that referenced this pull request Oct 5, 2023
SHA: e0ac302
Reason: push, by jrandolf

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@whimboo
Copy link
Contributor

whimboo commented Oct 5, 2023

Given that both tiltX and tiltY have been removed the wpt tests need to be updated. @jrandolf can you please fix them? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants