1273106 - When building with a more recent OS X SDK, the search input field in the bookmarks sidebar has a (broken!) placeholder

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[:Gijs (he/him)]

STR:

1. open bookmarks sidebar (cmd-B)
2. observe the input field at the top of the sidebar

ER:
empty

AR:
says "Search"

The placeholder isn't there on Nightly, but it's there on local builds (on OS X 10.11). There doesn't seem to be any XUL markup that explains its presence, as far as I can see with either the builtin devtools or DOM Inspector. It doesn't go away when typing.

I'm assuming the placeholder is created by OS X somehow. It doesn't line up well with our cursor, and it doesn't go away with typing, so I think we should just get rid of it...

Comment 1

[Stefan [:stefanh]]

From https://developer.apple.com/library/mac/releasenotes/AppKit/RN-AppKit/#10_11SearchField

NSSearchField Option for Centered Placeholder
We've added a new "centersPlaceholder" property that controls whether the placeholder, magnifier search button, and search menu are centered when the search field does not have focus and does not have text. With focus, the control animates its contents to the edge.

The default value for centersPlaceholder is YES since the centered-look described above was the default in Yosemite.

With the centered-look, the only way to have an empty placeholder is to use a space, “ “, since nil or the empty string will use the “Search” default. If linked on or after 10.11, only nil will result in the default, no matter what centersPlaceholder is.

Since the search field can appear centered or not, we’ve added a “centered” parameter to the custom layout methods in NSSearchFieldCell. Note that the value will not always be the same as the search field’s focus state, nor is it the same as the new centersPlaceholder parameter which states the ability to be centered. The older NSSearchFieldCell versions remain since they may be useful to those who implement custom search fields using cells.

@property BOOL centersPlaceholder;

Defaults to YES. When set, the search field's components are centered within the control if the field is empty and does not have focus. When receiving focus, given the field is empty, the centered objects will animate to the edges of the control. When this property is set to NO, the components are always at the edge. When YES, the configuration requires wantsLayer to also be YES. When NO, wantsLayer should be NO for standard appearance but can be YES for custom drawing

See also https://groups.google.com/a/chromium.org/forum/#!topic/blink-reviews/zeOHMBODv_8

Comment 2

[Stefan [:stefanh]]

Actually, this seems to work: https://bug-147192-attachments.webkit.org/attachment.cgi?id=257274 (in nsNativeThemeCocoa::DrawSearchField). However, I get the impression that this method is depreciated).

Comment 3

[Markus Stange [:mstange]]

I'd prefer using setCenteredLook:NO over setPlaceholderString:@"". Stefan, do you want to write the patch?

Instead of using NSInvocation the way Blink does it, we can just do what we always do and add an #ifdef'd @interface with that method so that we can call it regularly (after checking that it's available).

Comment 4

[Stefan [:stefanh]]

I'll have a patch tomorrow.

Comment 5

[Stefan [:stefanh]]

Attachment: 1273106.1.diff

I made a build using the 10.10 SDK and didn't see the issue, so this only seems to happen when building with the 10.11 SDK (and probably with future SDKs). If you prefer, we can skip the conditional checks - the method has been available for many years (IIRC since the dawn of OS X).

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8753975 [details] [diff] [review]
1273106.1.diff

Review of attachment 8753975 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, let's remove the ifdefs. According to the Apple docs, NSTextFieldCell has had the property placeholderString since 10.3.

Comment 7

[Stefan [:stefanh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d1e54fa796ccd353fe6bd78667e6367e42e2e49

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e814e3864b70

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e814e3864b70