[WK2][Cocoa] Implement user interface for HTML form validation

[Chris Dumez]

Implement user interface for HTML form validation.

[Chris Dumez]

<rdar://problem/28944652>

[Chris Dumez]

Created attachment 293201 [details]
WIP Patch

[Build Bot]

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]

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]

Created attachment 293490 [details]
WIP patch

[WebKit Commit Bot]

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]

Created attachment 293493 [details]
WIP Patch

[WebKit Commit Bot]

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 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]

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 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]

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]

Created attachment 293522 [details]
WIP patch

[Chris Dumez]

Created attachment 293523 [details]
Patch

[Chris Dumez]

Created attachment 293587 [details]
Patch

[Chris Dumez]

Created attachment 293591 [details]
Patch

[Chris Dumez]

Created attachment 293608 [details]
Patch

[Chris Dumez]

Created attachment 293616 [details]
Patch

[Chris Dumez]

Created attachment 293618 [details]
Patch

[Chris Dumez]

Created attachment 293620 [details]
Patch

[Chris Dumez]

Created attachment 293634 [details]
Patch

[Chris Dumez]

Demo page: http://jsbin.com/butukewayu/edit?html,output

[Tim Horton]

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 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 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 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]

(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]

Created attachment 293637 [details]
Patch

[Tim Horton]

(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 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 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 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]

(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]

(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]

Created attachment 293665 [details]
Patch

[Chris Dumez]

Created attachment 293672 [details]
Patch

[Chris Dumez]

Created attachment 293720 [details]
Patch

[Chris Dumez]

Created attachment 293743 [details]
Patch

[Chris Dumez]

Created attachment 293744 [details]
Patch

[Chris Dumez]

(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 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]

Created attachment 293746 [details]
Patch

[Chris Dumez]

(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]

Created attachment 293787 [details]
Patch

[Simon Fraser (smfr)]

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 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 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 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]

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 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]

(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]

Created attachment 293809 [details]
Patch

[Chris Dumez]

Created attachment 293814 [details]
Patch

[Chris Dumez]

Created attachment 293825 [details]
Patch

[Chris Dumez]

Created attachment 293828 [details]
Patch

[Chris Dumez]

Created attachment 293844 [details]
Patch

[Chris Dumez]

Comment on attachment 293844 [details]
Patch

Clearing flags on attachment: 293844

Committed r208361: <http://trac.webkit.org/changeset/208361>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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