Search icon in Synced Tabs in Windows sidebar is uncentered

VERIFIED FIXED in Firefox 56

Status

()

VERIFIED FIXED
a year ago
a year ago

People

(Reporter: rfeeley, Assigned: eoger)

Tracking

({regression})

56 Branch
Firefox 57
Unspecified
Windows
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 fixed, firefox57 verified)

Details

MozReview Requests

()

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

Attachments

(3 attachments)

(Reporter)

Description

a year ago
Created attachment 8897506 [details]
IMG_20170815_110028.jpg

Spotted in Sync Fest. The search icon in the Synced Tabs sidebar search box in Windows is not vertically centered and should match the search box in the History and Bookmarks sidebar.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → eoger
Status: NEW → ASSIGNED

Comment 2

a year ago
This got broken in bug 1380268 which changed the image. It also forgot to remove the hover styling, which makes the search icon disappear when hovered. We should just fix this properly.
Blocks: 1380268
status-firefox56: --- → affected
status-firefox57: --- → affected
Keywords: regression
Version: 55 Branch → 56 Branch

Comment 3

a year ago
mozreview-review
Comment on attachment 8897556 [details]
Bug 1390586 - Center search icon in Synced Tabs sidebar on Windows.

https://reviewboard.mozilla.org/r/168828/#review174456

::: browser/themes/windows/syncedtabs/sidebar.css:39
(Diff revision 1)
>  .textbox-search-icon {
>    width: 16px;
>    height: 16px;
>    background-image: url(chrome://global/skin/icons/search-textbox.svg);
>    background-repeat: no-repeat;
> +  background-position: center;

This isn't enough, because the 16x16 box is not vertically centered either.

Can we change set it to inline-block and vertical-align it such that it is actually centered? We may need to do the same on Linux. And we should remove the broken hover styling (and/or replace it with styling that does work).
Attachment #8897556 - Flags: review?(gijskruitbosch+bugs) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
Created attachment 8898027 [details]
after_fix.zip

Comment 6

a year ago
mozreview-review
Comment on attachment 8897556 [details]
Bug 1390586 - Center search icon in Synced Tabs sidebar on Windows.

https://reviewboard.mozilla.org/r/168828/#review174680

r=me, though this means no hover state on Windows anymore. I assume we're OK with that? Maybe file a followup bug for that if not?
Attachment #8897556 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 7

a year ago
Thanks! I'll ask rfeeley tomorrow about the hover state.

Comment 8

a year ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/128050973ec0
Center search icon in Synced Tabs sidebar on Windows. r=Gijs

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/128050973ec0
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Blocks: 1355330
Setting 56 status to disabled under the assumption that this is a Photon-only issue. Please set it back to affected and request uplift if that's not the case.
status-firefox55: --- → unaffected
status-firefox56: affected → disabled
status-firefox-esr52: --- → unaffected

Comment 11

a year ago
I have reproduced this bug with Nightly 57.0a1 (2017-08-15) on Windows 8.1, 64 Bit!

This bug's fix is verified on Latest Nightly 57.0a1

Build ID :  20170829100127
User Agent : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170823]
I have reproduced this bug with Nightly 57.0a1 (2017-08-15) on Ubuntu 16.04 LTS!

This bug's fix is verified with latest Nightly!

Build  ID  : 20170829100404
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0

 [bugday-20170830]

Comment 13

a year ago
As per comment 11 & comment 12, I am marking this bug as verified fixed!
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified

Comment 14

a year ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> Setting 56 status to disabled under the assumption that this is a
> Photon-only issue. Please set it back to affected and request uplift if
> that's not the case.

I set it as affected in comment #2 deliberately, because I tested on 56 and it was broken. :-(
status-firefox56: disabled → affected

Comment 15

a year ago
Comment on attachment 8897556 [details]
Bug 1390586 - Center search icon in Synced Tabs sidebar on Windows.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1380268
[User impact if declined]: synced tabs sidebar search box looks broken on Windows, even more broken when you hover the 'search' icon
[Is this code covered by automated tests?]: no, styling only
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: already verified, but for 56:
1. open synced tabs sidebar
2. observe spyglass icon on the end-side of the search box
-- prepatch: not vertically centered
-- should be: vertically well-aligned
3. hover over said icon
-- prepatch: icon disappears
-- should be: not disappearing, at a minimum...

[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: css-only change
[String changes made/needed]: none
Attachment #8897556 - Flags: approval-mozilla-beta?
Comment on attachment 8897556 [details]
Bug 1390586 - Center search icon in Synced Tabs sidebar on Windows.

Fix a regression and was verified. Beta56+.
Attachment #8897556 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 17

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/95c49dd1457c
status-firefox56: affected → fixed
You need to log in before you can comment on or make changes to this bug.