RESOLVED FIXED 164143
[WK2][Cocoa] Implement user interface for HTML form validation
https://bugs.webkit.org/show_bug.cgi?id=164143
Summary [WK2][Cocoa] Implement user interface for HTML form validation
Chris Dumez
Reported 2016-10-28 13:36:36 PDT
Implement user interface for HTML form validation.
Attachments
WIP Patch (33.77 KB, patch)
2016-10-28 13:40 PDT, Chris Dumez
buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.75 MB, application/zip)
2016-10-28 15:04 PDT, Build Bot
no flags
WIP patch (47.84 KB, patch)
2016-10-31 16:33 PDT, Chris Dumez
no flags
WIP Patch (46.20 KB, patch)
2016-10-31 16:44 PDT, Chris Dumez
buildbot: commit-queue-
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.37 MB, application/zip)
2016-10-31 18:39 PDT, Build Bot
no flags
Archive of layout-test-results from ews123 for ios-simulator-wk2 (deleted)
2016-10-31 19:16 PDT, Build Bot
no flags
WIP patch (50.09 KB, patch)
2016-10-31 20:40 PDT, Chris Dumez
no flags
Patch (57.79 KB, patch)
2016-10-31 20:56 PDT, Chris Dumez
no flags
Patch (60.84 KB, patch)
2016-11-01 14:00 PDT, Chris Dumez
no flags
Patch (60.90 KB, patch)
2016-11-01 14:07 PDT, Chris Dumez
no flags
Patch (61.35 KB, patch)
2016-11-01 15:42 PDT, Chris Dumez
no flags
Patch (64.27 KB, patch)
2016-11-01 16:31 PDT, Chris Dumez
no flags
Patch (61.46 KB, patch)
2016-11-01 17:01 PDT, Chris Dumez
no flags
Patch (61.01 KB, patch)
2016-11-01 17:05 PDT, Chris Dumez
no flags
Patch (60.97 KB, patch)
2016-11-01 19:55 PDT, Chris Dumez
no flags
Patch (60.98 KB, patch)
2016-11-01 21:41 PDT, Chris Dumez
no flags
Patch (62.52 KB, patch)
2016-11-02 09:45 PDT, Chris Dumez
no flags
Patch (62.87 KB, patch)
2016-11-02 10:41 PDT, Chris Dumez
no flags
Patch (64.95 KB, patch)
2016-11-02 17:01 PDT, Chris Dumez
no flags
Patch (64.94 KB, patch)
2016-11-02 20:51 PDT, Chris Dumez
no flags
Patch (64.97 KB, patch)
2016-11-02 21:15 PDT, Chris Dumez
no flags
Patch (76.79 KB, patch)
2016-11-02 22:17 PDT, Chris Dumez
no flags
Patch (77.42 KB, patch)
2016-11-03 12:27 PDT, Chris Dumez
no flags
Archive of layout-test-results from ews124 for ios-simulator-wk2 (deleted)
2016-11-03 14:23 PDT, Build Bot
no flags
Patch (79.74 KB, patch)
2016-11-03 15:01 PDT, Chris Dumez
no flags
Patch (79.80 KB, patch)
2016-11-03 15:55 PDT, Chris Dumez
no flags
Patch (79.88 KB, patch)
2016-11-03 16:33 PDT, Chris Dumez
no flags
Patch (79.90 KB, patch)
2016-11-03 16:49 PDT, Chris Dumez
no flags
Patch (79.90 KB, patch)
2016-11-03 18:39 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2016-10-28 13:37:11 PDT
Chris Dumez
Comment 2 2016-10-28 13:40:21 PDT
Created attachment 293201 [details] WIP Patch
Build Bot
Comment 3 2016-10-28 15:04:13 PDT
Comment on attachment 293201 [details] WIP Patch Attachment 293201 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2395211 New failing tests: fast/forms/validation-message-on-listbox.html fast/forms/validation-message-on-menulist.html fast/forms/validation-message-on-radio.html fast/forms/validation-message-on-checkbox.html fast/forms/validation-message-on-range.html fast/forms/validation-message-clone.html fast/forms/validation-message-in-relative-body.html fast/forms/validation-message-appearance.html fast/forms/validation-message-on-textarea.html
Build Bot
Comment 4 2016-10-28 15:04:17 PDT
Created attachment 293226 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Chris Dumez
Comment 5 2016-10-31 16:33:20 PDT
Created attachment 293490 [details] WIP patch
WebKit Commit Bot
Comment 6 2016-10-31 16:36:23 PDT
Attachment 293490 [details] did not pass style-queue: ERROR: Source/WebCore/platform/ios/ValidationBubbleIOS.mm:36: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebCore/platform/ios/ValidationBubbleIOS.mm:51: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebCore/platform/ios/ValidationBubbleIOS.mm:58: Should have a space between // and comment [whitespace/comments] [4] ERROR: Source/WebCore/platform/ios/ValidationBubbleIOS.mm:59: Should have a space between // and comment [whitespace/comments] [4] Total errors found: 4 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Chris Dumez
Comment 7 2016-10-31 16:44:24 PDT
Created attachment 293493 [details] WIP Patch
WebKit Commit Bot
Comment 8 2016-10-31 16:47:33 PDT
Attachment 293493 [details] did not pass style-queue: ERROR: Source/WebCore/platform/ios/ValidationBubbleIOS.mm:41: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build Bot
Comment 9 2016-10-31 18:39:27 PDT
Comment on attachment 293493 [details] WIP Patch Attachment 293493 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2417582 New failing tests: fast/forms/validation-message-on-listbox.html fast/forms/validation-message-on-menulist.html fast/forms/validation-message-on-radio.html fast/forms/validation-message-on-checkbox.html fast/forms/validation-message-on-range.html fast/forms/validation-message-clone.html fast/forms/validation-message-in-relative-body.html fast/forms/validation-message-appearance.html svg/wicd/test-rightsizing-b.xhtml fast/forms/validation-message-on-textarea.html
Build Bot
Comment 10 2016-10-31 18:39:31 PDT
Created attachment 293509 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Build Bot
Comment 11 2016-10-31 19:16:31 PDT
Comment on attachment 293493 [details] WIP Patch Attachment 293493 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2417870 New failing tests: fast/forms/validation-message-on-listbox.html fast/forms/validation-message-on-menulist.html fast/forms/validation-message-on-radio.html fast/forms/validation-message-on-checkbox.html fast/forms/validation-message-on-range.html fast/forms/validation-message-on-textarea.html
Build Bot
Comment 12 2016-10-31 19:16:39 PDT
Created attachment 293515 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Chris Dumez
Comment 13 2016-10-31 20:40:12 PDT
Created attachment 293522 [details] WIP patch
Chris Dumez
Comment 14 2016-10-31 20:56:21 PDT
Chris Dumez
Comment 15 2016-11-01 14:00:06 PDT
Chris Dumez
Comment 16 2016-11-01 14:07:37 PDT
Chris Dumez
Comment 17 2016-11-01 15:42:34 PDT
Chris Dumez
Comment 18 2016-11-01 16:31:39 PDT
Chris Dumez
Comment 19 2016-11-01 17:01:55 PDT
Chris Dumez
Comment 20 2016-11-01 17:05:57 PDT
Chris Dumez
Comment 21 2016-11-01 19:55:44 PDT
Chris Dumez
Comment 22 2016-11-01 19:56:39 PDT
Tim Horton
Comment 23 2016-11-01 20:11:15 PDT
Comment on attachment 293634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293634&action=review > Source/WebCore/html/ValidationMessage.cpp:80 > + // On Mac and iOS, we hide the validation message rather than update it. This is a what comment, not a why, and I really want a why because this seems odd. > Source/WebCore/html/ValidationMessage.cpp:82 > + requestToHideMessage(); This all looks indented wrong? > Source/WebCore/platform/ValidationBubble.h:77 > + Timer m_delayedDisplayTimer; This looks like UI process code, so it probably can't use Timer? > Source/WebCore/platform/ValidationBubble.h:78 > + UIViewController* m_presentingViewController; Star's on the wrong side, and can you use UIKit from WebCore? Or, should you? > Source/WebCore/platform/ios/ValidationBubbleIOS.mm:59 > +static const unsigned displayDelayMS = 250; Wat > Source/WebCore/platform/ios/ValidationBubbleIOS.mm:78 > + [popoverView addConstraints:[NSLayoutConstraint constraintsWithVisualFormat:@"H:|-(8)-[textField]-(8)-|" options:0 metrics:nil views:@{@"textField" : label.get()}]]; Can we... not use autolayout? > Source/WebCore/platform/ios/ValidationBubbleIOS.mm:79 > + [popoverView addConstraints:[NSLayoutConstraint constraintsWithVisualFormat:@"V:|-(8)-[textField]-(8)-|" options:0 metrics:nil views:@{@"textField" : label.get()}]]; Do we ever use it anywhere else? > Source/WebCore/platform/ios/ValidationBubbleIOS.mm:83 > + CGSize popoverSize = [popoverView sizeThatFits:CGSizeMake(320, CGFLOAT_MAX)]; 320? > Source/WebCore/platform/mac/ValidationBubbleMac.mm:51 > + label.get().preferredMaxLayoutWidth = 200; Why randomly not square brackets? > Source/WebCore/platform/mac/ValidationBubbleMac.mm:60 > + [m_popover.get() setAnimates:NO]; No need for the get > Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:1019 > + m_validationBubble->showRelativeTo(anchorClientRect, uiClient().presentingViewController()); Have you tested this under page scale and whatnot? Also RTL. > LayoutTests/platform/ios-simulator-wk2/TestExpectations:1461 > +fast/forms/validation-message-on-listbox.html Are they ref tests? Can we not check in new results instead?
Chris Dumez
Comment 24 2016-11-01 20:56:01 PDT
Comment on attachment 293634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293634&action=review Thanks for the feedback. >> Source/WebCore/html/ValidationMessage.cpp:80 >> + // On Mac and iOS, we hide the validation message rather than update it. > > This is a what comment, not a why, and I really want a why because this seems odd. Oh, I'll update the comment. As soon as the user starts typing, we want to hide the validation message. The default behavior so far was to update the validation message if a constraint is still violated. We can probably factor this better too. >> Source/WebCore/html/ValidationMessage.cpp:82 >> + requestToHideMessage(); > > This all looks indented wrong? It is, will fix. >> Source/WebCore/platform/ValidationBubble.h:77 >> + Timer m_delayedDisplayTimer; > > This looks like UI process code, so it probably can't use Timer? Yes, this runs in the UIProcess on WebKit2. Why cannot we use a Timer in the UIProcess? It definitely works so is this a rule I don't know about? This is not the only place in the UIProcess we use a Timer either (although I may have added that one too): Source/WebKit2/UIProcess/Cocoa/NavigationState.h: WebCore::Timer m_releaseActivityTimer; If a Timer is not a preferred way of deferring work in the UIProcess, then can you tell me what is? >> Source/WebCore/platform/ValidationBubble.h:78 >> + UIViewController* m_presentingViewController; > > Star's on the wrong side, and can you use UIKit from WebCore? Or, should you? As I explained in the ChangeLog, I put this class in WebCore, under platform/, even though this is a UI class because I want to share it between WebKit2 and WebKit1 and avoid code duplication. What is the proper way of sharing such code between WebKit1 and WebKit2 if this is not appropriate? >> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:59 >> +static const unsigned displayDelayMS = 250; > > Wat What does "Wat" mean? >> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:78 >> + [popoverView addConstraints:[NSLayoutConstraint constraintsWithVisualFormat:@"H:|-(8)-[textField]-(8)-|" options:0 metrics:nil views:@{@"textField" : label.get()}]]; > > Can we... not use autolayout? Certainly, I may need help though as I am not familiar with this at all. >> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:83 >> + CGSize popoverSize = [popoverView sizeThatFits:CGSizeMake(320, CGFLOAT_MAX)]; > > 320? Inspired from: Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm: CGSize popoverSize = [_tableViewController.get().tableView sizeThatFits:CGSizeMake(320, CGFLOAT_MAX)]; We want to restrict the width (and let the text wrap) so it does not get too wide. >> Source/WebCore/platform/mac/ValidationBubbleMac.mm:51 >> + label.get().preferredMaxLayoutWidth = 200; > > Why randomly not square brackets? I'll use brackets. >> Source/WebCore/platform/mac/ValidationBubbleMac.mm:60 >> + [m_popover.get() setAnimates:NO]; > > No need for the get Will fix. >> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:1019 >> + m_validationBubble->showRelativeTo(anchorClientRect, uiClient().presentingViewController()); > > Have you tested this under page scale and whatnot? Also RTL. I have tried Zooming on Mac and iOS. Does zooming change page scale or do I need to do something else to test it. I haven't tried RTL yet. Do you mean that the message itself should be RTL (e.g. Arabic) or that the page itself should use RTL style? >> LayoutTests/platform/ios-simulator-wk2/TestExpectations:1461 >> +fast/forms/validation-message-on-listbox.html > > Are they ref tests? Can we not check in new results instead? No they're not ref tests, they actually go and get the bubble element in the Shadow DOM using the internals API and then do checks on the position of that bubble.
Chris Dumez
Comment 25 2016-11-01 21:03:10 PDT
Comment on attachment 293634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293634&action=review >>> Source/WebCore/platform/ValidationBubble.h:78 >>> + UIViewController* m_presentingViewController; >> >> Star's on the wrong side, and can you use UIKit from WebCore? Or, should you? > > As I explained in the ChangeLog, I put this class in WebCore, under platform/, even though this is a UI class because I want to share it between WebKit2 and WebKit1 and avoid code duplication. > What is the proper way of sharing such code between WebKit1 and WebKit2 if this is not appropriate? I know we are not allowed to link against UIKit in WebCore (because of cycles) but I am soft linking. There are a lot of other places in WebCore that do as well: Source/WebCore/editing/cocoa/HTMLConverter.mm:SOFT_LINK_FRAMEWORK(UIKit) Source/WebCore/page/cocoa/SettingsCocoa.mm:SOFT_LINK_FRAMEWORK(UIKit) Source/WebCore/page/ios/UserAgentIOS.mm:SOFT_LINK_FRAMEWORK(UIKit); Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:SOFT_LINK_FRAMEWORK(UIKit) Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm:SOFT_LINK_FRAMEWORK(UIKit) Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:SOFT_LINK_FRAMEWORK(UIKit) Source/WebCore/platform/ios/ThemeIOS.mm:SOFT_LINK_FRAMEWORK(UIKit) Source/WebCore/platform/ios/ValidationBubbleIOS.mm:SOFT_LINK_FRAMEWORK(UIKit) Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:SOFT_LINK_FRAMEWORK(UIKit) Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:SOFT_LINK_FRAMEWORK(UIKit) Source/WebCore/rendering/RenderThemeIOS.mm:SOFT_LINK_FRAMEWORK(UIKit)
Simon Fraser (smfr)
Comment 26 2016-11-01 21:15:14 PDT
Comment on attachment 293634 [details] Patch You should think about how to test this, and ideally add tests in this patch. I suggest using UIScriptController to do at least the following: * Ensure the bubble contents are what you expect (extend contentsOfUserInterfaceItem() for the bubble) * Ensure the bubble is in the correct location, with and without zooming * Ensure the bubble appears and disappears when you expect (maybe implement callbacks like didShowForcePressPreviewCallback/didDismissForcePressPreviewCallback)
Chris Dumez
Comment 27 2016-11-01 21:17:27 PDT
(In reply to comment #26) > Comment on attachment 293634 [details] > Patch > > You should think about how to test this, and ideally add tests in this > patch. I suggest using UIScriptController to do at least the following: > > * Ensure the bubble contents are what you expect (extend > contentsOfUserInterfaceItem() for the bubble) > * Ensure the bubble is in the correct location, with and without zooming > * Ensure the bubble appears and disappears when you expect (maybe implement > callbacks like > didShowForcePressPreviewCallback/didDismissForcePressPreviewCallback) I did not realize I could use UIScriptController for this. I'll look into it, thanks.
Chris Dumez
Comment 28 2016-11-01 21:41:46 PDT
Tim Horton
Comment 29 2016-11-01 22:19:35 PDT
(In reply to comment #24) > Yes, this runs in the UIProcess on WebKit2. Why cannot we use a Timer in the > UIProcess? It definitely works so is this a rule I don't know about? It works, until you have iOS WebKit1 and WebKit2 in the same UI process, and then bad things happen (because the WebKit1 timer heap is owned by the web thread, and the WebKit2 one by the main thread, but they're process-global... or some such). I think you'll find that most UI process code uses RunLoopTimer (or RunLoop::Timer? I don't remember.) for this reason, and we should fix any other ones you might have added :) > >> Source/WebCore/platform/ValidationBubble.h:78 > >> + UIViewController* m_presentingViewController; > > > > Star's on the wrong side, and can you use UIKit from WebCore? Or, should you? > > As I explained in the ChangeLog, I put this class in WebCore, under > platform/, even though this is a UI class because I want to share it between > WebKit2 and WebKit1 and avoid code duplication. > What is the proper way of sharing such code between WebKit1 and WebKit2 if > this is not appropriate? Well, for one I think there should be a chunk of code between WebCore and the WebKits for things like this, but for now, you're right, it goes in WebCore. I know you're soft linking, but you should check with Dan/Anders/Sam, because they have yelled at me in the past for introducing new UIKit dependencies in WebCore (even soft ones). > >> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:59 > >> +static const unsigned displayDelayMS = 250; > > > > Wat > > What does "Wat" mean? Heh, sorry. It means "where did the 250 come from! and is there something better we can do?". > >> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:78 > >> + [popoverView addConstraints:[NSLayoutConstraint constraintsWithVisualFormat:@"H:|-(8)-[textField]-(8)-|" options:0 metrics:nil views:@{@"textField" : label.get()}]]; > > > > Can we... not use autolayout? > > Certainly, I may need help though as I am not familiar with this at all. This is something I can help with. > >> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:83 > >> + CGSize popoverSize = [popoverView sizeThatFits:CGSizeMake(320, CGFLOAT_MAX)]; > > > > 320? > > Inspired from: > Source/WebKit2/UIProcess/ios/forms/WKFormSelectPopover.mm: CGSize > popoverSize = [_tableViewController.get().tableView > sizeThatFits:CGSizeMake(320, CGFLOAT_MAX)]; > > We want to restrict the width (and let the text wrap) so it does not get too > wide. Makes sense. It's the original iPhone screen width; I wonder if we should name it and put it somewhere. Or if it should be based on the current available screen width (consider... iPad, for example... or Mac). > >> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:1019 > >> + m_validationBubble->showRelativeTo(anchorClientRect, uiClient().presentingViewController()); > > > > Have you tested this under page scale and whatnot? Also RTL. > > I have tried Zooming on Mac and iOS. Does zooming change page scale or do I > need to do something else to test it. Yep, pinch-zoom is what I meant. > I haven't tried RTL yet. Do you mean that the message itself should be RTL > (e.g. Arabic) or that the page itself should use RTL style? Well, both, but what I really meant was whether the popover shows up targeting the right place in an RTL page (non-zero scroll origin throws a lot of things off... that said, it should be OK, but always good to test). > >> LayoutTests/platform/ios-simulator-wk2/TestExpectations:1461 > >> +fast/forms/validation-message-on-listbox.html > > > > Are they ref tests? Can we not check in new results instead? > > No they're not ref tests, they actually go and get the bubble element in the > Shadow DOM using the internals API and then do checks on the position of > that bubble. Oh dear. OK.
Chris Dumez
Comment 30 2016-11-01 22:28:18 PDT
Comment on attachment 293634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293634&action=review >>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:59 >>> +static const unsigned displayDelayMS = 250; >> >> Wat > > What does "Wat" mean? 250ms is the duration of the scroll into view animation on iOS when focusing a field.
Simon Fraser (smfr)
Comment 31 2016-11-01 22:32:41 PDT
Comment on attachment 293637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293637&action=review > Source/WebCore/platform/ios/ValidationBubbleIOS.mm:110 > + m_delayedDisplayTimer.startOneShot(bubbleDisplayDelay); You should not hardcode this delay. Instead, you should listen for the end of the scroll.
Wenson Hsieh
Comment 32 2016-11-02 07:35:02 PDT
Comment on attachment 293637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293637&action=review > Source/WebCore/platform/ios/ValidationBubbleIOS.mm:76 > + [popoverView.get() addSubview:label.get()]; I don't think get() is necessary here. > Source/WebKit2/WebProcess/WebCoreSupport/WebValidationMessageClient.h:47 > + const WebCore::Element* m_currentAnchor { nullptr }; Can an Element be destroyed from under the WebValidationMessageClient?
Chris Dumez
Comment 33 2016-11-02 09:04:34 PDT
(In reply to comment #32) > Comment on attachment 293637 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293637&action=review > > > Source/WebCore/platform/ios/ValidationBubbleIOS.mm:76 > > + [popoverView.get() addSubview:label.get()]; > > I don't think get() is necessary here. Will fix. > > > Source/WebKit2/WebProcess/WebCoreSupport/WebValidationMessageClient.h:47 > > + const WebCore::Element* m_currentAnchor { nullptr }; > > Can an Element be destroyed from under the WebValidationMessageClient? HTMLFormControlElement owns ValidationMessage (using unique_ptr<>). ValidationMessage is the one interacting with the ValidationMessageClient and asking for a message to be shown/hidden for its associated element. When the element gets destroyed, so does its ValidationMessage object. The ValidationMessage destructor takes care of calling hideValidationMessage() on the client which will reset WebValidationMessageClient::m_currentAnchor to null. Therefore, I think lifetime management is OK here.
Chris Dumez
Comment 34 2016-11-02 09:05:56 PDT
(In reply to comment #31) > Comment on attachment 293637 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293637&action=review > > > Source/WebCore/platform/ios/ValidationBubbleIOS.mm:110 > > + m_delayedDisplayTimer.startOneShot(bubbleDisplayDelay); > > You should not hardcode this delay. Instead, you should listen for the end > of the scroll. I'll look into this. I considered it but it seems it was going to be a lot more intrusive and also a bit more complicated because there is not always a scroll animation.
Chris Dumez
Comment 35 2016-11-02 09:45:34 PDT
Chris Dumez
Comment 36 2016-11-02 10:41:40 PDT
Chris Dumez
Comment 37 2016-11-02 17:01:38 PDT
Chris Dumez
Comment 38 2016-11-02 20:51:37 PDT
Chris Dumez
Comment 39 2016-11-02 21:15:23 PDT
Chris Dumez
Comment 40 2016-11-02 21:17:03 PDT
(In reply to comment #31) > Comment on attachment 293637 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293637&action=review > > > Source/WebCore/platform/ios/ValidationBubbleIOS.mm:110 > > + m_delayedDisplayTimer.startOneShot(bubbleDisplayDelay); > > You should not hardcode this delay. Instead, you should listen for the end > of the scroll. Done.
Chris Dumez
Comment 41 2016-11-02 21:19:33 PDT
Comment on attachment 293634 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293634&action=review >>> Source/WebCore/platform/ValidationBubble.h:77 >>> + Timer m_delayedDisplayTimer; >> >> This looks like UI process code, so it probably can't use Timer? > > Yes, this runs in the UIProcess on WebKit2. Why cannot we use a Timer in the UIProcess? It definitely works so is this a rule I don't know about? > > This is not the only place in the UIProcess we use a Timer either (although I may have added that one too): > Source/WebKit2/UIProcess/Cocoa/NavigationState.h: WebCore::Timer m_releaseActivityTimer; > > If a Timer is not a preferred way of deferring work in the UIProcess, then can you tell me what is? Fixed. I no longer need a Timer now that I listen for the scroll event and wait for the scroll to finish before displaying the validation message. >>>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:59 >>>> +static const unsigned displayDelayMS = 250; >>> >>> Wat >> >> What does "Wat" mean? > > 250ms is the duration of the scroll into view animation on iOS when focusing a field. Fixed, no longer needed. >>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:78 >>> + [popoverView addConstraints:[NSLayoutConstraint constraintsWithVisualFormat:@"H:|-(8)-[textField]-(8)-|" options:0 metrics:nil views:@{@"textField" : label.get()}]]; >> >> Can we... not use autolayout? > > Certainly, I may need help though as I am not familiar with this at all. I no longer use auto layout in my latest iteration on both Mac and iOS.
Chris Dumez
Comment 42 2016-11-02 22:17:56 PDT
Chris Dumez
Comment 43 2016-11-02 22:19:42 PDT
(In reply to comment #26) > Comment on attachment 293634 [details] > Patch > > You should think about how to test this, and ideally add tests in this > patch. I suggest using UIScriptController to do at least the following: > > * Ensure the bubble contents are what you expect (extend > contentsOfUserInterfaceItem() for the bubble) I have started doing this and it seems to work Ok. I need to add more tests but at least the testing approach works.
Chris Dumez
Comment 44 2016-11-03 12:27:29 PDT
Simon Fraser (smfr)
Comment 45 2016-11-03 13:32:54 PDT
Comment on attachment 293787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293787&action=review > Source/WebCore/ChangeLog:18 > + > + * WebCore.xcodeproj/project.pbxproj: This should list some tests? Maybe need to regenerate the changelogs > Source/WebCore/html/HTMLFormControlElement.cpp:523 > +#if !PLATFORM(IOS) > + // The form control is already scrolled into view on iOS when we call focus() below. > scrollIntoViewIfNeeded(false); > +#endif Platform #ifdefs like this are nasty. Can we do something with an enum (kinda like SelectionRevealMode) instead? > Source/WebCore/html/ValidationMessage.cpp:79 > +#if PLATFORM(COCOA) Why an #ifdef? > Source/WebCore/platform/ValidationBubble.h:50 > +using PlatformWebView = NSView; > +#elif PLATFORM(IOS) > +OBJC_CLASS UIView; > +using PlatformWebView = UIView; > +#else > +using PlatformWebView = void; "PlatformWebView" is misleading, since you are just typedeffing them to views. Is it *the* web view, or just any view? > Source/WebCore/platform/ios/ValidationBubbleIOS.mm:60 > +static const unsigned horizontalPadding = 8; > +static const unsigned verticalPadding = 8; > +static const unsigned maxLabelWidth = 300; These should be CGFloats > Source/WebCore/platform/ios/ValidationBubbleIOS.mm:75 > + [label setNumberOfLines:0]; // No limit. What if the text is huge? > Source/WebCore/platform/mac/ValidationBubbleMac.mm:39 > +static const unsigned horizontalPadding = 5; > +static const unsigned verticalPadding = 5; > +static const unsigned maxLabelWidth = 300; > + CGFloats > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2183 > + _page->setIsAboutToShowKeyboard(true); Boolean trap! Use an enum. > Source/WebKit2/UIProcess/WebPageProxy.h:1106 > + void setIsAboutToShowKeyboard(bool isAboutToShowKeyboard) { m_isAboutToShowKeyboard = isAboutToShowKeyboard; } It's unclear what the duration of "about to show" is. Maybe call this "keyboardIsAnimatingIn()"?
Chris Dumez
Comment 46 2016-11-03 13:54:22 PDT
Comment on attachment 293787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293787&action=review Thanks for reviewing. >> Source/WebCore/ChangeLog:18 >> + * WebCore.xcodeproj/project.pbxproj: > > This should list some tests? Maybe need to regenerate the changelogs Will regenerate. >> Source/WebCore/html/HTMLFormControlElement.cpp:523 >> +#endif > > Platform #ifdefs like this are nasty. Can we do something with an enum (kinda like SelectionRevealMode) instead? Ok, I will look into this before landing. >> Source/WebCore/html/ValidationMessage.cpp:79 >> +#if PLATFORM(COCOA) > > Why an #ifdef? Ok, I will remove this for now. If other ports want to enable this feature and update the message as the user types, we can always introduce a setting later on. >> Source/WebCore/platform/ValidationBubble.h:50 >> +using PlatformWebView = void; > > "PlatformWebView" is misleading, since you are just typedeffing them to views. Is it *the* web view, or just any view? The client is passing both the anchor rect and the view in which the rect coordinates are from. How about simply PlatformView? >> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:60 >> +static const unsigned maxLabelWidth = 300; > > These should be CGFloats Ok. >> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:75 >> + [label setNumberOfLines:0]; // No limit. > > What if the text is huge? Yes, this is on my lists of issues to address before we can ship this. At the moment, the feature is disabled at runtime. I'll file a bugzilla bug to track this issue. >> Source/WebCore/platform/mac/ValidationBubbleMac.mm:39 >> + > > CGFloats Ok. >> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2183 >> + _page->setIsAboutToShowKeyboard(true); > > Boolean trap! Use an enum. Is it though? There is a single parameter and based on the name of the method, it is pretty clear what the true / false means at call sites IMO. >> Source/WebKit2/UIProcess/WebPageProxy.h:1106 >> + void setIsAboutToShowKeyboard(bool isAboutToShowKeyboard) { m_isAboutToShowKeyboard = isAboutToShowKeyboard; } > > It's unclear what the duration of "about to show" is. Maybe call this "keyboardIsAnimatingIn()"? Ok.
Chris Dumez
Comment 47 2016-11-03 14:06:01 PDT
Comment on attachment 293787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293787&action=review >>> Source/WebCore/html/HTMLFormControlElement.cpp:523 >>> +#endif >> >> Platform #ifdefs like this are nasty. Can we do something with an enum (kinda like SelectionRevealMode) instead? > > Ok, I will look into this before landing. SelectionRevealMode sort of does the same thing unfortunately: SelectionRevealMode revealMode = SelectionRevealMode::Reveal; #if PLATFORM(IOS) // Focusing a form element triggers animation in UIKit to scroll to the right position. // Calling updateFocusAppearance() would generate an unnecessary call to ScrollView::setScrollPosition(), // which would jump us around during this animation. See <rdar://problem/6699741>. bool isFormControl = is<HTMLFormControlElement>(*this); if (isFormControl) revealMode = SelectionRevealMode::RevealUpToMainFrame; #endif
Build Bot
Comment 48 2016-11-03 14:23:18 PDT
Comment on attachment 293787 [details] Patch Attachment 293787 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2456325 New failing tests: fast/forms/validation-messages.html
Build Bot
Comment 49 2016-11-03 14:23:24 PDT
Created attachment 293801 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Chris Dumez
Comment 50 2016-11-03 14:25:10 PDT
Comment on attachment 293787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293787&action=review >>>> Source/WebCore/html/HTMLFormControlElement.cpp:523 >>>> +#endif >>> >>> Platform #ifdefs like this are nasty. Can we do something with an enum (kinda like SelectionRevealMode) instead? >> >> Ok, I will look into this before landing. > > SelectionRevealMode sort of does the same thing unfortunately: > SelectionRevealMode revealMode = SelectionRevealMode::Reveal; > #if PLATFORM(IOS) > // Focusing a form element triggers animation in UIKit to scroll to the right position. > // Calling updateFocusAppearance() would generate an unnecessary call to ScrollView::setScrollPosition(), > // which would jump us around during this animation. See <rdar://problem/6699741>. > bool isFormControl = is<HTMLFormControlElement>(*this); > if (isFormControl) > revealMode = SelectionRevealMode::RevealUpToMainFrame; > #endif It looks like we can actually drop the call to scrollIntoViewIfNeeded() altogether. On Mac, the call to focus() also scrolls the element into view already.
Chris Dumez
Comment 51 2016-11-03 14:34:11 PDT
(In reply to comment #48) > Comment on attachment 293787 [details] > Patch > > Attachment 293787 [details] did not pass ios-sim-ews (ios-simulator-wk2): > Output: http://webkit-queues.webkit.org/results/2456325 > > New failing tests: > fast/forms/validation-messages.html iOS does not support eventSender.keyDown which is used for one of the form controls. I'll try and find an alternative that works on both Mac and iOS.
Chris Dumez
Comment 52 2016-11-03 15:01:21 PDT
Chris Dumez
Comment 53 2016-11-03 15:55:09 PDT
Chris Dumez
Comment 54 2016-11-03 16:33:40 PDT
Chris Dumez
Comment 55 2016-11-03 16:49:19 PDT
Chris Dumez
Comment 56 2016-11-03 18:39:38 PDT
Chris Dumez
Comment 57 2016-11-03 18:41:32 PDT
Comment on attachment 293844 [details] Patch Clearing flags on attachment: 293844 Committed r208361: <http://trac.webkit.org/changeset/208361>
Chris Dumez
Comment 58 2016-11-03 18:41:42 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 59 2016-11-09 09:47:45 PST
(In reply to comment #57) > Comment on attachment 293844 [details] > Patch > > Clearing flags on attachment: 293844 > > Committed r208361: <http://trac.webkit.org/changeset/208361> It broke the Apple Mac cmake build, because WebValidationMessageClient.cpp wasn't added to the cmake build system. Fix landed in https://trac.webkit.org/changeset/208432 and https://trac.webkit.org/changeset/208433
Note You need to log in before you can comment on or make changes to this bug.