GoogleChrome / dialog-polyfill

Polyfill for the HTML dialog element
BSD 3-Clause "New" or "Revised" License
2.45k stars 245 forks source link

Update dialog-polyfill.js to support open event #90

Closed lozandier closed 8 years ago

lozandier commented 8 years ago

When the a dialog's open or showModal event is invoked, trigger an open event to correspond with the close event to comply w/ WHATWG expectations on how the dialog element should work. To not break the Principle of Least Surprise of the existing behavior of the Polyfill, the open event is only fired when show & showModal methods are explicitly used like how the close method currently behaves & differs from using the open attribute explicitly.

Overall Side-effects

googlebot commented 8 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


googlebot commented 8 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


lozandier commented 8 years ago

I signed it!

On Monday, December 28, 2015, googlebot notifications@github.com wrote:

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

[image: :memo:] Please visit https://cla.developers.google.com/ https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll

verify. Thanks.

— Reply to this email directly or view it on GitHub https://github.com/GoogleChrome/dialog-polyfill/pull/90#issuecomment-167618666 .

Kevin Lozandier lozandier@gmail.com lozandier@gmail.com

googlebot commented 8 years ago

CLAs look good, thanks!

googlebot commented 8 years ago

CLAs look good, thanks!

addyosmani commented 8 years ago

Friendly ping @samthor :)

samthor commented 8 years ago

Thanks for the contribution, but this doesn't seem to match the spec. Is there a change or spec update that I'm missing?

lozandier commented 8 years ago

@samthor Ah, reading the spec before, it seems I was too sure to conflate the close event specified by close() ("Queue a task to fire a simple event named close at subject") to the corresponding showModal & open events.

So it seems this can't be merged.

That said, how you recommend suggesting that WHATWG considers this? It seems intuitive in the long-run that such an event canonically exist; what's the W3C mailing list trail that lead for it to be originally omitted? Nonetheless, that's ultimately a tangent.

I'll close this PR if you have no objections.

samthor commented 8 years ago

Closed as not part of spec.