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

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Preferences
P1
normal
VERIFIED FIXED
2 months ago
2 months ago

People

(Reporter: rickychien, Assigned: rickychien)

Tracking

(Blocks: 1 bug)

55 Branch
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [photon-preference])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 months ago
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+

Updated

2 months ago
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Whiteboard: [photon-preference][triage] → [photon-preference]
(Assignee)

Updated

2 months ago
No longer depends on: 1365847
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 months ago
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
(Assignee)

Updated

2 months ago
Summary: Add arrow indicator history in menuitems → Show arrow indicator in menuitem if search result exists in its corresponding content
Comment hidden (mozreview-request)

Comment 4

2 months ago
mozreview-review
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-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 months ago
Created attachment 8877395 [details]
search-arrow-indicator.png

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.
(Assignee)

Comment 9

2 months ago
Btw, patch has updated and xmlns:xlink is removed and search-indicator.svg has renamed to search-arrow-indicator.svg.

Comment 10

2 months ago
mozreview-review
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-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

2 months ago
Updated patch for fixing display issue on Windows and extra padding will only be shown when search is active.

Comment 15

2 months ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

2 months ago
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)
Comment hidden (mozreview-request)

Comment 20

2 months ago
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
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED

Comment 22

2 months ago
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

Updated

2 months ago
Duplicate of this bug: 1370495
You need to log in before you can comment on or make changes to this bug.