Closed Bug 1370491 Opened 7 years ago Closed 7 years ago

Show arrow indicator in menuitem if search result exists in its corresponding content

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Version:
55 Branch
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- verified

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(2 files)

Bug 1370491 - Show arrow indicator in menuitem
59 bytes, text/x-review-board-request
jaws
: review+
Details
search-arrow-indicator.png
101.69 KB, image/png
Details 
Follow https://bugzilla.mozilla.org/show_bug.cgi?id=1365847#c27, we might deal with this missing part.

This was a unclear part in our Search spec, but spec has updated https://mozilla.invisionapp.com/share/ZDAGPK3AF#/screens/218928191

History section is an exception. Each menuitem should be able to show tooltips if there are matching result corresponding to their content.
Flags: qe-verify+
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Whiteboard: [photon-preference][triage] → [photon-preference]
No longer depends on: 1365847
Jared, in order to make those hidden history content searchable (invisible content in deck element), we have to append all history section's l10n strings to menuitem's searchkeywords.

Please review it kindly. Thanks
Summary: Show search tooltips on menuitem for history section → Add arrow indicator history in menuitems
Summary: Add arrow indicator history in menuitems → Show arrow indicator in menuitem if search result exists in its corresponding content
Comment on attachment 8876642 [details]
Bug 1370491 - Show arrow indicator in menuitem

https://reviewboard.mozilla.org/r/147990/#review153018

I applied the patch and don't see any change to the menuitems within the popup for "Remember history"/"Never remember history"/"Use custom settings for history". I can't get the search-indicator.svg to appear. Also, I think the search-indicator.svg filename should be changed to more specifically state that it is different than the normal search-glass icon.

::: toolkit/themes/shared/icons/search-indicator.svg:5
(Diff revision 2)
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" viewBox="0 0 8 10">

Gecko supports href without the xlink namespace so you can remove this reference and on line 10 just use href="#anchor".

You can also run this SVG through SVGO (https://jakearchibald.github.io/svgomg/ if you don't have it/want it installed locally) to compress the SVG.
Attachment #8876642 - Flags: review?(jaws) → review-
I'm not sure why you are unable to see my change. Attached my screenshot. You can try to search "history" or "cookie" to see what happens when opening menulist.

I've tried those SVGO tools as you mentioned but it didn't get rid of the xlink namespace, so I have to remove it manually.
Btw, patch has updated and xmlns:xlink is removed and search-indicator.svg has renamed to search-arrow-indicator.svg.
Comment on attachment 8876642 [details]
Bug 1370491 - Show arrow indicator in menuitem

https://reviewboard.mozilla.org/r/147990/#review153298

Did you test this patch on Windows? .menu-iconic-left is display:none for me. Disabling that rule shows me https://www.screencast.com/t/p9GqmHESpi, which looks to have too much padding-inline-start on the menuitems now that they would have an icon.

::: browser/themes/shared/incontentprefs/preferences.inc.css:641
(Diff revision 5)
> +menulist > menupopup menuitem:not([image]) > .menu-iconic-left {
> +  width: 12px;
> +  height: 10px;
> +  margin-inline-end: 6px;
> +}
> +
> +menulist > menupopup menuitem[indicator=true]:not([image]) > .menu-iconic-left {
> +  background: url(chrome://global/skin/icons/search-arrow-indicator.svg) no-repeat center/12px 10px;
> +}

We shouldn't be displaying any of this blank area unless a search is active.

Also, please separate the background rule to use the longhand properties.
Attachment #8876642 - Flags: review?(jaws) → review-
Updated patch for fixing display issue on Windows and extra padding will only be shown when search is active.
Comment on attachment 8876642 [details]
Bug 1370491 - Show arrow indicator in menuitem

https://reviewboard.mozilla.org/r/147990/#review153534

r=me with the following changes

::: browser/components/preferences/in-content-new/findInPage.js:315
(Diff revisions 5 - 8)
> +        if (menulist && menulist.getAttribute("indicator") !== "true") {
> +          menulist.setAttribute("indicator", "true");
> +          this.listSearchMenuitemIndicators.push(menulist);

Please change listSearchMenuitemIndicators to a Set, and then remove the call to getAttribute here since it won't be necessary.

We should change the other arrays to Sets too.

::: browser/themes/shared/incontentprefs/preferences.inc.css:644
(Diff revisions 5 - 8)
>  }
>  
> -menulist > menupopup menuitem:not([image]) > .menu-iconic-left {
> -  width: 12px;
> +menulist[indicator=true] > menupopup menuitem:not([image]) > .menu-iconic-left {
> +  display: -moz-box;
> +  width: 10px;
> +  min-width: 10px; /* Override the min-witdh defined in menu.css */

spelling error in the comment, min-width instead of min-witdh.

also, can this just be `min-width: auto;`?
Attachment #8876642 - Flags: review?(jaws) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s dcca5bf1047f -d 50a06f21ea6c: rebasing 401855:dcca5bf1047f "Bug 1370491 - Show arrow indicator in menuitem r=jaws" (tip)
merging browser/components/preferences/in-content-new/findInPage.js
merging browser/components/preferences/in-content-new/privacy.xul
warning: conflicts while merging browser/components/preferences/in-content-new/findInPage.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6256be938d18
Show arrow indicator in menuitem r=jaws
https://hg.mozilla.org/mozilla-central/rev/6256be938d18
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Build ID: 20170615030208
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

Verified as fixed on Firefox Nightly 56.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Depends on: 1398303
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: