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

fix: close request if sheet is showing #821

Merged
merged 3 commits into from
Jan 17, 2019
Merged

fix: close request if sheet is showing #821

merged 3 commits into from
Jan 17, 2019

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Jan 11, 2019

closes #820

The following tasks have been completed:

  • Confirmed there are no ReSpec errors/warnings.
  • Modified Web platform tests
  • Modified MDN Docs - not needed.
  • Has undergone security/privacy review - bug fix. Not needed.

Implementation commitment:

  • Safari - @aestes, please provide link 🙏.
  • Chrome - current behavior.
  • Firefox - current behavior.
  • Edge - @zouhir - please confirm 🙏.

Optional, impact on Payment Handler spec?
None


Preview | Diff

@marcoscaceres
Copy link
Member Author

@zouhir, not sure if you can confirm this yet, but I guess Edge going forward will rely on Blink's implementation of Payment Request but still use Microsoft Wallet for payments. Is that a valid assumption? If yes, then I can just assume whatever is in Blink will work in Edge, in as far as the API mechanics are concerned (then I don't need to bug you to confirm things 😇 ).

@marcoscaceres
Copy link
Member Author

Added test:
web-platform-tests/wpt@5f1cbd0

@rsolomakhin
Copy link
Collaborator

assume whatever is in Blink will work in Edge.

May or may not, depending on where Chromium ends and Edge begins. Blink is a very thin layer that mostly forwards calls to the browser process, where most of the logic is implemented. The reason for that is we trust the renderer process as little as possible, since it parses and runs arbitrary HTML/JS from the web.

@zouhir
Copy link
Collaborator

zouhir commented Jan 15, 2019

Thanks @marcoscaceres for checking with me - it is hard for me to confirm at the moment but the scnario you described is what I personally think is ideal for now. generally speaking there is no intent to diverge. I believe in the following couple weeks I will have better comment on this 😊.

@marcoscaceres
Copy link
Member Author

Thanks @zouhir and @rsolomakhin for the details.

I'll wait a few days for @aestes to confirm it's ok to make this small change, and there is not some obvious use case we might have overlooked.

@marcoscaceres
Copy link
Member Author

Going to move forward with this. Can revise if @aestes raises any concerns.

@marcoscaceres marcoscaceres merged commit 5f977ed into gh-pages Jan 17, 2019
@marcoscaceres marcoscaceres deleted the show_closes branch January 17, 2019 04:34
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.

Clarification question for show() algorithm
3 participants