Closed Bug 1273106 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: Gijs, Assigned: stefanh)

Details

Attachments

(1 file)

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...
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
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).
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).
I'll have a patch tomorrow.
Assignee: nobody → stefanh
Attached patch 1273106.1.diffSplinter Review
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).
Attachment #8753975 - Flags: review?(mstange)
Status: NEW → ASSIGNED
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.
Attachment #8753975 - Flags: review?(mstange) → review+
https://hg.mozilla.org/mozilla-central/rev/e814e3864b70
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.