Suppress native placeholder text on MacOS Input Type=Search [chromium/src : master]
Eric Lawrence (Gerrit)
PTAL?
To view, visit change 610501. To unsubscribe, or for help writing mail filters, visit settings.
Keishi Hattori (Gerrit)
Patch set 1:Code-Review +1
Erik Chen (Gerrit)
1 comment:
File third_party/WebKit/Source/core/layout/LayoutThemeMac.mm:
Patch Set #1, Line 980:
[search_.Get() setPlaceholderString:@""];Will this have the right behavior on macOS 10.10?
Do we need to keep the old behavior there, and the new behavior on 10.11+?
To view, visit change 610501. To unsubscribe, or for help writing mail filters, visit settings.
Eric Lawrence (Gerrit)
Patch Set 1:
(1 comment)
1 comment:
Patch Set #1, Line 980:
[search_.Get() setPlaceholderString:@""];Will this have the right behavior on macOS 10.10?
I'm not an expert on the Mac SDK, but my understanding from reading the docs (and the Mozilla bug tracker) is that the setPlaceholderString function works properly in iOS 10.3 and later.
Do we have any older MacOS versions around that I could test with?
To view, visit change 610501. To unsubscribe, or for help writing mail filters, visit settings.
Erik Chen (Gerrit)
Patch Set #1, Line 980:
[search_.Get() setPlaceholderString:@""];
Do we have any older MacOS versions around that I could test with?
Yes...Are you in MTV? If so, come find me in the afternoon.
is that the setPlaceholderString function works properly in iOS 10.3 and later.
iOS != macOS. The versioning is unrelated.
To view, visit change 610501. To unsubscribe, or for help writing mail filters, visit settings.
Eric Lawrence (Gerrit)
Patch Set #1, Line 980:
[search_.Get() setPlaceholderString:@""];> Do we have any older MacOS versions around that I could test with?
Sorry, I mistyped, I meant "OS X 10.3".
Unfortunately, I'm in Austin, not MTV.
If your older Mac is configured appropriately, it should be straightforward to run the binaries built off the bots?
git clone https://github.com/luci/luci-py
python isolateserver.py download -I https://isolateserver.appspot.com --namespace default-gzip -s d3e7f3eb915eaea39c275e385425a08a2f62c4d8 --target NoPlaceholderTest
To view, visit change 610501. To unsubscribe, or for help writing mail filters, visit settings.
Erik Chen (Gerrit)
Erik Chen would like Robert Sesek and Kent Tamura to review this change.
Suppress native placeholder text on MacOS Input Type=Search
The OS X 10.11 SDK introduced a change whereby the "Search" placeholder
text is no longer suppressed by setting setCenteredLook to NO on the
NSSearchFieldCell control. Instead, we now explicitly invoke
|setPlaceholderString| on the control.
Bug: 752362
Change-Id: I0da791fa3510d682731c5d64971ac669a7608d5a
---
M third_party/WebKit/Source/core/layout/LayoutThemeMac.mm
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/third_party/WebKit/Source/core/layout/LayoutThemeMac.mm b/third_party/WebKit/Source/core/layout/LayoutThemeMac.mm
index df13deb..a43eadd 100644
--- a/third_party/WebKit/Source/core/layout/LayoutThemeMac.mm
+++ b/third_party/WebKit/Source/core/layout/LayoutThemeMac.mm
@@ -977,18 +977,7 @@
[search_.Get() setBezeled:YES];
[search_.Get() setEditable:YES];
[search_.Get() setFocusRingType:NSFocusRingTypeExterior];
- SEL sel = @selector(setCenteredLook:);
- if ([search_.Get() respondsToSelector:sel]) {
- BOOL bool_value = NO;
- NSMethodSignature* signature =
- [NSSearchFieldCell instanceMethodSignatureForSelector:sel];
- NSInvocation* invocation =
- [NSInvocation invocationWithMethodSignature:signature];
- [invocation setTarget:search_.Get()];
- [invocation setSelector:sel];
- [invocation setArgument:&bool_value atIndex:2];
- [invocation invoke];
- }
+ [search_.Get() setPlaceholderString:@""];
}
return search_.Get();
To view, visit change 610501. To unsubscribe, or for help writing mail filters, visit settings.
Erik Chen (Gerrit)
Working on getting an older macOS device. In the meanwhile...
The code you're removing was explicitly added to remove the default placeholder:
https://codereview.chromium.org/586413002
It was based on a WebKit change: https://trac.webkit.org/changeset/168870/webkit which seems to be solving a different problem:
https://bugs.webkit.org/show_bug.cgi?id=132930
What I would like to understand is whether centeredLook = NO has other side effects that we want to keep.
Adding original authors/reviewers in case they still remember.
Eric Lawrence (Gerrit)
Patch Set 1:
Working on getting an older macOS device. In the meanwhile...
The code you're removing was explicitly added to remove the default placeholder:
https://codereview.chromium.org/586413002It was based on a WebKit change: https://trac.webkit.org/changeset/168870/webkit which seems to be solving a different problem:
https://bugs.webkit.org/show_bug.cgi?id=132930What I would like to understand is whether centeredLook = NO has other side effects that we want to keep.
Adding original authors/reviewers in case they still remember.
I /think/ centeredLook doesn't have other effects that are relevant to us. In Blink, we suppress the Search field's built-in icon (a magnifying glass) so the only remaining visible piece is the placeholder text that this CL suppresses.
(You can see a Search field that uses the default centeredLook atop the OS X built-in "Mail" app).
Erik Chen (Gerrit)
Patch Set 1:
Patch Set 1:
Working on getting an older macOS device. In the meanwhile...
The code you're removing was explicitly added to remove the default placeholder:
https://codereview.chromium.org/586413002It was based on a WebKit change: https://trac.webkit.org/changeset/168870/webkit which seems to be solving a different problem:
https://bugs.webkit.org/show_bug.cgi?id=132930What I would like to understand is whether centeredLook = NO has other side effects that we want to keep.
Adding original authors/reviewers in case they still remember.
I /think/ centeredLook doesn't have other effects that are relevant to us. In Blink, we suppress the Search field's built-in icon (a magnifying glass) so the only remaining visible piece is the placeholder text that this CL suppresses.
(You can see a Search field that uses the default centeredLook atop the OS X built-in "Mail" app).
Still haven't managed to get a hold of a 10.10 machine. I suppose that if there were a visible effect, we'd expect the layout tests to catch the change. For that matter, I'm kind of surprised this behavior change *wasn't* caught by the layout tests...and then I remembered that the layout tests are still building against the 10.10 SDK. [they're the last holdout].
But the fact that this CL doesn't cause the layout tests to fail suggests that this has the appropriate behavior.
To view, visit change 610501. To unsubscribe, or for help writing mail filters, visit settings.
Eric Lawrence (Gerrit)
But the fact that this CL doesn't cause the layout tests to fail suggests that this has the appropriate behavior.
Indeed, the original regression presumably would have broken the
\src\third_party\WebKit\LayoutTests\fast\forms\search\search-appearance-basic.html image if the 10.11 SDK were linked.
I dug up an old Mac running 10.10.5 Mavericks and was surprised to discover that the change in this CL did not work as expected. On 10.10.5 running the latest 62.3185 Canary, the placeholder text is hidden. When running a private build of the current patchset, the unwanted placeholder text reappears. More surprisingly to me, Firefox does not show the placeholder despite (seemingly) using only the same setPlaceholderString call.
Patch set 1:Code-Review -1
Kent Tamura (Gerrit)
Patch Set 2 lgtm.
Do we still need NSInvocation with the current SDK?
Patch set 2:Code-Review +1
Erik Chen (Gerrit)
lgtm with nit
I dug up an old Mac running 10.10.5 Mavericks and was surprised to discover that the change in this CL did not work as expected. On 10.10.5 running the latest 62.3185 Canary, the placeholder text is hidden. When running a private build of the current patchset, the unwanted placeholder text reappears. More surprisingly to me, Firefox does not show the placeholder despite (seemingly) using only the same setPlaceholderString call.
Interesting. Presumably this is why the original code didn't just call setPlaceholderText:@"".
1 comment:
File third_party/WebKit/Source/core/layout/LayoutThemeMac.mm:
Patch Set #2, Line 985:
if (IsOS10_9() || IsOS10_10()) {base::mac::IsAtMostOS10_10()
To view, visit change 610501. To unsubscribe, or for help writing mail filters, visit settings.
Eric Lawrence (Gerrit)
>> reappears. More surprisingly to me, Firefox does not show the placeholder
>> despite (seemingly) using only the same setPlaceholderString call.
> Interesting. Presumably this is why the original code didn't just call setPlaceholderText:@"".
I built a small Cocoa test app last night and confirmed that setPlaceholderString() isn't effective on 10.10. Interestingly, the Firefox stuff was all a bit of a red herring, insofar as Firefox Nightly is still using the 10.7 SDK, but they're not using NSSearchField at all for their HTML rendering; they use a plain text field. (They use a NSSearchField in their XUL Bookmark manager, and it shows the placeholder text on 10.10 but hides it on 10.12).
1 comment:
Patch Set #2, Line 985:
if (IsOS10_9() || IsOS10_10()) {base::mac::IsAtMostOS10_10()
My presumption, based on the existence of the platform/mac/VersionUtilMac.h and its contents, was that it would not be appropriate for this code to rely upon /base/mac/mac_util.h.
If that's not the case, I'm happy to change it?
To view, visit change 610501. To unsubscribe, or for help writing mail filters, visit settings.
Kent Tamura (Gerrit)
1 comment:
Patch Set #2, Line 985:
if (IsOS10_9() || IsOS10_10()) {
My presumption, based on the existence of the platform/mac/VersionUtilMac. […]
We have a plan to use base:: in Blink core, but I think it's not ready yet. Using VersionUtilMac.h is safer.
To view, visit change 610501. To unsubscribe, or for help writing mail filters, visit settings.
Erik Chen (Gerrit)
Patch Set 2:
(1 comment)
got it thanks, still lgtm
Eric Lawrence (Gerrit)
Thanks, all!
Patch set 2:Code-Review +1Commit-Queue +2
1 comment:
Patch Set #2, Line 985:
if (IsOS10_9() || IsOS10_10()) {
We have a plan to use base:: in Blink core, but I think it's not ready yet. Using VersionUtilMac. […]
Ack
To view, visit change 610501. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
Commit Bot merged this change.
Suppress native placeholder text on MacOS Input Type=Search
The OS X 10.11 SDK introduced a change whereby the "Search" placeholder
text is no longer suppressed by setting setCenteredLook to NO on the
NSSearchFieldCell control. Instead, we now explicitly invoke
|setPlaceholderString| on the control.
Bug: 752362
Change-Id: I0da791fa3510d682731c5d64971ac669a7608d5a
Reviewed-on: https://chromium-review.googlesource.com/610501
Reviewed-by: Kent Tamura <tk...@chromium.org>
Reviewed-by: Eric Lawrence <elaw...@chromium.org>
Reviewed-by: Keishi Hattori <kei...@chromium.org>
Reviewed-by: Erik Chen <erik...@chromium.org>
Commit-Queue: Eric Lawrence <elaw...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495549}
---
M third_party/WebKit/Source/core/layout/LayoutThemeMac.mm
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/third_party/WebKit/Source/core/layout/LayoutThemeMac.mm b/third_party/WebKit/Source/core/layout/LayoutThemeMac.mm
index df13deb..49c01e1 100644
--- a/third_party/WebKit/Source/core/layout/LayoutThemeMac.mm
+++ b/third_party/WebKit/Source/core/layout/LayoutThemeMac.mm
@@ -977,17 +977,26 @@
[search_.Get() setBezeled:YES];
[search_.Get() setEditable:YES];
[search_.Get() setFocusRingType:NSFocusRingTypeExterior];
- SEL sel = @selector(setCenteredLook:);
- if ([search_.Get() respondsToSelector:sel]) {
- BOOL bool_value = NO;
- NSMethodSignature* signature =
- [NSSearchFieldCell instanceMethodSignatureForSelector:sel];
- NSInvocation* invocation =
- [NSInvocation invocationWithMethodSignature:signature];
- [invocation setTarget:search_.Get()];
- [invocation setSelector:sel];
- [invocation setArgument:&bool_value atIndex:2];
- [invocation invoke];
+
+ // Suppress NSSearchFieldCell's default placeholder text. Prior to OS10.11,
+ // this is achieved by calling |setCenteredLook| with NO. In OS10.11 and
+ // later, instead call |setPlaceholderString| with an empty string.
+ // See https://crbug.com/752362.
+ if (IsOS10_9() || IsOS10_10()) {
+ SEL sel = @selector(setCenteredLook:);
+ if ([search_.Get() respondsToSelector:sel]) {
+ BOOL bool_value = NO;
+ NSMethodSignature* signature =
+ [NSSearchFieldCell instanceMethodSignatureForSelector:sel];
+ NSInvocation* invocation =
+ [NSInvocation invocationWithMethodSignature:signature];
+ [invocation setTarget:search_.Get()];
+ [invocation setSelector:sel];
+ [invocation setArgument:&bool_value atIndex:2];
+ [invocation invoke];
+ }
+ } else {
+ [search_.Get() setPlaceholderString:@""];
}
}
To view, visit change 610501. To unsubscribe, or for help writing mail filters, visit settings.