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.

Sign up for GitHub

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

Jump to bottom

Change confirm IDP login webdriver command to click dialog button #510

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Change confirm IDP login webdriver command to click dialog button #510

merged 5 commits into from
Nov 29, 2023

Conversation

tttzach
Copy link
Collaborator

@tttzach tttzach commented Nov 7, 2023 •
edited by pr-preview bot
Loading

Updating the spec to repurpose confirm IDP login into click dialog button. Please take a close look, this is my first edit to the spec.

Preview | Diff

cbiesinger
cbiesinger reviewed Nov 7, 2023
View reviewed changes
Copy link
Collaborator

@cbiesinger cbiesinger left a comment

Choose a reason for hiding this comment

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

Hmm as far as I can tell, the error API is not part of the spec yet. As such, we should probably not reference the error API-specific codes yet here (for example, [=error dialog=] does not go anywhere -- did you get a warning about that when you ran make?)

@tttzach tttzach mentioned this pull request Nov 7, 2023
Merged
@tttzach
Copy link
Collaborator Author

tttzach commented Nov 7, 2023

Can we split the error API parts into a different PR? and land those when the error API is part of the spec? It's a bit difficult to describe ErrorGotIt and ErrorMoreDetails without referencing them.

Also the error codes refer to https://w3c.github.io/webdriver/#dfn-error-code. (Nope no warning on make but [=error dialog=] shows up as regular text without a hyperlink on the index.html)

@cbiesinger
Copy link
Collaborator

Sorry yes, that's what I meant -- remove those parts from this PR and land them together with or after the general error API PR. (I shouldn't have said "codes", I meant ErrorGotIt and ErrorMoreDetails, but I think we're on the same page)

Weird that the [=error dialog=] thing did not show a warning but I think it's best to remove that for now.

cbiesinger
cbiesinger approved these changes Nov 7, 2023
View reviewed changes
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@tttzach
Copy link
Collaborator Author

tttzach commented Nov 7, 2023

I've made the changes, thanks Christian!

@tttzach tttzach requested a review from npm1 November 7, 2023 21:13
@tttzach
Copy link
Collaborator Author

tttzach commented Nov 7, 2023

Requesting Mozilla to take a look @bvandersloot-mozilla @martinthomson @cboozar.

npm1
npm1 approved these changes Nov 8, 2023
View reviewed changes
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 8, 2023
@tttzach
[FedCM] Repurpose ConfirmIdpLogin into ClickDialogButton
e5e42e7 
ConfirmIdpLogin is a webdriver command to click on the "Continue" button
on the FedCM mismatch dialog. To make it more extensible to other
buttons on the FedCM dialog, this patch
1. Renames ConfirmIdpLogin into ClickDialogButton
2. Passes DialogButton as a parameter to ClickDialogButton

In a follow-up patch, we will add support for buttons on the FedCM error
dialog.

Spec PR
w3c-fedid/FedCM#510

WPT PR
web-platform-tests/wpt#43002

Bug: 1499341
Change-Id: Iba12a1581dbe4236525a7d84b5f7ce11b95711f2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5003255
Reviewed-by: Christian Biesinger <[email protected]>
Reviewed-by: Ken Buchanan <[email protected]>
Commit-Queue: Zachary Tan <[email protected]>
Reviewed-by: Dmitry Gozman <[email protected]>
Reviewed-by: David Trainor <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1221852}
T3-M4 pushed a commit to bayandin/chromedriver that referenced this pull request Nov 9, 2023
@tttzach
[FedCM] Repurpose ConfirmIdpLogin into ClickDialogButton
e5471b5 
ConfirmIdpLogin is a webdriver command to click on the "Continue" button
on the FedCM mismatch dialog. To make it more extensible to other
buttons on the FedCM dialog, this patch
1. Renames ConfirmIdpLogin into ClickDialogButton
2. Passes DialogButton as a parameter to ClickDialogButton

In a follow-up patch, we will add support for buttons on the FedCM error
dialog.

Spec PR
w3c-fedid/FedCM#510

WPT PR
web-platform-tests/wpt#43002

Bug: 1499341
Change-Id: Iba12a1581dbe4236525a7d84b5f7ce11b95711f2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5003255
Reviewed-by: Christian Biesinger <[email protected]>
Reviewed-by: Ken Buchanan <[email protected]>
Commit-Queue: Zachary Tan <[email protected]>
Reviewed-by: Dmitry Gozman <[email protected]>
Reviewed-by: David Trainor <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1221852}
@bvandersloot-mozilla
Copy link
Collaborator

