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

10.0-fix-issue-727 #1010

Merged
merged 3 commits into from
Aug 4, 2018
Merged

Conversation

devang-dreambits
Copy link

[FIX] fixed issue for chrome; now, for unsaved data, it displays a dialog of warning...

@devang-dreambits
Copy link
Author

@pedrobaeza Please help me in this PR .. Travis is failing and it doesn't look like my commit is the cause.

@@ -10,6 +10,7 @@ odoo.define('web_confirm_window_close', function (require) {
if ($('html').find('.oe_form_dirty').length) {
e.preventDefault();
}
return 'Your have unsaved data. Are you sure you want to close window?';
Copy link
Member

Choose a reason for hiding this comment

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

Please use translations

Copy link
Author

Choose a reason for hiding this comment

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

@Tardo I came to know about Chrome & Firefox has removed the custom message on window close event...
Chrome
https://www.chromestatus.com/feature/5349061406228480

Firefox
https://developer.mozilla.org/en-US/docs/Web/Events/beforeunload

So shall I add a custom message in return statement? Or returning blank string will be fine...

Copy link
Member

Choose a reason for hiding this comment

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

https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onbeforeunload#Notes

Starting with Firefox 44, Chrome 51, Opera 38 and Safari 9.1, a generic string not under the control of the webpage will be shown instead of the returned string.

I don't know what is the criteria to keep maintaining features that works on outdated browsers.

Any suggestion @OCA/web-maintainers ?

Copy link
Member

Choose a reason for hiding this comment

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

Well, this is a very specific case that can be determined by the maintainers of the module.

Choose a reason for hiding this comment

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

@pedrobaeza @Tardo @OCA/web-maintainers I checked this PR with @devang-dreambits yesterday and what we found was that returning any string is enough to invoke the Confirm Dialog in chrome.
In outdated browsers, both chrome and firefox did show the returned string in the dialog but that it not an requirement.
So wouldn't it be better to return an empty string as it would work well with both new and old browsers and also not get into extra effort of making it translatable as it would be ignored by browsers anyways

Copy link
Member

Choose a reason for hiding this comment

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

OK for me.

@pedrobaeza
Copy link
Member

The error is unrelated to your PR, so you can ignore it.

@@ -10,6 +10,7 @@ odoo.define('web_confirm_window_close', function (require) {
if ($('html').find('.oe_form_dirty').length) {
e.preventDefault();
}
return 'Your have unsaved data. Are you sure you want to close window?';

Choose a reason for hiding this comment

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

As discussed earlier and as @Tardo found here

https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onbeforeunload#Notes

Starting with Firefox 44, Chrome 51, Opera 38 and Safari 9.1, a generic string not under the control of the webpage will be shown instead of the returned string.

Any string returned would be ignored anyways and in older browser string would work just find and display their default generic dialog so better way is it to return blank string.

Please return blank string instead of this custom message

@devang-dreambits
Copy link
Author

@pedrobaeza Thank you for the help.

@pedrobaeza pedrobaeza merged commit fb000f2 into OCA:10.0 Aug 4, 2018
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