Bug 203440 – Add bindings support for the enterkeyhint HTML attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Wenson Hsieh]

Work towards enterkeyhint support.

[Wenson Hsieh]

Created attachment 382036 [details]
Patch

[Wenson Hsieh]

Created attachment 382069 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 382069 [details]
Patch

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

> Source/WebCore/html/EnterKeyHint.cpp:35
> +static const AtomString& enterKeyHintNameEnter()
> +{
> +    static NeverDestroyed<AtomString> name("enter", AtomString::ConstructFromLiteral);

I don't think these strings should be AtomString.
We're always doing case insensitive comparison, and we're just returning for IDL.
There is virtually no benefit for these rarely used strings to be AtomString.

> Source/WebCore/html/HTMLElement.idl:68
> +    attribute DOMString enterKeyHint;

We need a runtime flag for this.
e.g. I don't think it would be supported on any port but Apple's mobile ports, right?
Does desktop version of Chrome support this attribute as well?

[Wenson Hsieh]

Comment on attachment 382069 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/html/EnterKeyHint.cpp:35
>> +    static NeverDestroyed<AtomString> name("enter", AtomString::ConstructFromLiteral);
> 
> I don't think these strings should be AtomString.
> We're always doing case insensitive comparison, and we're just returning for IDL.
> There is virtually no benefit for these rarely used strings to be AtomString.

Okay! I’ll change these back to regular string literals.

>> Source/WebCore/html/HTMLElement.idl:68
>> +    attribute DOMString enterKeyHint;
> 
> We need a runtime flag for this.
> e.g. I don't think it would be supported on any port but Apple's mobile ports, right?
> Does desktop version of Chrome support this attribute as well?

Yes, the desktop version of Chrome supports this property (i.e. it is present in IDL, and reflects the enumerated attribute values per specification). However, specifying it doesn’t result in any behavioral difference, since there is no virtual keyboard on desktop.

It seems reasonable to match this behavior; I’ll add a runtime switch for enterKeyHint, and enable it by default on Cocoa platforms, for now.

[Wenson Hsieh]

Created attachment 382092 [details]
Patch

[Wenson Hsieh]

Created attachment 382096 [details]
Review feedback + new baselines

[Wenson Hsieh]

Created attachment 382112 [details]
Rebase on trunk

[WebKit Commit Bot]

Comment on attachment 382112 [details]
Rebase on trunk

Clearing flags on attachment: 382112

Committed r251686: <https://trac.webkit.org/changeset/251686>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56690697>

[Alexey Proskuryakov]

This commit added platform/mac-wk2/imported/w3c/web-platform-tests/html/dom/idlharness.https.html, which now shows up as a test with missing results. I don't think that it was supposed to be landed at all.

[Wenson Hsieh]

(In reply to Alexey Proskuryakov from comment #11)
> This commit added
> platform/mac-wk2/imported/w3c/web-platform-tests/html/dom/idlharness.https.
> html, which now shows up as a test with missing results. I don't think that
> it was supposed to be landed at all.

My bad — this was supposed to be a test expectation. Fixing it now.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #12)
> (In reply to Alexey Proskuryakov from comment #11)
> > This commit added
> > platform/mac-wk2/imported/w3c/web-platform-tests/html/dom/idlharness.https.
> > html, which now shows up as a test with missing results. I don't think that
> > it was supposed to be landed at all.
> 
> My bad — this was supposed to be a test expectation. Fixing it now.

Replaced the erroneous test with a test expectation in https://trac.webkit.org/r251705.