New issue
Advanced search Search tips

Issue 652017 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: 2017-11-10
OS: Linux , Windows , Chrome
Pri: 1
Type: Bug
Launch-M-Target: 64-Beta

Blocking:
issue 630357


Participants' hotlists:
Harmony-Ready-For-Review

Show other hotlists

Other hotlists containing this issue:
HarmonyFutureP1s


Sign in to add a comment

Harmony - update Reload confirmation dialog

Project Member Reported by shrike@chromium.org, Oct 1 2016

Issue description

Comment 1 by shrike@chromium.org, Oct 11 2016

Owner: kylixrd@chromium.org

Comment 2 by shrike@chromium.org, Dec 14 2016

If you add a screenshot of this dialog as it exists now I can list the exact changes that need to be made to Harmonize it.
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dcc44ff52e9de6af0034d5b996b8047fab1a972b

commit dcc44ff52e9de6af0034d5b996b8047fab1a972b
Author: Allen Bauer <kylixrd@chromium.org>
Date: Thu Jul 06 18:19:59 2017

Remove close (X) from javascript unload and reload dialogs.

Bug:  652017 
Bug:  652015 
Change-Id: I61970a2755ac95d9b19cf648422dce5585ec427c
Reviewed-on: https://chromium-review.googlesource.com/557941
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484675}
[modify] https://crrev.com/dcc44ff52e9de6af0034d5b996b8047fab1a972b/components/app_modal/views/javascript_app_modal_dialog_views.cc
[modify] https://crrev.com/dcc44ff52e9de6af0034d5b996b8047fab1a972b/components/app_modal/views/javascript_app_modal_dialog_views.h

> Remove close (X) from javascript unload and reload dialogs.

We have a lot of dialogs to remove this from :)

I plan to expose a views::LayoutProvider

  // Returns whether non-bubble dialogs should have a close [x] by default.
  virtual bool ShouldShowCloseXOnDialog() const;

context: https://chromium-review.googlesource.com/c/559205/
I don't think that's the right answer.  For one, I don't think we should gate changes here on Harmony -- we should just change this immediately.  For another, whether there is a close X is the same question as what the modality of the dialog is.  There should be one function that dictates both modality and close button presence.  I think there's a separate bug covering that latter idea.

In the meantime, overriding the existing ShouldShowCloseButton() method per-dialog seems better to me.
Labels: -M-56
Owner: ----
Status: Available (was: Assigned)
Description: Show this description
Cc: tapted@chromium.org ellyjo...@chromium.org
This is still an app-modal NSAlert on Mac.  Issue 728143  explains why that was necessary for OnBeforeUnload. I'm not sure whether the same applies for reload confirmation.

The mock doesn't have equal-sized buttons, but I'm assuming we want that.

Otherwise, on Windows, we need to make the secondary text grey and fix the general-dialog-spacing-thingo.
Screen Shot 2017-08-09 at 11.49.44 am.png
33.3 KB View Download
Screen Shot 2017-08-09 at 11.53.38 am.png
13.4 KB View Download
Also the dialog title has a heavier weight in the mock than currently?
Labels: Launch-M-Target-64-Beta
NextAction: 2017-11-10

Comment 14 by bsep@chromium.org, Oct 6 2017

Labels: -Pri-2 Pri-1

Comment 15 by bsep@chromium.org, Oct 20 2017

Cc: -tapted@chromium.org
Owner: tapted@chromium.org
Status: Assigned (was: Available)
Load balancing
Cc: tapted@chromium.org
Owner: pbos@chromium.org
Bouncing to pbos while I'm buried under some mac-specific stuff
Project Member

Comment 17 by bugdroid1@chromium.org, Nov 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6314cbc42d31aaf3cadd302cfb0ec31dfa7f0578

commit 6314cbc42d31aaf3cadd302cfb0ec31dfa7f0578
Author: Peter Boström <pbos@chromium.org>
Date: Fri Nov 10 00:46:57 2017

