Bug 185260 - Some fields are not identified as [WKWebProcessPlugInNodeHandle isTextField]
Summary: Some fields are not identified as [WKWebProcessPlugInNodeHandle isTextField]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: Safari 11
Hardware: All iOS 11
: P1 Major
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-03 10:52 PDT by Richard Houle
Modified: 2018-05-09 13:31 PDT (History)
9 users (show)

See Also:


Attachments
Proposed fix. Instead of of using HTMLInputElement::isText, use HTMLInputElement::isTextElement. (721 bytes, patch)
2018-05-03 11:02 PDT, Richard Houle
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.75 MB, application/zip)
2018-05-03 12:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews202 for win-future (12.82 MB, application/zip)
2018-05-03 20:38 PDT, Build Bot
no flags Details
Proposed fix + API test (14.63 KB, patch)
2018-05-08 10:23 PDT, Richard Houle
rniwa: review+
Details | Formatted Diff | Diff
V2 Proposed fix + API test (15.01 KB, patch)
2018-05-08 17:29 PDT, Richard Houle
thorton: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
V3 Proposed fix + API test (16.53 KB, patch)
2018-05-09 12:40 PDT, Richard Houle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Houle 2018-05-03 10:52:31 PDT
<rdar://problem/39290394> Some fields are not identified as [WKWebProcessPlugInNodeHandle isTextField]

INPUT element are not considered as text field when calling -[WKWebProcessPlugInNodeHandle isTextField] when they have attribute 'type="number"'.
Comment 1 Richard Houle 2018-05-03 11:02:40 PDT
Created attachment 339429 [details]
Proposed fix. Instead of of using HTMLInputElement::isText, use HTMLInputElement::isTextElement.
Comment 2 Richard Houle 2018-05-03 11:03:56 PDT
Daniel Bates pointed me that this bug was forseen in a FIXME comment in HTMLInputElement.h:

[[
    // FIXME: It's highly likely that any call site calling this function should instead
    // be using a different one. Many input elements behave like text fields, and in addition
    // any unknown input type is treated as text. Consider, for example, isTextField or
    // isTextField && !isPasswordField.
    WEBCORE_EXPORT bool isText() const;
]]

<https://trac.webkit.org/browser/trunk/Source/WebCore/html/HTMLInputElement.h?rev=231291#L112>
Comment 3 Richard Houle 2018-05-03 11:14:19 PDT
Raising to P1, to match Safari's BRB priority.
Comment 4 Build Bot 2018-05-03 12:37:24 PDT
Comment on attachment 339429 [details]
Proposed fix. Instead of of using HTMLInputElement::isText, use HTMLInputElement::isTextElement.

Attachment 339429 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7553526

New failing tests:
fast/mediastream/delayed-permission-allowed.html
Comment 5 Build Bot 2018-05-03 12:37:25 PDT
Created attachment 339442 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 6 Build Bot 2018-05-03 20:38:38 PDT
Comment on attachment 339429 [details]
Proposed fix. Instead of of using HTMLInputElement::isText, use HTMLInputElement::isTextElement.

Attachment 339429 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7559903

New failing tests:
http/tests/preload/onload_event.html
Comment 7 Build Bot 2018-05-03 20:38:50 PDT
Created attachment 339516 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 8 annabell 2018-05-03 21:21:19 PDT
This is exactly what I’ve been looking for https://www.gmaillogin-wiki.com

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 9 Ryosuke Niwa 2018-05-04 00:38:03 PDT
Can we add an API test for this?
Comment 10 Brent Fulgham 2018-05-04 08:28:58 PDT
These test failures do not seem to be related to this patch.
Comment 11 Richard Houle 2018-05-08 10:23:00 PDT
Created attachment 339828 [details]
Proposed fix + API test
Comment 12 Brent Fulgham 2018-05-08 13:54:23 PDT
Comment on attachment 339828 [details]
Proposed fix + API test

View in context: https://bugs.webkit.org/attachment.cgi?id=339828&action=review

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=185260

<rdar://problem/39290394>

> Source/WebKit/ChangeLog:6
> +        INPUT element are not considered as text field when

are not considered TO BE text fields when

> Tools/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=185260

<rdar://problem/39290394>
Comment 13 Ryosuke Niwa 2018-05-08 14:14:40 PDT
Comment on attachment 339828 [details]
Proposed fix + API test

View in context: https://bugs.webkit.org/attachment.cgi?id=339828&action=review

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IsTextField.mm:57
> +    

Nit: trailing whitespace.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IsTextField.mm:66
> +    

Ditto.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IsTextField.mm:68
> +    BOOL results = YES;
> +    results &= [self isTextFieldForHTMLInputType:@"date" document:document jsContext:jsContext];

This would make it impossible to tell which test case had failed when they do.
Please generate an assert for each test case instead.
Alternatively, you can use WKBundlePostSynchronousMessage.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/UIDelegate.mm:628
> +TEST(WebKit, IsTextField)

This is rather a generic name. How about InjectedBundleNodeHandleIsTextField?
Comment 14 Ryosuke Niwa 2018-05-08 14:15:15 PDT
Comment on attachment 339828 [details]
Proposed fix + API test

View in context: https://bugs.webkit.org/attachment.cgi?id=339828&action=review

> Tools/TestWebKitAPI/Tests/WebKitCocoa/UIDelegate.mm:631
> +    

Nit: Trailing whitespace.
Comment 15 Richard Houle 2018-05-08 17:29:46 PDT
Created attachment 339907 [details]
V2 Proposed fix + API test
Comment 16 Jessie Berlin 2018-05-08 17:37:45 PDT
Comment on attachment 339907 [details]
V2 Proposed fix + API test

Setting commit-queue+ since Richard isn't a WebKit committer (yet).
Comment 17 WebKit Commit Bot 2018-05-08 17:51:01 PDT
Comment on attachment 339907 [details]
V2 Proposed fix + API test

Rejecting attachment 339907 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ntext = [[browserContextController mainFrame] jsContextForWorld:[WKWebProcessPlugInScriptWorld normalWorld]];
          ^
2 errors generated.

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/TestWebKitAPI.build/Release/WebProcessPlugIn.build/Objects-normal/x86_64/InjectedBundleNodeHandleIsTextField.o Tests/WebKitCocoa/InjectedBundleNodeHandleIsTextField.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/7617874
Comment 18 Richard Houle 2018-05-09 12:40:59 PDT
Created attachment 340004 [details]
V3 Proposed fix + API test
Comment 19 WebKit Commit Bot 2018-05-09 13:31:19 PDT
Comment on attachment 340004 [details]
V3 Proposed fix + API test

Clearing flags on attachment: 340004

Committed r231591: <https://trac.webkit.org/changeset/231591>
Comment 20 WebKit Commit Bot 2018-05-09 13:31:21 PDT
All reviewed patches have been landed.  Closing bug.