Harmony - update Page unresponsive dialog |
|||||||||||||
Issue descriptionNote that the mock shows one button on the left and another on the right of the dialog. Per bettes@, both buttons should be placed next to each other on the right side. Mock: https://docs.google.com/presentation/d/1NCYvxQ8VWuDDIK1uTiOL6zXQ1H0fJEoKxTivG8NIFVg/edit#slide=id.g164c64c35c_0_0
,
Jan 5 2017
The image is 100x100, and the list of hung tabs is based on the size of the text. Should I force those to be a multiple of 16, or is this okay?
,
Jan 6 2017
Sorry for the delay in responding, and thank you for your work on this. There are 3 "default" dialog widths to choose from, but I see that it's already wider than the widest default width. And you also mention that it gets wider to accommodate the titles, and has the very large icon. I think just leave the dialog size and icon as they are for now, and we'll get UX to decide how we should proceed. A few tweaks: * The distance between the buttons should be 8pt, ignoring the light blue focus ring. Right now it looks like its 8pt from the left button's right edge to the outside of the focus ring. * The rightmost button should be 16pt from the right and bottom edges of the dialog (again ignoring the focus ring) * The scroll border should also be 16pt from the right edge of the dialog * The buttons should be 70pt wide * The dialog title should be 8pt from the left and top edges of the dialog * The center of the close button should be 16pt from the right and top edges of the dialog
,
Jan 14 2017
I have revised the dialog, I think it is correct now. A lot of the points in #4 were due to me taking the screenshot at 1.5x and so weren't actually incorrect. But this one is at 1x. Assigning to bettes@ for review.
,
Jan 15 2017
Looking good. Seems like the buttons are too tall (should be 28 but looks like it's 36). What happened to the bottom edge of the scroll border? And do you know why there's a bullet next to the page title? bettes@ - is the plan to continue using this icon?
,
Jan 17 2017
Assigning back to bsep@ to address shrike's feedback in #7. FYI, the fate of the icon is TBD. When you assign this back to me for review, can you include information on what steps users would take before getting this dialog? And if it's seen on all platforms. Thank you!
,
Jan 17 2017
I suspect both the buttons being too tall and the bottom scroll border missing are the result of me using force-device-scale-factor=1 on a 1.5x system. I'll confirm later. A user shouldn't see this in normal circumstances. To trigger this dialog for testing you can navigate to about:hang and wait.
,
Jan 28 2017
Can you make sure your CL adds the dialog to the dialog browser test framework for easy triggering? I'm not sure why there's a bullet in the name of the page title either, but that should probably be tackled separately from this bug. (Feel free to file, though) Is this using the 448 pt. width? It looks close to 450, I'm not sure whether I'm supposed to be including the dialog borders/edges. It looks to me like there's too much space between the listbox and the bottom button row, how much is there? It also looks like there's too much padding above the dialog title. I assume we don't care in this CL about things like the dialog shadow looking pretty gross (not the smooth fadeout in the spec) or the precise vertical anchoring position of this dialog (not sure what the current spec is for how modal dialogs should be anchored), but only about the content inside the dialog.
,
Jan 28 2017
The bullet makes more sense when there are multiple hung pages (since it's a listbox) but I bet that's an exceptionally rare case. But we can worry about it later. I think the border isn't included in the width. At any rate it ought to Just Work, it's not controlled by individual dialogs. There's 2 units between the listbox and buttons, it looks like because it's double-applying the button top padding. I fixed that and attached a screenshot of what it looks like without. I think it looks a little better. I triple-checked the dialog title and I think it's actually right. The label is 28px and the top border is 16px. It's easier to tell if you change the dialog to u2588, the bottom of the rectangle is 44px from the top. The label takes up a little extra room above the actual text (probably to handle ascenders) but it just appears as whitespace. I don't know what (if anything) to do about that. I think the dialog shadow is actually really hard to change. But no I didn't look at in for this CL. I also think the dialog is anchored correctly. The spec says it should be 1px below the omnibox, which it is. Unless that's not supposed to count the border, the spec is slightly ambiguous. But that's global for all dialogs so I don't have to change it for this CL.
,
Jan 31 2017
Working on adding it to the browser test framework, then going to submit this as-is, unless there's any more feedback.
,
Feb 2 2017
For the title thing, the "UI with titles" spec is confusing (see https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-03b-dialog-keylines.png%3Fz=width ); it says in text that it's supposed to be 12 pt above, and in the image that it's supposed to be 14. Neither one of these is 16. The title font itself is supposed to be "Regular 15 pt, leading 22 pt" ( https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-01-typography.png%3Fz=width ). Both of those sound shorter than a 28 px label, but I admit I'm not good with font metrics, so maybe I'm wrong? I think bettes@ needs to clarify some of this. For dialog anchoring, it looks like most global dialogs are anchored such that their top border overlaps the omnibox bottom border: https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Preview#%2FP%20-%20javascript_01.png%3Fz=width
,
Feb 2 2017
Right, it's confusing because even if the font size is Npt, that doesn't mean the label area is going to be Npx tall because I don't think N counts ascenders. I did realize that I made the 28px measurement when using force-device-scale-factor=1 which means it's bogus. I remeasured, the label height is 20 dips and the actual size of the text is 14 dips, which at least matches one of the spec numbers you threw out. I can't figure out where any of these numbers actually come from, however.
,
Feb 2 2017
14/20 sounds like a weird place that's neither "Title" nor "Body 1" typography. My suspicion, honestly, is that all our font sizes are noncompliant with the Harmony spec on at least Windows. I found a very sketchy piece of code someone already landed to bump the font size by 1 in the Harmony case in a random dialog, which is exactly the sort of one-off fix I don't want to do. The "14" number in comment 14 is a padding value above the label, rather than a size of the label itself. I'm not sure what it means when you say you don't know where the numbers come from. You mean you don't know which part of the code is responsible for the final size results seen here?
,
Feb 2 2017
Yes, I tried to trace where the number 20 was generated from and I got lost in the text rendering subsystem. It seems like there's some kind of list of "correct" font sizes that's generated somewhere, so yes it's definitely wrong in lots of places and not just in this dialog.
,
Feb 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b621d8cec36bec73ef3eacddbcd7c998aee80cae commit b621d8cec36bec73ef3eacddbcd7c998aee80cae Author: bsep <bsep@chromium.org> Date: Sat Feb 18 22:40:00 2017 Harmony - convert hung renderer dialog. Also added the dialog to BrowserDialogTest. Also did some refactoring on DialogClientView to avoid hardcoding the minimum button width and to apply the button row insets more coherently. BUG= 652507 Review-Url: https://codereview.chromium.org/2660553005 Cr-Commit-Position: refs/heads/master@{#451497} [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/chrome/browser/ui/views/chrome_views_delegate.cc [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/chrome/browser/ui/views/chrome_views_delegate.h [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/chrome/browser/ui/views/harmony/harmony_layout_delegate.cc [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/chrome/browser/ui/views/harmony/layout_delegate.cc [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/chrome/browser/ui/views/harmony/layout_delegate.h [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/chrome/browser/ui/views/hung_renderer_view.cc [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/chrome/browser/ui/views/hung_renderer_view.h [add] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/chrome/browser/ui/views/hung_renderer_view_browsertest.cc [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/chrome/test/BUILD.gn [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/ui/views/controls/button/md_text_button.cc [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/ui/views/layout/layout_constants.h [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/ui/views/views_delegate.cc [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/ui/views/views_delegate.h [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/ui/views/window/dialog_client_view.cc [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/ui/views/window/dialog_client_view.h [modify] https://crrev.com/b621d8cec36bec73ef3eacddbcd7c998aee80cae/ui/views/window/dialog_client_view_unittest.cc
,
Feb 18 2017
,
Feb 20 2017
Please don't mark Harmony dialog update bugs "Fixed" until they've gone through the formal review + signoff process.
,
Feb 22 2017
Can you add a screenshot of the dialog in its current state?
,
Feb 22 2017
If you know these are problems you should go ahead and fix them? Basically the designer will ask for those fixes? Other issue is the bullet next to the page name - I don't think that should be there.
,
Feb 22 2017
They aren't issues specific to this dialog. tapted@ is working on both of those right now. As far as the bullet goes, #8 said we're not sure what we're doing with it yet, so I haven't bothered revisiting it. In any case it's also not specific to this dialog.
,
Feb 22 2017
Got it.
,
Jun 1 2017
UX review notes: HungRendererDialogView specific - remove illustration from codebase - update button text "Kill" to "Exit page" - use checkmark for selection (https://icons.googleplex.com/#icon=ic_done&search=check) General - for modal (centered dialogs), there should never be a close-x button - all of our modal dialogs are apparently draggable now by click-and-drag or long-press (?!?!) - the title and body text are still at #000 at 100% - the button text weight is Bold, not Semibold I've created a master folder to hold all the screenshots as we go along. https://drive.google.com/open?id=0BxMIIGI80eU-TEk4SmZKX3g3bnc HungRendererDialogView Spec (titled Page unresponsive) https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec/_html#%3Fz=width&f=hidden
,
Jul 14 2017
Random drive-by question: "- use checkmark for selection (https://icons.googleplex.com/#icon=ic_done&search=check)" What selection? There is no selection here. This dialog is shown when a render process hangs. There can be more than one tab that is operated by a render process, so in this dialog is a list box listing the tabs that are hung because the render process is hung. There is no selection. Either you wait until the render process (and all of its tabs) recovers, or you kill the render process and all the tabs within it. There is no selection involved. It's all the tabs or none of them.
,
Jul 14 2017
#27: This was cleared up outside of the bug. There was a mistaken impression that the table view was for selection. You are allowed to click on the hung tabs and it will highlight them, which looks like you're selecting them. The UX is confusing that way.
,
Aug 9 2017
,
Aug 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9dc9455fcce8f715889354169f67bc8cb3930e57 commit 9dc9455fcce8f715889354169f67bc8cb3930e57 Author: Bret Sepulveda <bsep@chromium.org> Date: Tue Aug 22 03:07:20 2017 Harmony - Convert hung renderer dialog for the new spec. This patch removes the "frozen tab" image, changes the language of the explanatory label and buttons, and removes the bullets from the table of hung tabs. This patch also changes the layout of TableView to use metrics instead of constants and fixes the line height of Windows body fonts to be 20pt (instead of 21). TBR=grt@chromium.org Bug: 652507 Change-Id: I3a3e08d915ecc8b3ac9089a5867468dfe8cc42eb Reviewed-on: https://chromium-review.googlesource.com/571199 Reviewed-by: Bret Sepulveda <bsep@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Reviewed-by: Trent Apted <tapted@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Commit-Queue: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#496193} [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/app/generated_resources.grd [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/conflicting_module_view_win.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/harmony/chrome_layout_provider.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/harmony/chrome_layout_provider.h [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/harmony/harmony_layout_provider.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/harmony/harmony_typography_provider.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/harmony/harmony_typography_provider.h [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/harmony/layout_provider_unittest.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/harmony/textfield_layout.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/hung_renderer_view.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/hung_renderer_view.h [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/infobars/confirm_infobar.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/passwords/credentials_item_view.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/passwords/credentials_selection_view.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/chrome/browser/ui/views/translate/translate_bubble_view.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/ui/gfx/platform_font_win.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/ui/gfx/platform_font_win.h [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/ui/views/controls/table/table_view.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/ui/views/controls/table/table_view.h [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/ui/views/layout/layout_provider.cc [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/ui/views/layout/layout_provider.h [modify] https://crrev.com/9dc9455fcce8f715889354169f67bc8cb3930e57/ui/views/style/typography.h
,
Sep 5 2017
,
Sep 5 2017
,
Nov 7 2017
The button text "Exit page" doesn't make sense when it's multiple pages.
,
Nov 10 2017
The NextAction date has arrived: 2017-11-10
,
Jan 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b542a405daf4fcee68a1799ff0dc99b056f3f645 commit b542a405daf4fcee68a1799ff0dc99b056f3f645 Author: Bret Sepulveda <bsep@chromium.org> Date: Wed Jan 17 02:28:40 2018 Add >1 page case to Hung Renderer UI tests and fix button label. The Hung Renderer UI tests weren't compiling and were missing a test case for multiple hung pages, this patch fixes both. Also, the "Exit Page" button was updated to read "Exit Pages" when appropriate. Bug: 652507 Change-Id: I794bc5bc7cd0bd2a1cddb746bd679ca02e2c1694 Reviewed-on: https://chromium-review.googlesource.com/854689 Commit-Queue: Bret Sepulveda <bsep@chromium.org> Reviewed-by: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#529549} [modify] https://crrev.com/b542a405daf4fcee68a1799ff0dc99b056f3f645/chrome/app/generated_resources.grd [modify] https://crrev.com/b542a405daf4fcee68a1799ff0dc99b056f3f645/chrome/browser/ui/views/hung_renderer_view.cc [modify] https://crrev.com/b542a405daf4fcee68a1799ff0dc99b056f3f645/chrome/browser/ui/views/hung_renderer_view.h [modify] https://crrev.com/b542a405daf4fcee68a1799ff0dc99b056f3f645/chrome/browser/ui/views/hung_renderer_view_browsertest.cc [modify] https://crrev.com/b542a405daf4fcee68a1799ff0dc99b056f3f645/chrome/test/BUILD.gn
,
Jan 24 2018
The button string is used in the Mac version of the dialog but the Mac code was not updated along with the string.
,
Jan 24 2018
#36: I got a separate bug for it, see bug 803855
,
Jul 25
This is as much work as we're going to do for Harmony specifically; we should file a new bug for further improvements. |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-03b-dialog-keylines.png%3Fz=width){kind=link}
/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-01-typography.png%3Fz=width){kind=link}
/Secondary%20UI%20Previews%20and%20specs%20(exports)/Preview#%2FP%20-%20javascript_01.png%3Fz=width){kind=link}
Comment 1 by shrike@chromium.org
, Oct 11 2016