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

Issue 754873 link

Starred by 2 users

Issue metadata

Status: Archived
Owner:
Last visit > 30 days ago
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

search result row url link is empty

Project Member Reported by warx@chromium.org, Aug 11 2017

Issue description

There is a bug I found for issue 750690.

The search result could be a pure url link, please see the attachment. The specs doesn't cover this case.

sgabriel@, how should we handle this case?
 
pure_link_bubble_launcher.png
94.4 KB View Download
pure_link_fullscreen_launcher.png
63.4 KB View Download
are pure links only for chrome internal pages?

Comment 2 by warx@chromium.org, Aug 11 2017

no, I also see pure url (https://cs.chromium.org/#/) in my chromebook.
Got it. Let's make them using the single row spec, only in blue for URL:

- Roboto-regular 14px
- #3367D6 100%. 

Spec preview attached and added to:
https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZY5BZ8F2qhKy/files/MCE5Uhnn_J4GkP3CDSJOq7_-wMSJbxAVNpw

spec-preview.png
54.1 KB View Download

Comment 4 by warx@chromium.org, Aug 15 2017

Issue 754849 has been merged into this issue.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 15 2017

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

commit d323a482ee55624809476267e6c7160c2c2bbcdd
Author: Qiang Xu <warx@chromium.org>
Date: Tue Aug 15 18:49:38 2017

app_list: correctly show pure url link in search result row

changes:
Before this change, is_url is used to check whether the search result
is a url type result, if so, make |title_text_| show result's details
and |details_text_| show result's title. However, there might be a case
that a url title without details. This fix is to cover this case based
on specs.
(1) Swap title and details if OmniboxResult is a url result with
non-empty description.
(2) using |is_omnibox_result_| to specify matched/proposed color
requirements.
specs in  crbug.com/754873 
screenshot: https://screenshot.googleplex.com/jq0LhniuaEL

Test: test with fullscreen app list flag.
Bug:  754873 
Change-Id: I77335da01a1d012d2b1f895fb5aa07f43c0f8afb
Reviewed-on: https://chromium-review.googlesource.com/614609
Commit-Queue: Qiang(Joe) Xu <warx@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494468}
[modify] https://crrev.com/d323a482ee55624809476267e6c7160c2c2bbcdd/chrome/browser/ui/app_list/search/omnibox_result.cc
[modify] https://crrev.com/d323a482ee55624809476267e6c7160c2c2bbcdd/chrome/browser/ui/app_list/search/omnibox_result.h
[modify] https://crrev.com/d323a482ee55624809476267e6c7160c2c2bbcdd/ui/app_list/search_result.h
[modify] https://crrev.com/d323a482ee55624809476267e6c7160c2c2bbcdd/ui/app_list/views/search_result_view.cc

Comment 6 by warx@chromium.org, Aug 15 2017

Labels: M-61 Merge-Request-61

Comment 7 by ketakid@google.com, Aug 15 2017

Labels: -Merge-Request-61 Merge-Approved-61
Approving merge to M61 Chrome OS.
Project Member

Comment 8 by sheriffbot@chromium.org, Aug 21 2017

Cc: ketakid@google.com
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 21 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cce58f52e8c1fb8125a4687a9ba4bab57f57c96a

commit cce58f52e8c1fb8125a4687a9ba4bab57f57c96a
Author: Vadim Tryshev <vadimt@google.com>
Date: Mon Aug 21 21:30:51 2017

app_list: correctly show pure url link in search result row

changes:
Before this change, is_url is used to check whether the search result
is a url type result, if so, make |title_text_| show result's details
and |details_text_| show result's title. However, there might be a case
that a url title without details. This fix is to cover this case based
on specs.
(1) Swap title and details if OmniboxResult is a url result with
non-empty description.
(2) using |is_omnibox_result_| to specify matched/proposed color
requirements.
specs in  crbug.com/754873 
screenshot: https://screenshot.googleplex.com/jq0LhniuaEL

TBR=warx@chromium.org

(cherry picked from commit d323a482ee55624809476267e6c7160c2c2bbcdd)

Test: test with fullscreen app list flag.
Bug:  754873 
Change-Id: I77335da01a1d012d2b1f895fb5aa07f43c0f8afb
Reviewed-on: https://chromium-review.googlesource.com/614609
Commit-Queue: Qiang(Joe) Xu <warx@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#494468}
Reviewed-on: https://chromium-review.googlesource.com/624798
Reviewed-by: Vadim Tryshev <vadimt@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#724}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/cce58f52e8c1fb8125a4687a9ba4bab57f57c96a/chrome/browser/ui/app_list/search/omnibox_result.cc
[modify] https://crrev.com/cce58f52e8c1fb8125a4687a9ba4bab57f57c96a/chrome/browser/ui/app_list/search/omnibox_result.h
[modify] https://crrev.com/cce58f52e8c1fb8125a4687a9ba4bab57f57c96a/ui/app_list/search_result.h
[modify] https://crrev.com/cce58f52e8c1fb8125a4687a9ba4bab57f57c96a/ui/app_list/views/search_result_view.cc

Comment 10 by warx@chromium.org, Aug 21 2017

Status: Fixed (was: Assigned)

Comment 11 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment