Harmony - update Find-in-page dialog |
||||||||||||||||||
Issue description
,
Sep 30 2016
,
Oct 1 2016
,
Oct 11 2016
,
Jan 24 2017
Unassigning my Harmony bugs pending re-triage of who should own what.
,
Jun 29 2017
,
Jun 29 2017
This is going to be a rather complicated one to start with. For the overall look, see the mock link in comment 1. As you noted in IM, we need to change the string from "x of y" to "x/y". We also (I think; check with bettes) need to add an optional focus ring around the textfield when it has been given explicit keyboard focus with tab. For layout, see the "toasts" portions of the spec at https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-07a-layout.png , though that only covers the edge padding. The spacing between the icons should be using the RELATED_CONTROL_HORIZONTAL distance, I think. It's not clear to me how wide we're supposed to make the textfield; I would expect "some number of n-widths, rounded to the next harmony layout unit". Don't know exactly how wide to make the "1/1" indicator; see what the current code does. There's also a behavior change here where the find dialog is reparented to a new omnibox icon. We'd need to add the icon itself, the plumbing in the code, and change the anchoring and the show/hide animations to match other omnibox-parented things. This needs to be tested in fullscreen mode as well. All in all I'd expect to see several different CLs involved in getting this one up to spec. On the plus side, once you finish with this, you ought to have a pretty solid feel for a lot of the different challenges involved in harmonizing dialogs.
,
Jul 13 2017
Attached snaps of the mock which shows 16 px insets from the top to the text field. The other png is the regular mock without any measurements. It appears to show the separator as equidistant from the match count field and the button on the right
,
Jul 13 2017
Ignore the mock insets; the layout rules for toasts supersede that. The separator is equidistant with and without your patch, but it's twice as far (on both sides) with the extra margins you added.
,
Jul 13 2017
If we find a way to lay out without taking the vector icon button enlargements into account, it will bring the button to the right of the separator 4 px closer, which will look much more balanced.
,
Jul 13 2017
Did you do those insets on every side? It looks like everything has gotten even farther apart horizontally. Also things still seem to have too much vertical margin. Would be interested to see the bounds rects or something similar to show why the layout manager is allocating so much space.
,
Jul 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a5f3fa6e02a96926c01d20fbf38be2257c251d49 commit a5f3fa6e02a96926c01d20fbf38be2257c251d49 Author: ananta <ananta@chromium.org> Date: Tue Jul 18 00:19:20 2017 Harmonize the find in page dialog. This patch changes the insets and control spacing and the match count text We space the controls in the find bar using margins to ensure that the distances are computed to the vector image glyphs and not to the border. Next steps would be to tackle the rest of the items in this dialog for harmony BUG= 651643 TBR=blundell Review-Url: https://codereview.chromium.org/2968713003 Cr-Commit-Position: refs/heads/master@{#487316} [modify] https://crrev.com/a5f3fa6e02a96926c01d20fbf38be2257c251d49/chrome/browser/ui/find_bar/find_bar_host_browsertest.cc [modify] https://crrev.com/a5f3fa6e02a96926c01d20fbf38be2257c251d49/chrome/browser/ui/views/find_bar_view.cc [modify] https://crrev.com/a5f3fa6e02a96926c01d20fbf38be2257c251d49/chrome/browser/ui/views/harmony/chrome_layout_provider.cc [modify] https://crrev.com/a5f3fa6e02a96926c01d20fbf38be2257c251d49/chrome/browser/ui/views/harmony/chrome_layout_provider.h [modify] https://crrev.com/a5f3fa6e02a96926c01d20fbf38be2257c251d49/chrome/browser/ui/views/harmony/harmony_layout_provider.cc [modify] https://crrev.com/a5f3fa6e02a96926c01d20fbf38be2257c251d49/components/find_in_page_strings.grdp
,
Aug 9 2017
,
Aug 10 2017
,
Sep 5 2017
,
Sep 18 2017
Re-assigning this to pkasting@
,
Sep 20 2017
+bsep@ Can we re-assign this bug?
,
Sep 20 2017
,
Sep 22 2017
,
Sep 22 2017
I'm not sure what else needs to be done here. The comment in the CL is vague.
,
Sep 23 2017
If this isn't ready for review, please remove it from the hotlist
,
Sep 23 2017
For this one, bsep@ and I synced with pkasting@ few days ago. The overall implementation seems to be complete; however, there were some finer points (listed in comment#7) that pkasting@ was worried about and it's unsure if those are fully implemented.
,
Sep 25 2017
Is adding the new omnibox icon and other requisite changes necessary for the first Harmony phase? This could take a while to get the implementation nailed down.
,
Sep 25 2017
kylixrd@ - does the following rule answer your question and make sense? - Omnibox icon visible: when the popover is open - Omnibox icon NOT visible: when the popover is closed
,
Sep 25 2017
hwi@ - Yes, that makes it clear. AFAIK, this is the only instance where an icon in the OmniBox behaves in that manner. In the other instances, the icon is visible (and clickable) before the popover is shown. What happens when the user clicks the icon? Does the icon disappear once the popover is dismissed? Does the view always show next to the "star" icon in cases where there may be other icons visible? For instance, when "mixed-script" shield icon is visible, should the "find" icon be between the "shield" icon and the "star" icon?
,
Sep 25 2017
What happens when the user clicks the icon? - (I've made up this rule just now) Nothing happens when the icon is clicked. Find-in-page is one of the rare case of "sticky" popover, which dismiss upon explicit "close [x]" or "[Esc] when the popover is focused". - This rule can prevent an accidental dismissal while the UI is still in use, and, doesn't add unnecessary clutter to the omnibox when it's not summoned. Does the icon disappear once the popover is dismissed? - Yes. Does the view always show next to the "star" icon in cases where there may be other icons visible? For instance, when "mixed-script" shield icon is visible, should the "find" icon be between the "shield" icon and the "star" icon? - Always next to the bookmark star sounds right to me. - FYI, the following is the current order of the omnibox icons: - content settings (order within the content settings: https://cs.chromium.org/chromium/src/chrome/browser/ui/content_settings/content_setting_image_model.cc?rcl=252600a1fd20c6fdf104c63e2f50272b94270388&l=141) - zoom - passwords - save credit card - bookmark star
,
Sep 26 2017
hwi@ - Thanks! Now, where shall I obtain the find vector-icon? It will need to be in a format from which I'll be able to convert to a skia vector image.
,
Sep 26 2017
re: icon on c34 This needs bettes@'s attention. The icon is originated from https://icons.googleplex.com/#search=find%20in%20page. However, we typically make some spacing/sizing adjustments to visually balance with other omnibox icons. Also, we need to determine if OSX needs a bitmap version or not. So please wait to clarify all these. bettes@ knows the current status of all things icons. Thanks!
,
Sep 29 2017
The NextAction date has arrived: 2017-09-29
,
Sep 29 2017
Next question for the UX folks: Where does the find-in-page dialog go for RTL languages? The code will currently place it over to the left edge of the browser window without regard for the OmniBox location. Should it line up with the left edge of the OmniBox in the same manner is the right edge does for LTR languages?
,
Sep 29 2017
>> Should it line up with the left edge of the OmniBox in the same manner is the right edge does for LTR languages? Yes. A mock and an example from password popover attached attached. Note: in terms of omnibox icon flipping, I need to research more on the icon direction guide since some icons (key icon for password) are flipped but some (magnifying glass to on the omnibox search state) currently on chrome.
,
Sep 29 2017
c39 (finishing the sentence): *but some (magnifying glass to on the omnibox search state) *are not flipped* currently on chrome.
,
Oct 2 2017
Thanks, hwi@! You're awesome!
,
Oct 2 2017
Another little tidbit. If the OmniBox is hidden, by policy or some other reason, it reverts to the previous positioning logic.
,
Oct 2 2017
Thanks kylixrd@ for the screenshot and the note on the omnibox+position behavior. re: icon for 'find-in-page' Non-flipping/mirroring is the way to go for this case per the 'search icon' guidance from the MD spec: https://material.io/guidelines/usability/bidirectionality.html#bidirectionality-rtl-mirroring-guidelines Note: we need to look into other part of the UIs but it's not scope of this bug. I've been doing a separate RTL audit. re: omnibox+position I'll also look into this during my RTL audit.
,
Oct 3 2017
Thanks a lot, kylixrd@! I think this seems ready for bettes@ to review.
,
Oct 5 2017
There is currently some push-back in the CL regarding the behavior of the find bar. It's moved into a position that, currently, mimics the look of other "bubble" views attached to icons in the OmniBox. However, those views behave a little differently than the find bar view. The find bar uses a slide animation to show/hide, whereas the other bubbles appear to use a fade-in/out animation. The find bar can only be dismissed by pressing escape (while it is focused) or pressing the close(X) button. Bubbles can also be dismissed by losing activation or clicking on the OmniBox icon. The idea here is to make the find-bar an actual bubble using the same internal infrastructure. This brings with it some different inherent behaviors. I've been investigating this approach, and it's possible, but will require some more changes. Some behaviors may be difficult to adjust without affecting all OmniBox bubbles. How strictly tied to the current find bar behaviors are we?
,
Oct 5 2017
re: appearing animation It might be a good idea to match other bubbles' fade-in/out. re: slide animation The motion might be also used to reveal a find result shown underneath the find bar. Screenrecording: go/cr641643c47 re: The find bar can only be dismissed by pressing escape (while it is focused) or pressing the close(X) button. Bubbles can also be dismissed by losing activation or clicking on the OmniBox icon. The current behavior should be kept for the find bar to support the case where the user wants to interact with the page while wanting to keep the finder bar open to use for other finds.
,
Oct 5 2017
To be clear, as the person who's pushing back, I don't think the find bar should go away when it loses activation. I do think it should go away when you click the omnibox icon; since all the other icons are clickable it seems like SOMETHING should happen when you click on it. I also don't think this is a huge issue either way.
,
Oct 5 2017
c49: Thanks for the clarification, bsep@. Other peer omnibox icons stay in place when they are clicked so there's less concern of losing the UI completely by mistake. You can click the icon again. However, Find icon is dismissed by clicking it, which gives a feeling of slightly losing a control, although it might not be critical. Also it's not desired to keep the find icon in the omnibox when the find popover is closed. Another sticky popover that uses the same behavior as Find popover is the permission request popover, which is harder for user to re-summon when it's dismissed by click on its anchor (i.e. omnibox security chip) by mistake. So, in choosing the behavior between 1) icon-to-dismiss vs. 2) icon-to-do-nothing for these two cases (i.e. sticky popovers), I recommend 2).
,
Oct 12 2017
Visual design edits needed before marking as Fixed: Icon colors: Convert from #5a5a5a to #757575 Icon widths: The current assets you have in the repository for the up and down arrow (i believe) are 10x6. To make the sizing we have in 'Find in Page', we need to increase the width to 12. Icon edges: I assume the up and down arrows are vector files? If so, please convert the rounded stroke ends to sharp stroke ends. This is also true for the close-x but that's being tracked elsewhere. Disabled icon color: Please ensure that the disabled icons are #9e9e9e (#000 0.38a). https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-00-stickersheet.png%3Ff=hidden
,
Oct 16 2017
was there a verdict on whether the stock icon needed tweaks? (c35)
,
Nov 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/218c89220f26ee9bcc748ad0635d4cfce810aa12 commit 218c89220f26ee9bcc748ad0635d4cfce810aa12 Author: Allen Bauer <kylixrd@chromium.org> Date: Wed Nov 01 12:57:22 2017 FindBarIcon's visiblity matches the visibility of the find bar. Added find_bar_icon to display the find icon in the OmniBox. This icon is only displayed while the find bar is displayed. Moved the find bar to be snuggled right up under the OmniBox close to the find bar icon image. TBR=phajdan.jr@chromium.org Bug: 651643 Change-Id: I99d9bd458eb14dea3bcee2e6ab86c1598c3e6ef8 Reviewed-on: https://chromium-review.googlesource.com/685637 Commit-Queue: Allen Bauer <kylixrd@chromium.org> Reviewed-by: Trent Apted <tapted@chromium.org> Reviewed-by: Allen Bauer <kylixrd@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Cr-Commit-Position: refs/heads/master@{#513111} [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/BUILD.gn [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.mm [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/find_bar/find_bar_controller.cc [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/find_bar/find_bar_controller.h [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/location_bar/location_bar.h [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/views/find_bar_host.cc [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/views/frame/browser_view_layout.cc [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/views/frame/browser_view_unittest.cc [add] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/views/location_bar/find_bar_icon.cc [add] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/views/location_bar/find_bar_icon.h [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/views/location_bar/location_bar_view.cc [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/browser/ui/views/location_bar/location_bar_view.h [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/chrome/test/base/test_browser_window.h [modify] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/components/toolbar/BUILD.gn [add] https://crrev.com/218c89220f26ee9bcc748ad0635d4cfce810aa12/components/toolbar/vector_icons/find_in_page.icon
,
Nov 2 2017
The find in page bubble/bar uses the dark browser theme in incognito mode to match the top level browser window. No other bubbles do. Now that we're moving the find bar to be under the location bar, like other bubbles, should the other bubbles use the incognito theme too, or should the find bar stop using incognito? (I prefer the former.)
,
Nov 2 2017
Is there someplace I can get the updated icons as an .svg or some other format that can be converted into the chrome vector icon format? Changing the colors is going to be a little more involved since the way they're set is based off the text color by applying some alpha blending. This is likely because they are supposed to respond to themes with differing colors.
,
Nov 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae1b29530b99d4e748fd75dfaac83c35530c105b commit ae1b29530b99d4e748fd75dfaac83c35530c105b Author: Allen Bauer <kylixrd@chromium.org> Date: Tue Nov 07 20:09:01 2017 Returning from full-screen mode while the find-bar is visible, make sure the find bar icon is in the activated/highlighted state. Observed during the demo at the harmony-eng meeting. Bug: 651643 Change-Id: I0e5f230f1aff6c33ed182a957698889c737c5314 Reviewed-on: https://chromium-review.googlesource.com/751693 Commit-Queue: Allen Bauer <kylixrd@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#514564} [modify] https://crrev.com/ae1b29530b99d4e748fd75dfaac83c35530c105b/chrome/browser/ui/views/find_bar_host.cc [modify] https://crrev.com/ae1b29530b99d4e748fd75dfaac83c35530c105b/chrome/browser/ui/views/location_bar/find_bar_icon.cc [modify] https://crrev.com/ae1b29530b99d4e748fd75dfaac83c35530c105b/chrome/browser/ui/views/location_bar/find_bar_icon.h [modify] https://crrev.com/ae1b29530b99d4e748fd75dfaac83c35530c105b/chrome/browser/ui/views/location_bar/location_bar_view.cc
,
Nov 16 2017
,
Nov 17 2017
,
Nov 22 2017
The assets in the previous comment image were manually hacked. I'd like to get official image assets that can be converted to the proper icon format. SVG files work the best. Also, note that these specific images are used in multiple locations, specially the close icon.
,
Nov 29 2017
Here are the current assets within the repo: https://cs.chromium.org/chromium/src/chrome/app/vector_icons/caret_down.icon?type=cs https://cs.chromium.org/chromium/src/chrome/app/vector_icons/caret_down.1x.icon?type=cs https://cs.chromium.org/chromium/src/chrome/app/vector_icons/caret_up.icon?type=cs https://cs.chromium.org/chromium/src/chrome/app/vector_icons/caret_up.1x.icon?type=cs https://cs.chromium.org/chromium/src/components/vector_icons/close_16.icon?type=cs https://cs.chromium.org/chromium/src/components/vector_icons/close_16.1x.icon?type=cs If you're using the vector-icons Code Search Chrome extension (https://github.com/sadrulhc/vector-icons), note that it doesn't quite faithfully convert the .icon files to an svg.
,
Dec 12 2017
@54: Find bar should stop using incognito theming for now, since it's a standard bubble now and we explicitly decided not to theme those (which I'm not sure I love, but OK).
,
Dec 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/acea41cedbf44025e042ee779ca08ce1ada64477 commit acea41cedbf44025e042ee779ca08ce1ada64477 Author: Allen Bauer <kylixrd@chromium.org> Date: Thu Dec 14 16:13:30 2017 Make sure the find bar isn't themed in incognito mode. Per this comment on linked bug: https://crbug.com/651643#c62 Bug: 651643 Change-Id: Ic90a7a83232d4c3eace025872ef839d68d68d45c Reviewed-on: https://chromium-review.googlesource.com/823083 Reviewed-by: Peter Kasting <pkasting@chromium.org> Commit-Queue: Allen Bauer <kylixrd@chromium.org> Cr-Commit-Position: refs/heads/master@{#524079} [modify] https://crrev.com/acea41cedbf44025e042ee779ca08ce1ada64477/chrome/browser/ui/views/find_bar_view.cc [modify] https://crrev.com/acea41cedbf44025e042ee779ca08ce1ada64477/chrome/browser/ui/views/find_bar_view.h
,
Jan 4 2018
,
Jul 25
This is as good as it's going to get for the Harmony effort; the spec is now outdated so we should file a new bug for further changes. |
||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||
/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-07a-layout.png){kind=link}
/Secondary%20UI%20Previews%20and%20specs%20(exports)/Spec#%2FSPEC-secondary-UI-00-stickersheet.png%3Ff=hidden){kind=link}
Comment 1 by shrike@chromium.org
, Sep 30 2016