1700839 - Text overlaps icon in Library search field in macOS <10.14

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Sam Johnson]

Attachment: Example

Steps to reproduce:

1. Be on macOS 10.13.x.
2. In Nightly, open Bookmarks menu > Show All Bookmarks.
3. Observe search field in the newly opened Library window.

Expected result:
Icon and text are not overlapping, as in newer versions of macOS. In High Sierra, the "Search" text in search fields is centered in other applications.

Actual result:
See screenshot. The icon is in the center of the field, but the text is left-aligned, so they overlap.

Mozregression blames this commit: Bug 1697350 - Simplify focus ring drawing code for search fields.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1697350

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Hi Markus, do you have cycles to look at this new regression in 88?

Comment 3

[Markus Stange [:mstange]]

Attachment: WIP: Bug 1700839 - clang-format nsNativeThemeCocoa.mm.

Depends on D110142

Comment 4

[Markus Stange [:mstange]]

Attachment: WIP: Bug 1700839 - Use a single space character in the search field cell so that isEmpty returns false and the magnifying glass icon is drawn on the left.

Depends on D110143

Comment 5

[Markus Stange [:mstange]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #2)

Hi Markus, do you have cycles to look at this new regression in 88?

Thanks for putting this on my radar again. I think I have a fix but still need to test it.

Comment 6

[Markus Stange [:mstange]]

Sam, could you check if the bug is fixed in this build?
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/SY-tw5WpQx63I1epp-NGAw/runs/0/artifacts/public/build/target.dmg

Comment 7

[Sam Johnson]

Attachment: Screen Shot 2021-03-29 at 3.46.07 PM.png

Thanks Markus! Unfortunately, I am still seeing the issue on the linked build.

Comment 8

[Markus Stange [:mstange]]

Thanks for testing!

Comment 9

[Tim Spurway [:tspurway]]

markus could you set priority / severerity? Also, does it only affect 10.13?

Comment 10

[Markus Stange [:mstange]]

Setting to P2 because it's a regression. We do not yet know which macOS versions are affected.

Comment 11

[Sam Johnson]

I found a machine running 10.12.6 and can confirm the issue also affects that version.

Comment 12

[Markus Stange [:mstange]]

I've checked the other versions, it does not affect any 10.14+ version.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Do we know if 89 is affected with the other MR1 changes going into it?

Comment 14

[Markus Stange [:mstange]]

I'm not aware of any changes that would have stopped this bug from happening on 89.

Comment 15

[Markus Stange [:mstange]]

(In reply to Sam Johnson from comment #7)

Thanks Markus! Unfortunately, I am still seeing the issue on the linked build.

I figured out why: I was only applying the fix to mSearchFieldCell but not to mToolbarSearchFieldCell. The library window uses the latter.

Comment 16

[Sam Johnson]

Attachment: Screen Shot 2021-04-06 at 11.43.16 AM.png

(In reply to Markus Stange [:mstange] from comment #15)

(In reply to Sam Johnson from comment #7)

Thanks Markus! Unfortunately, I am still seeing the issue on the linked build.

I figured out why: I was only applying the fix to mSearchFieldCell but not to mToolbarSearchFieldCell. The library window uses the latter.

Ah! Indeed, the bookmarks sidebar (which I did not check previously) is fixed with that build (bottom, top is Nightly). However, it gains a permanent "clear" button with the change.

Comment 17

[Markus Stange [:mstange]]

Yup. The clear button even appears on Big Sur with the original patch, and I'm fixing that now.

Comment 18

[Markus Stange [:mstange]]

Attachment: stand-alone testcase (save as main.mm)

Comment 19

[Markus Stange [:mstange]]

Attachment: Bug 1700839 - Use the same NSSearchFieldCell object for both toolbar-style and non-toolbar-style searchfields. r=#mac-reviewers.

Comment 20

[Markus Stange [:mstange]]

Attachment: Bug 1700839 - Make sure that search field cells render the magnifying glass icon at the start of the field. r=#mac-reviewers

Depends on D110971

Comment 21

[Markus Stange [:mstange]]

New try build: target.dmg

Comment 22

[Sam Johnson]

Try build looks good to me on 10.12.6, 10.13.6, and 11.2.3.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c03938fc1d2d
Use the same NSSearchFieldCell object for both toolbar-style and non-toolbar-style searchfields. r=mac-reviewers,bradwerth.
https://hg.mozilla.org/integration/autoland/rev/8a26e78f4fec
Make sure that search field cells render the magnifying glass icon at the start of the field. r=mac-reviewers,bradwerth

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c03938fc1d2d
https://hg.mozilla.org/mozilla-central/rev/8a26e78f4fec

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 26

[Markus Stange [:mstange]]

Comment on attachment 9213857 [details]
Bug 1700839 - Use the same NSSearchFieldCell object for both toolbar-style and non-toolbar-style searchfields. r=#mac-reviewers.

Beta/Release Uplift Approval Request

• User impact if declined: Incorrectly rendered search fields in Firefox UI elements on macOS 10.12 and 10.13
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Open the bookmarks sidebar and the library window, and check that the search field looks good on all versions of macOS and with/without text inside, with/without focus, and when the containing window is active/inactive.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We don't have many searchfields in the UI and Sam and I have already tested them on four different macOS versions. (10.12, 10.13, 11.2, 11.3) And the code changes can't really break anything that's not a search field.
• String changes made/needed: none

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9213857 [details]
Bug 1700839 - Use the same NSSearchFieldCell object for both toolbar-style and non-toolbar-style searchfields. r=#mac-reviewers.

Thanks for tracking this down. Approved for 88.0b9.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c0db33d5b31c
https://hg.mozilla.org/releases/mozilla-beta/rev/4bbafdd03c16

Comment 29

[Alexandru Trif, Desktop QA [:atrif]]

Reproduced the initial issue on Firefox 89.0a1 (2021-03-24) on macOS 10.12.
The issue is verified fixed with Firefox 88.0b9 (20210407182807) from comment 28 and 89.0a1 (20210407212527). The text and the icon are correctly aligned on macOS 10.12 and 10.13.