New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 864799 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 28
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Improve AiS stock answer readability

Project Member Reported by emilyschechter@chromium.org, Jul 17

Issue description

Can we make stock answers more readable by doing some one-off formatting with superscript/sizing? 
 
Added this redline for reference. Added color to the +/- numbers in the the first string and a different grey for the secondary string to differentiate between the query and the secondary answer string. 
Screen Shot 2018-07-17 at 4.46.48 PM.png
106 KB View Download
Labels: Group-Omnibox
Status: Available (was: Untriaged)
Labels: -Pri-3 Pri-2
Owner: orinj@chromium.org
Cc: jdonnelly@chromium.org
orinj: I vaguely recall we talked about how there would have to be a little bit of code refactoring to accommodate more colors for parts of answers in the Refresh UI. But I think it didn't look like that big of a change? Let me know how much effort you think making these adjustments will take.
Status: Started (was: Available)
It's one of those situations where some one-offs and TODOs accumulated with color setting happening here and there.  I'm planning to clean this up at least enough to achieve the new colorings while moving in the direction of simplicity.  I don't think it's a big effort, but these text objects are subtly stateful and a little quirky to work with sometimes, so it's possible some time may be required to avoid regression.  Doesn't look too bad, though, I can probably have a CL working by tomorrow.
Well... the plot thickens.  I have text "types" coming in which can be used to apply styling to text ranges.  So the stock answers are already colored and bolded as in the redline spec.  The trouble is that now answers other than stocks will be styled as well, according to the text types... for example, now Shaq is 7' 1" in *bold* not normal weight.  So either we have to:

* Go to the source and change the text types on answers (may be deep and have side effects, e.g. on old layout answers)
* Or use some other means to detect stocks and only apply the styling in this case

My question is:  How "one-off" is this styling, really?  In my experience, the future is unknown so it's better to do things within a consistent logical framework instead of accumulating special exceptions.  But if it really is one-off, I can detect ANSWER_TYPE_FINANCE and ignore text types for all other answers.

Other suggestions for how to reconcile the inconsistency are also welcome.
See crrev.com/c/1186130 .  This text-type interpretation approach will support both the old and new style answers, and will be much more maintainable going forward.  Getting it to this point was involved, but now that it's done, restyling new answers is simple.  Adding the color changes and bold for stocks became easy, and changing others will be similar.
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 28

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

commit 69f31703a3b680c0b03d8004c2fb88400bda37a2
Author: Orin Jaworski <orinj@chromium.org>
Date: Tue Aug 28 00:16:03 2018

[omnibox] Simplify new answer styling with text type interpretation

This CL unifies disparate bits of logic for styling new answers by
interpreting the text field types provided on the answer JSON from
server. It began with styling stock answers, but the TextType
values could not be applied consistently to new answers without
considering additional context like answer type and line rank.
So instead of complicating presentation logic further, it was
eliminated. Now the presentation style is decided once at parse time
and only the newly interpreted text types are used to decide
rendering properties for text ranges. See TextTypeNewAnswer for more.
Some TODOs were resolved and parsing logic simplified as well.

Bug:  864799 
Change-Id: If7ad3e4c83cb0eee2b4053646dacf7c139e461fe
Reviewed-on: https://chromium-review.googlesource.com/1186130
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586515}
[modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell_unittest.mm
[modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/chrome/browser/ui/views/omnibox/omnibox_result_view.cc
[modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/chrome/browser/ui/views/omnibox/omnibox_text_view.cc
[modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/chrome/browser/ui/views/omnibox/omnibox_text_view.h
[modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/components/omnibox/browser/autocomplete_match_type_unittest.cc
[modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/components/omnibox/browser/search_suggestion_parser.cc
[modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/components/omnibox/browser/suggestion_answer.cc
[modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/components/omnibox/browser/suggestion_answer.h
[modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/components/omnibox/browser/suggestion_answer_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment