Bug 208472 – [iOS] Ugly and misaligned form validation bubble

• Component: Forms
• Status: RESOLVED FIXED

[Ferdy Christant]

Created attachment 392191 [details]
Screenshot of validation tooltip

See screenshot. When using HTML5 native validation on form elements, the browser will use a tool-tip to alert users of invalid values.

In the case of iOS Safari 13 (only version I tested), the tool-tip is rendered very poorly. Specifically, the tool-tip is quite high with the validation message being top-aligned, whereas it should be vertically middle aligned.

Although this seems a minor issue, I do find it very annoying, because I was aiming to go for a pure HTML5 validation solution without any script involved. The tooltips generally look OK across browsers, yet mobile Safari is way off, it seems.

[Chris Dumez]

*** Bug 208474 has been marked as a duplicate of this bug. ***

[Radar WebKit Bug Importer]

<rdar://problem/59984027>

[Wenson Hsieh]

Created attachment 395484 [details]
Fixes the bug

[Wenson Hsieh]

Created attachment 395485 [details]
Screenshot showing MDN (fixed)

[Tim Horton]

Comment on attachment 395484 [details]
Fixes the bug

This is fairly horrifying. I love it.

[Wenson Hsieh]

Comment on attachment 395484 [details]
Fixes the bug

(In reply to Tim Horton from comment #5)
> Comment on attachment 395484 [details]
> Fixes the bug
> 
> This is fairly horrifying. I love it.

Horrifying indeed 😅

[EWS]

Committed r259550: <https://trac.webkit.org/changeset/259550>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395484 [details].

[Daniel Bates]

Reopening to attach new patch.

[Daniel Bates]

Created attachment 395512 [details]
To Land

[Darin Adler]

Comment on attachment 395484 [details]
Fixes the bug

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

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:43
> +static const CGFloat validationBubbleHorizontalPadding = 17;
> +static const CGFloat validationBubbleVerticalPadding = 9;
> +static const CGFloat validationBubbleMaxLabelWidth = 300;

New code -> constexpr for this sort of thing

Touch code -> add comments explaining why we have hard-coded constants?

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:72
> +    label.numberOfLines = 4;

Magic number here, without obvious provenance. Should it have a named constant too? And a comment explaining its value?

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:91
> +static CGRect WebValidationBubbleViewController_labelFrame(WebValidationBubbleViewController *instance, SEL)

Does this have to be a method? Can we just use a C function for this instead?

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:97
> +static UILabel *WebValidationBubbleViewController_label(WebValidationBubbleViewController *instance, SEL)

Does this have to be a method? Can we just use a C function for this instead?

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:114
> +        class_addIvar(theClass, "_label", sizeof(UILabel *), log2(sizeof(UILabel *)), "@");

It’s messy to do this successfully with an instance variable. A simpler and safer way to do may be to use objc_set/getAssociatedObject. One good thing about that is that you can use OBJC_ASSOCIATION_RETAIN_NONATOMIC and then there’s no need to release the object in the dealloc function. In fact, might be able to avoid overriding dealloc altogether.

I think if you did that you could also just entirely avoid the key/value stuff as well.

[Wenson Hsieh]

Comment on attachment 395484 [details]
Fixes the bug

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

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:43
>> +static const CGFloat validationBubbleMaxLabelWidth = 300;
> 
> New code -> constexpr for this sort of thing
> 
> Touch code -> add comments explaining why we have hard-coded constants?

Updated to use constexpr.

These constants existed before the patch; I just moved them up to the top of the file.
Perhaps Chris recalls the details behind these specific numbers; I’ll attempt to add comments explaining their purpose.

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:72
>> +    label.numberOfLines = 4;
> 
> Magic number here, without obvious provenance. Should it have a named constant too? And a comment explaining its value?

This was preexisting as well; presumably, it was added to keep the validation bubble message from being too tall.

I’ve grouped it together with the rest of the constants above, as a constexpr.

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:91
>> +static CGRect WebValidationBubbleViewController_labelFrame(WebValidationBubbleViewController *instance, SEL)
> 
> Does this have to be a method? Can we just use a C function for this instead?

Sure, I can make this a C function.

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:97
>> +static UILabel *WebValidationBubbleViewController_label(WebValidationBubbleViewController *instance, SEL)
> 
> Does this have to be a method? Can we just use a C function for this instead?

Ditto.

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:114
>> +        class_addIvar(theClass, "_label", sizeof(UILabel *), log2(sizeof(UILabel *)), "@");
> 
> It’s messy to do this successfully with an instance variable. A simpler and safer way to do may be to use objc_set/getAssociatedObject. One good thing about that is that you can use OBJC_ASSOCIATION_RETAIN_NONATOMIC and then there’s no need to release the object in the dealloc function. In fact, might be able to avoid overriding dealloc altogether.
> 
> I think if you did that you could also just entirely avoid the key/value stuff as well.

Changed to use associated objects.

[Wenson Hsieh]

Created attachment 395526 [details]
Patch

[Darin Adler]

Comment on attachment 395526 [details]
Patch

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

Thanks. Looks good! I really appreciate you considering my suggestions.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:41
> +// Add a bit of vertical and horizontal padding between the
> +// label and its parent view.

Would be even better if we somehow pointed out that these numbers were carefully chosen to achieve a particular look.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:47
> +// Avoid making the validation bubble too wide by enforcing a
> +// maximum width on the content size of the validation bubble
> +// view controller.

Great reasoning to have a constant, not clear where the 300 comes from, though. Same question: Did we carefully choose this?

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:51
> +// Avoid making the validation bubble too tall by truncating
> +// the label to a maximum of 4 lines.

OK, now I am a broken record. I really want these to be "why" comments so the new person coming along doesn’t just change them because they think they know better.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:57
> +static void *validationBubbleViewControllerLabelKey = &validationBubbleViewControllerLabelKey;

Our coding style calls for void*, not "void *". Also, since the key is const void* (not void*), we use this better version:

    static const void* const validationBubbleViewControllerLabelKey = &validationBubbleViewControllerLabelKey;

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:59
> +inline UILabel *webValidationBubbleViewControllerLabel(WebValidationBubbleViewController *instance)

I would just say "static" here instead of "inline". The compiler should do a good job of inlining if appropriate without our help. We only really need to use "inline" if we are putting something into a header and need permission to include it in more than one translation unit.

Also, static makes sure it’s private to a single translation unit, but inline instead makes it safe to have copies in multiple translation units. We want the former.

Making it private to a single translation unit also lets us take the risk of using slightly shorter function names without as much fear of conflicts, although unified sources bring that fear back. I’d love for these to have shorter, but still clear, names. Since this is C++, the argument type participates in overload resolution, so the name of the class does not need to be included in the function name, particularly not the full name of the class.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:64
> +inline CGRect webValidationBubbleViewControllerLabelFrame(WebValidationBubbleViewController *instance)

Ditto.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:70
> +inline void invokeUIViewControllerSelector(id instance, SEL selector)

Ditto. Except that I would not change the name of this one. Unless …

You *could* change the name to callSuper or sendSuper or something like that and change the argument type to WebValidationBubbleViewController * specifically, instead of "id".

C++ function overloading can lead to much clearer function names with nearly the same safety about getting the right function. If you were doing this for more than one class, all the functions could still be named callSuper, and overloading would choose the correct one.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:72
>      objc_super superClass { instance, PAL::getUIViewControllerClass() };

The name "superClass" here is not really quite right. First of all, the term of art is a single word "superclass". Second, an objc_super structure is not a superclass. It’s a structure to indicate how to search for a method for a super call, that can call to a superclass or to one of its superclasses. So I would name this something different, like maybe just "super".

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:73
>      auto superclassFunction = reinterpret_cast<void(*)(objc_super*, SEL)>(objc_msgSendSuper);

I don’t think superclassFunction is the perfect name for a casted copy of objc_msgSendSuper. It’s the dispatcher, not an actual function from the superclass, which is what superclassFunction sounds like to me. I think this and the next line would read better as a one-liner. Our you could name this msgSendSuper, or castedMsgSentSuper, or msgSendSuperForNoArgumentsOrReturnValue.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:82
>  static void WebValidationBubbleViewController_dealloc(WebValidationBubbleViewController *instance, SEL)
>  {
> -    [instance.label release];
> -    [instance setValue:nil forKey:@"_label"];
> +    objc_setAssociatedObject(instance, validationBubbleViewControllerLabelKey, nil, OBJC_ASSOCIATION_RETAIN_NONATOMIC);
>  
>      invokeUIViewControllerSelector(instance, @selector(dealloc));
>  }

This should not be needed; associated objects should already have their lifetimes tied to the object they are associated with. Does something go wrong if we don’t override dealloc?

[Wenson Hsieh]

Comment on attachment 395526 [details]
Patch

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

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:41
>> +// label and its parent view.
> 
> Would be even better if we somehow pointed out that these numbers were carefully chosen to achieve a particular look.

Okay. This was (presumably) chosen to avoid laying out the label against the edges of the view controller’s content view, so I’ll make that clear in this comment.

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:47
>> +// view controller.
> 
> Great reasoning to have a constant, not clear where the 300 comes from, though. Same question: Did we carefully choose this?

It’s not 100% clear to me either. This constant was added in r208361, along with the initial implementation of form validation UI on iOS.

Chris, do you recall if this was part of a design specification, or just a constant chosen to avoid letting the validation bubble grow too wide (e.g. on iPad)?

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:51
>> +// the label to a maximum of 4 lines.
> 
> OK, now I am a broken record. I really want these to be "why" comments so the new person coming along doesn’t just change them because they think they know better.

Again, this was added as a part of the initial implementation (r208361), and seems like a reasonable line limit to have for this UI.

Chris, do you recall how this value was determined?

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:57
>> +static void *validationBubbleViewControllerLabelKey = &validationBubbleViewControllerLabelKey;
> 
> Our coding style calls for void*, not "void *". Also, since the key is const void* (not void*), we use this better version:
> 
>     static const void* const validationBubbleViewControllerLabelKey = &validationBubbleViewControllerLabelKey;

Fixed.

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:59
>> +inline UILabel *webValidationBubbleViewControllerLabel(WebValidationBubbleViewController *instance)
> 
> I would just say "static" here instead of "inline". The compiler should do a good job of inlining if appropriate without our help. We only really need to use "inline" if we are putting something into a header and need permission to include it in more than one translation unit.
> 
> Also, static makes sure it’s private to a single translation unit, but inline instead makes it safe to have copies in multiple translation units. We want the former.
> 
> Making it private to a single translation unit also lets us take the risk of using slightly shorter function names without as much fear of conflicts, although unified sources bring that fear back. I’d love for these to have shorter, but still clear, names. Since this is C++, the argument type participates in overload resolution, so the name of the class does not need to be included in the function name, particularly not the full name of the class.

Alright — changed this back to a static function, and renamed to just `label(WebValidationBubbleViewController *controller)`

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:64
>> +inline CGRect webValidationBubbleViewControllerLabelFrame(WebValidationBubbleViewController *instance)
> 
> Ditto.

Changed this back to a static function, and renamed to just `labelFrame(WebValidationBubbleViewController *controller)`

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:70
>> +inline void invokeUIViewControllerSelector(id instance, SEL selector)
> 
> Ditto. Except that I would not change the name of this one. Unless …
> 
> You *could* change the name to callSuper or sendSuper or something like that and change the argument type to WebValidationBubbleViewController * specifically, instead of "id".
> 
> C++ function overloading can lead to much clearer function names with nearly the same safety about getting the right function. If you were doing this for more than one class, all the functions could still be named callSuper, and overloading would choose the correct one.

Renamed to `callSuper` with `WebValidationBubbleViewController *`, and changed it back to being a static function.

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:72
>>      objc_super superClass { instance, PAL::getUIViewControllerClass() };
> 
> The name "superClass" here is not really quite right. First of all, the term of art is a single word "superclass". Second, an objc_super structure is not a superclass. It’s a structure to indicate how to search for a method for a super call, that can call to a superclass or to one of its superclasses. So I would name this something different, like maybe just "super".

I can’t name it just super, since that’s a reserved keyword in Objective C.

How about superStructure?

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:73
>>      auto superclassFunction = reinterpret_cast<void(*)(objc_super*, SEL)>(objc_msgSendSuper);
> 
> I don’t think superclassFunction is the perfect name for a casted copy of objc_msgSendSuper. It’s the dispatcher, not an actual function from the superclass, which is what superclassFunction sounds like to me. I think this and the next line would read better as a one-liner. Our you could name this msgSendSuper, or castedMsgSentSuper, or msgSendSuperForNoArgumentsOrReturnValue.

Sounds good — renamed to `msgSendSuper`.

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:82
>>  }
> 
> This should not be needed; associated objects should already have their lifetimes tied to the object they are associated with. Does something go wrong if we don’t override dealloc?