This seems reasonable to me!

aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 29, 2023
@tttzach
[FedCM] Add webdriver support for error API
84209f2 
This patch makes the error dialog clickable in WPT tests through the
ClickFedCmDialogButton webdriver command.

Spec PR
w3c-fedid/FedCM#510

WPT PR
web-platform-tests/wpt#43002

Validate-Test-Flakiness: skip
Bug: 1499341
Change-Id: If9289e2ba6b6acb300cc50da74870adc0a3ab612
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018218
Commit-Queue: Zachary Tan <[email protected]>
Reviewed-by: Vladimir Nechaev <[email protected]>
Reviewed-by: Christian Biesinger <[email protected]>
Reviewed-by: danakj <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1230449}
T3-M4 pushed a commit to bayandin/chromedriver that referenced this pull request Nov 29, 2023
@tttzach
[FedCM] Add webdriver support for error API
a9da4a6 
This patch makes the error dialog clickable in WPT tests through the
ClickFedCmDialogButton webdriver command.

Spec PR
w3c-fedid/FedCM#510

WPT PR
web-platform-tests/wpt#43002

Validate-Test-Flakiness: skip
Bug: 1499341
Change-Id: If9289e2ba6b6acb300cc50da74870adc0a3ab612
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018218
Commit-Queue: Zachary Tan <[email protected]>
Reviewed-by: Vladimir Nechaev <[email protected]>
Reviewed-by: Christian Biesinger <[email protected]>
Reviewed-by: danakj <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1230449}
@npm1 npm1 merged commit 75a6d4a into w3c-fedid:main Nov 29, 2023
2 checks passed
github-actions bot added a commit that referenced this pull request Nov 29, 2023
@tttzach @github-actions
Change confirm IDP login webdriver command to click dialog button (#510)
a01e985 
SHA: 75a6d4a
Reason: push, by npm1

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to mattdanielbrown/WebID that referenced this pull request Nov 29, 2023
@tttzach @github-actions
Change confirm IDP login webdriver command to click dialog button (w3…
93e1098 
…c-fedid#510)

SHA: 75a6d4a
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@npm1 npm1 mentioned this pull request Nov 29, 2023
Merged
aarongable pushed a commit to chromium/chromium that referenced this pull request Dec 6, 2023
@tttzach
[FedCM] Remove vendor prefix for click dialog button command
11feba4 
Approved spec PR
w3c-fedid/FedCM#510

Bug: 1499341
Change-Id: Ie5287e999f5ae45bb91a296754f18a68f64522e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5092527
Reviewed-by: Vladimir Nechaev <[email protected]>
Reviewed-by: Christian Biesinger <[email protected]>
Commit-Queue: Zachary Tan <[email protected]>
Reviewed-by: Mathias Bynens <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1233995}
T3-M4 pushed a commit to bayandin/chromedriver that referenced this pull request Dec 7, 2023
@tttzach
[FedCM] Remove vendor prefix for click dialog button command
1de1f0e 
Approved spec PR
w3c-fedid/FedCM#510

Bug: 1499341
Change-Id: Ie5287e999f5ae45bb91a296754f18a68f64522e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5092527
Reviewed-by: Vladimir Nechaev <[email protected]>
Reviewed-by: Christian Biesinger <[email protected]>
Commit-Queue: Zachary Tan <[email protected]>
Reviewed-by: Mathias Bynens <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1233995}
aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 6, 2024
@tttzach
[FedCM] Remove vendor prefix for click dialog button command
e310b2e 
Approved spec PR
w3c-fedid/FedCM#510

(cherry picked from commit 11feba4)

Bug: 1499341
Change-Id: Ie5287e999f5ae45bb91a296754f18a68f64522e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5092527
Reviewed-by: Vladimir Nechaev <[email protected]>
Reviewed-by: Christian Biesinger <[email protected]>
Commit-Queue: Zachary Tan <[email protected]>
Reviewed-by: Mathias Bynens <[email protected]>
Cr-Original-Commit-Position: refs/heads/main@{#1233995}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5102371
Auto-Submit: Zachary Tan <[email protected]>
Cr-Commit-Position: refs/branch-heads/6167@{#105}
Cr-Branched-From: 222e786-refs/heads/main@{#1233107}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbiesinger cbiesinger cbiesinger approved these changes

@npm1 npm1 npm1 approved these changes

@bvandersloot-mozilla bvandersloot-mozilla Awaiting requested review from bvandersloot-mozilla

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tttzach @cbiesinger @bvandersloot-mozilla @npm1