Closed Bug 1390586 Opened 2 years ago Closed 2 years ago

Search icon in Synced Tabs in Windows sidebar is uncentered

Categories

(Firefox :: Sync, defect)

56 Branch
Unspecified
Windows
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: rfeeley, Assigned: eoger)

References

Details

(Keywords: regression)

Attachments

(3 files)

Attached image 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.
Assignee: nobody → eoger
Status: NEW → ASSIGNED
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
Keywords: regression
Version: 55 Branch → 56 Branch
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-
Attached file after_fix.zip
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+
Thanks! I'll ask rfeeley tomorrow about the hover state.
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/128050973ec0
Center search icon in Synced Tabs sidebar on Windows. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/128050973ec0
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
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.
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]
As per comment 11 & comment 12, I am marking this bug as verified fixed!
Status: RESOLVED → VERIFIED
(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. :-(
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+
You need to log in before you can comment on or make changes to this bug.