GNOME Bugzilla – Bug 611095
Update look and feel for Find entry field
Last modified: 2010-06-08 21:18:06 UTC
Created attachment 154698 [details] find box for the shell The Find (aka Search) feature is a very important part of the shell experience, but in the current design it's subtle design is too easy to miss, even for experienced users. This suggestion is to improve the visibility and richness of the Find box. See attached image 1. Normal State 2. Hover State 3. Focus state 4. Focus with 1 or more characters Notes: * The cursor should blink, but slower than "normal" and if pjssible, with a fade/pulse effect. This is an important visual clue that the user can "just start typing." * On hover, fade from #1 to #2 - about 1 sec from dim to bright. This fade in/out will help make the search field more discoverable and invites use, as users mouse over this area frequently * When focus is set on the field and before the user types a character, remove the hint text ("Find") and set a glow on the field. * If focus is set on the field but no characters have been typed, there is no need to show the cancel button or take over the results area. User can just click anywhere outside the find box to cancel. and return to state #1 * When one or more characters is entered, show the cancel button - users needs to click here specifically to cancel the search. If the user cancels go to #1.
(In reply to comment #0) > * The cursor should blink, but slower than "normal" and if possible, with a > fade/pulse effect. This is an important visual clue that the user can "just > start typing." The only way I can think of to blink the cursor without modifying clutter itself would be to toggle the cursor visibility with a timeout function - while easy to implement, this would of course be a dump toggle rather than fade/pulse. Unless I overlooked something - someone feeling capable of sweet-talking the clutter folks? > * On hover, fade from #1 to #2 - about 1 sec from dim to bright. This fade > in/out will help make the search field more discoverable and invites use, as > users mouse over this area frequently This should probably live in StWidget/StThemeNode, right? I'm thinking of either an additional "transition-time" parameter to st_widget_set_style_pseudo_class or a new function (st_widget_set_style_pseudo_class_full or something). If StWidget is the way to go, then this is blocked by bug 607500. > * When focus is set on the field [...] set a glow on the field. Something else which should probably go into StThemeNode and should wait for bug 607500.
Created attachment 154870 [details] [review] [Overview] Update look and feel of the search field [WIP] Port the search entry to CSS and update look and feel according to the latest design. Probably not quite ready for code review yet, but I'd like to check nevertheless that the interaction matches the design. Obviously, all items mentioned in the comment above are ommitted.
Created attachment 154871 [details] [review] [StEntry] Add hover style property Set the entry's pseudo class to "hover" when the pointer enters the inactive widget and reset it appropriately on leave.
Created attachment 154872 [details] [review] [searchEntry] Highlight the search field on hover This patch should be squashed with the first one after landing the previous patch.
Created attachment 154873 [details] [review] [StEntry] Make the cursor size stylable Just as the cursor color can be styled using the "caret-color" property, expose the cursor size as "caret-size".
Comment on attachment 154872 [details] [review] [searchEntry] Highlight the search field on hover Sorry for the noise - the patch depends on the StEntry modification to have a visible effect, but apart from that there is no reason to keep it separate from the first patch.
Created attachment 154874 [details] [review] [Overview] Update look and feel of the search field [WIP] Merge two small CSS patches with the main patch.
Created attachment 154949 [details] [review] [Overview] Update look and feel of the search field [WIP] Disconnect key-press handler in dash when hiding
Review of attachment 154873 [details] [review]: LGTM
Review of attachment 154871 [details] [review]: One comment. ::: src/st/st-entry.c @@ +574,3 @@ + ClutterCrossingEvent *event) +st_entry_enter_event (ClutterActor *actor, +static gboolean This handling of the hint text is super evil since it will get confused if the user actually types the hint. I think it's been fixed in Mx. Up to you whether to put this patch in as-is and wait on doing a sync later, or do a sync then rework this patch.
Comment on attachment 154873 [details] [review] [StEntry] Make the cursor size stylable Attachment 154873 [details] pushed as a0b5a44 - [StEntry] Make the cursor size stylable
Created attachment 155525 [details] [review] fix issues with hint text (In reply to comment #10) > This handling of the hint text is super evil since it will get confused if the > user actually types the hint. I think it's been fixed in Mx. > > Up to you whether to put this patch in as-is and wait on doing a sync later, or > do a sync then rework this patch. Not sure if you are referring to a "complete" sync here, this is the relevant commit from mx.
Created attachment 155526 [details] [review] [StEntry] Add hover style property Updated patch to incorporate hint changes in previous patch
Created attachment 155570 [details] [review] [Overview] Update look and feel of the search field [WIP] Rebased patch.
Review of attachment 155525 [details] [review]: That looks right, thanks a lot for doing the merge!
Review of attachment 155526 [details] [review]: Looks reasonable.
Review of attachment 155570 [details] [review]: Comments follow. ::: js/ui/dash.js @@ +204,3 @@ + this.entry = this.actor.clutter_text; + hint_text: _("Find ...") }); + this.actor = new St.Entry({ name: "searchEntry", Do we still need the check for .hint_text here? @@ +216,2 @@ this.pane = null; Assign this._keyPressId = 0 here in _init @@ +220,3 @@ + Lang.bind(this, this._onCapturedEvent)); + this._capturedEventId = global.stage.connect('captured-event', + show: function() { While we don't ever destroy the search entry, you should also catch 'destroy' and disconnect there for completeness I think. This will avoid a pitfall if we ever recreate the overview at runtime for any reason (and I can imagine doing that). @@ +226,3 @@ + global.stage.disconnect(this._capturedEventId); + if (this.isActive()) + hide: function() { Check if capturedEventId > 0 @@ +249,3 @@ + this.entry.text != ''; + return this.entry.text != this.actor.hint_text && + isActive: function() { Ditto for .hint_text here @@ +260,3 @@ + this.entry.text != ''; + return this.entry.text != this.actor.hint_text && + isActive: function() { I think we need to do this on BUTTON_PRESS; otherwise, the press event will be passed to whatever else outside the search, but we'll eat the BUTTON_RELEASE, which is going to cause problems. @@ +275,3 @@ + // haven't, the key is ignored + if (sym == Clutter.Escape) + if (this.entry.text != this.actor.hint_text) { Another hint text comparison @@ +920,3 @@ + Lang.bind(this, this._onKeyPress)); + this._keyPressId = global.stage.connect('key-press-event', + this._searchEntry.show(); Keep .show idempotent by checking if (this._keyPressId == 0) @@ +928,3 @@ this._activePane.close(); if (this._activePane != null) + global.stage.disconnect(this._keyPressId); Still need to keep .hide() idempotent; check if (this._keyPressId > 0)
Nice. A few comments: * On hover, fade from #1 to #2 - about 1 sec from dim to bright. * When clicking in the entry box (state 3?) the hotcorner isn't able to be used * Drop the ellipsis I think * Jeremy's mockups have a glow aura around the box when it is active * The cursor should slowly pulse * The entry box seems to change size/position when text is entered
(In reply to comment #18) > * On hover, fade from #1 to #2 - about 1 sec from dim to bright. See comment 1 - unless someone can think of a non-hackish way of doing it outside StWidget/StThemeNode, it will conflict with Walters' work on bug 607500. > * Jeremy's mockups have a glow aura around the box when it is active This could be done either for entries only, or as a general style property - I very much prefer the latter, which is (you guessed it!) kind of blocked by bug 607500 ... > * The cursor should slowly pulse This one is tough - the cursor is drawn by clutter, and it's either on or off. I had an IRC conversation with ebassi on this, it results that blinking the cursor is rather expensive currently (read: the entire stage has to be redrawn); with clutter-1.2 it should be possible to implement it cheaper, but there are no real plans for a cursor-blink property (hint: ebassi seems to be one of those fiddling with gtkrc to _disable_ the cursor blinking) Without modifying clutter itself, the only thing we could do is toggling the cursor visibility on a regular interval - no pulsing, no fading, just a dull on/off blinking. > * When clicking in the entry box (state 3?) the hotcorner isn't able to be > used I was afraid you would say that ... not I was really expecting to get away with it anyway ;)
Created attachment 155670 [details] [review] [Overview] Update look and feel of the search field [WIP] (In reply to comment #17) > I think we need to do this on BUTTON_PRESS; otherwise, the press event will be > passed to whatever else outside the search, but we'll eat the BUTTON_RELEASE, > which is going to cause problems. I found this only to be true iff activating the entry between BUTTON_PRESS and BUTTON_RELEASE (read: press mouse button, start typing, delete all typed characters, release mouse button); this is seems fairly constructed, but I did the change anyway ... In case it's not clear from the previous comment - missing bits are: - blinking cursor - "glow" effect on activated entry - fading between hover states
I spoke with McCann about this. Until we can get the fade transition and the glow treatment on the border done in a way that is re-usable and repeatable, we think this should be sufficient to push as is, with one change: Change the hover prelight such that only the border gets brighter, similar to how we do on the app icons. This way the rollover will not be as stark. To make this work, we should darken the border slightly in the default state so the prelight border shows up better. With that change I think this is good to go.
Created attachment 155706 [details] [review] [Overview] Update look and feel of the search field Update hover prelight.
Looks pretty good. One nitpick :) When I first bring up the overview the entry box has a cursor (good). However, when I click in the box, leave the overview, and come back it does not have a cursor (bad).
Created attachment 155752 [details] [review] [Overview] Update look and feel of the search field (In reply to comment #23) > However, when I click in the box, leave the overview, and come back it > does not have a cursor (bad). Wooops. Sorry ...
Looks good to me.
Review of attachment 155752 [details] [review]: Looks good.
Comment on attachment 155752 [details] [review] [Overview] Update look and feel of the search field Attachment 155752 [details] pushed as 858a6bf - [Overview] Update look and feel of the search field Leaving the bug open as there's still work to do to match the mockup.
Created attachment 157006 [details] [review] [SearchEntry] Add a glow effect when activated (Ab)use -st-shadow to add a glow effect to the activated search entry, matching the latest mockups. Depends on bug 613832.
Review of attachment 157006 [details] [review]: I can't perceive this change well but it seems fine to me.
Comment on attachment 157006 [details] [review] [SearchEntry] Add a glow effect when activated Attachment 157006 [details] pushed as 8a25d49 - [SearchEntry] Add a glow effect when activated
Created attachment 161381 [details] [review] [searchEntry] Update hover style Make the hover style of the search entry more distinct, and use a fade effect of about one second to transition from dim to bright.
Note that the last patch depends on bug 619025.
Created attachment 161384 [details] [review] [searchEntry] Update hover style Use a "real" constant for the transition duration instead of a magic value of 1000.
Created attachment 161385 [details] [review] [searchEntry] Correct some erroneous hover states As the search entry captures the pointer when activated, the hover state is not updated properly when the activation is cancelled (either by clicking outside the entry or by hitting Escape). Update the state manually in these cases. In this form the patch depends on the previous one, but could be updated easily if desired.
Review of attachment 161385 [details] [review]: ::: js/ui/dash.js @@ +242,3 @@ + this.actor.hover_transition = 0; + this.actor.set_hover(hovered); + this.actor.hover_transition = ENTRY_HOVER_TRANSITION; Our CSS shadows ignore the actor's paint opacity, which is why transitions involving shadows look extremely odd. As the activated search entry uses a shadow for the glow effect, we disable the fading effect temporarily in this case. The shadow drawing should probably still be fixed ...
Created attachment 162383 [details] [review] [searchEntry] Correct some erroneous hover states As the search entry captures the pointer when activated, the hover state is not updated properly when the activation is cancelled (either by clicking outside the entry or by hitting Escape). Update the state manually in these cases.
Created attachment 162385 [details] [review] [searchEntry] Update hover style Make the hover style of the search entry more distinct, and use a fade effect of about one second to transition from dim to bright. Updated both patches to account for changes in bug 619025.
Created attachment 163061 [details] [review] [searchEntry] Update hover style Updated patch to - match the series in bug 619025 - work around a crappy looking undesired style transition
Review of attachment 162383 [details] [review]: Ugh that we have to do this, but the patch looks good. (Another candidate for st_widget_contains() or clutter_container_contains(), I guess.)
Created attachment 163092 [details] [review] [searchEntry] Update hover style Remove the workaround as the underlying issue is fixed in master.
Comment on attachment 162383 [details] [review] [searchEntry] Correct some erroneous hover states Attachment 162383 [details] pushed as d96d07a - [searchEntry] Correct some erroneous hover states
Review of attachment 163092 [details] [review]: OK to commit if you fix the below one way or the other. You should make sure that our IRC conversation about remove_style_class/add_style_class gets recorded in a new bug so we don't forget about it. ::: data/theme/gnome-shell.css @@ +386,3 @@ + background-color: #e8e8e8; + caret-color: #545454; + transition-duration: 1000; If this is intentional because you want to allow transitions from 'focus' to 'focus hover' in themes (though there is none currently) then you should comment /* override transition-duration:0 for the case of 'focus hover' */ If that's not the intent, then remove this - this style will be applied on top of the rules from #searchEntry so we don't need to duplicate properties from there.
Attachment 163092 [details] pushed as 7c5343a - [searchEntry] Update hover style (In reply to comment #42) > If this is intentional because you want to allow transitions from 'focus' to > 'focus hover' in themes (though there is none currently) then you should > comment Not intentional, no - fixed. @Jeremy: I know that the pulsing cursor is still missing - I don't think we can achieve anything but a hackish-blinking cursor from within the shell. I had a conversation with ebassi recently though, and he's willing to add support to clutter itself - I doubt it will be high-priority, but it'll be way better than anything we can do ourselves, so I'm gonna close this bug for now.