Style views::MessageBoxView for Harmony.

Introduces CONTEXT_MESSAGE_BOX_BODY_TEXT and styles it like
CONTEXT_BODY_TEXT_LARGE and STYLE_SECONDARY under Harmony. This
specifically styles the JavaScript dialogs.

Bug:  chromium:652015 ,  chromium:652017 ,  chromium:683346 
Change-Id: I846c33736cfffb368105ea1cd49c2a8079e20b14
Reviewed-on: https://chromium-review.googlesource.com/759409
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515378}
[modify] https://crrev.com/6314cbc42d31aaf3cadd302cfb0ec31dfa7f0578/chrome/browser/ui/views/harmony/harmony_typography_provider.cc
[modify] https://crrev.com/6314cbc42d31aaf3cadd302cfb0ec31dfa7f0578/ui/views/controls/message_box_view.cc
[modify] https://crrev.com/6314cbc42d31aaf3cadd302cfb0ec31dfa7f0578/ui/views/style/typography.h

The NextAction date has arrived: 2017-11-10
Project Member

Comment 19 by bugdroid1@chromium.org, Nov 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/977ebe428e23759cdcdb5ea7e342f210604c17f0

commit 977ebe428e23759cdcdb5ea7e342f210604c17f0
Author: Peter Boström <pbos@chromium.org>
Date: Fri Nov 17 03:18:22 2017

Add conditional titlecase to JavaScript dialogs.

Adds titlecase title strings for window.prompt, alert, on-reload
and on-close dialogs.

Bug:  chromium:652015 ,  chromium:652017 ,  chromium:683346 
Change-Id: I197a8ad11d95d0d4f7671bba81f671d8c45206c2
Reviewed-on: https://chromium-review.googlesource.com/773907
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517271}
[modify] https://crrev.com/977ebe428e23759cdcdb5ea7e342f210604c17f0/components/app_modal_strings.grdp

Comment 20 by pbos@chromium.org, Nov 17 2017

Cc: pbos@chromium.org
Owner: bettes@chromium.org

Comment 21 by pbos@chromium.org, Nov 17 2017

Attaching screenshot @ 150% HDPI on Windows 10. The checkbox only shows up on repeat dialog invocations.
javascript-onbeforeunload-reload-150hdpi.png
159 KB View Download
Cc: -pbos@chromium.org
Owner: pbos@chromium.org
- Replace 'Don't reload' with 'Cancel'

Title casing button strings should only be on MacOS. All other platforms should use sentence casing (Don't reload). If there's a problem in making per-platform specifications in this realm, let me know and we can work towards simplifying the spec. 


Project Member

Comment 23 by bugdroid1@chromium.org, Dec 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/077959a33ff19709c8972d9c0549bdfea605a21a

commit 077959a33ff19709c8972d9c0549bdfea605a21a
Author: Peter Boström <pbos@chromium.org>
Date: Sat Dec 23 05:33:12 2017

Harmony fixes for the JavaScript app-modal dialog.

* Removes punctuation from the prevent-additional-dialogs checkbox.
* Removes the special cancel button titles in favor of a generic cancel
  button.

Bug:  chromium:652015 ,  chromium:652017 
Change-Id: Ib2d997ed7d20b8a1acb10056874b38f1b2480eac
Reviewed-on: https://chromium-review.googlesource.com/843129
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526140}
[modify] https://crrev.com/077959a33ff19709c8972d9c0549bdfea605a21a/chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm
[modify] https://crrev.com/077959a33ff19709c8972d9c0549bdfea605a21a/components/app_modal/views/javascript_app_modal_dialog_views.cc
[modify] https://crrev.com/077959a33ff19709c8972d9c0549bdfea605a21a/components/app_modal_strings.grdp

Comment 24 by pbos@chromium.org, Dec 23 2017

Status: Fixed (was: Assigned)

Sign in to add a comment