Oops, good catch — I forgot that I could just remove this override.

Fixed!

[Wenson Hsieh]

> > This should not be needed; associated objects should already have their lifetimes tied to the object they are associated with. Does something go wrong if we don’t override dealloc?
> 
> Oops, good catch — I forgot that I could just remove this override.
> 
> Fixed!

(I also verified with `leaks` on device that we don’t leak the label view as a result of taking this out).

[Wenson Hsieh]

Created attachment 395531 [details]
Patch

[Darin Adler]

Comment on attachment 395531 [details]
Patch

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

This patch looks amazing to me now. I think it’s likely more maintainable than what we had before.

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:94
> +    label(instance).frame = labelFrame(instance);

One last refactor that could tighten this up a little more. Could replace the function labelFrame with one named updateLabelFrame, and move this last line of code into it. There’s no other caller that needs to just "get" the label’s frame.

[Wenson Hsieh]

Comment on attachment 395531 [details]
Patch

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

>> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:94
>> +    label(instance).frame = labelFrame(instance);
> 
> One last refactor that could tighten this up a little more. Could replace the function labelFrame with one named updateLabelFrame, and move this last line of code into it. There’s no other caller that needs to just "get" the label’s frame.

Sounds good to me!

[Wenson Hsieh]

Created attachment 395532 [details]
Patch for landing

[Darin Adler]

Comment on attachment 395532 [details]
Patch for landing

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

> Source/WebCore/platform/ios/ValidationBubbleIOS.mm:-113
> -        class_addMethod(theClass, @selector(label), (IMP)WebValidationBubbleViewController_label, "v@:");
> -        class_addMethod(theClass, @selector(labelFrame), (IMP)WebValidationBubbleViewController_labelFrame, "v@:");

Looking back on it, I think the "v@:" was wrong for these methods since they had a non-void return value; maybe "@@:" for the first and something else for the second? So it’s good we are deleting this code.

[EWS]

Committed r259559: <https://trac.webkit.org/changeset/259559>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395532 [details].