Bug 27880

Summary: HTML 5: Support click() method on HTMLElement
Product: WebKit Reporter: Antoine Quint <ml>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andersca, arko, bharathwaaj.s, bugmail, cmarcelo, darin, ericbidelman, eric, ian, ojan, paulirish, sam, timothy, tkent, tonikitoo, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 65215    
Attachments:
Description Flags
Proposed patch
none
Updated patch
abarth: review+
Updated patch
none
Updated patch
none
Updated patch
timothy: review+, timothy: commit-queue-
Updated patch none

Description Antoine Quint 2009-07-31 07:48:48 PDT
The HTML 5  spec adds the click() method on all HTML elements, WebKit does not support it yet.
Comment 1 Adam Roben (:aroben) 2009-07-31 07:50:54 PDT
<rdar://problem/7108579>
Comment 2 Arko Saha 2011-11-17 05:37:59 PST
Created attachment 115571 [details]
Proposed patch
Comment 3 Adam Barth 2011-11-17 10:23:47 PST
Comment on attachment 115571 [details]
Proposed patch

Does this patch build on Mac?  I'm wondering if we need to change the public ObjC interfaces header.
Comment 4 Arko Saha 2011-11-18 02:32:25 PST
Created attachment 115771 [details]
Updated patch

Incorporating review comments. Also verified the patch on apple-mac port.
Comment 5 Adam Barth 2011-11-18 10:31:25 PST
Comment on attachment 115771 [details]
Updated patch

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

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:-421
> -- (void)click AVAILABLE_WEBKIT_VERSION_3_0_AND_LATER;

Looks like we lost the version attribute.  Do we need to keep it?
Comment 6 Adam Barth 2011-11-18 10:32:21 PST
Comment on attachment 115771 [details]
Updated patch

This change looks good to me.  Please double-check with some Apple folks about the version attribute in the public ObjC DOM bindings.
Comment 7 Arko Saha 2011-11-18 12:04:57 PST
Hi Sam/Darin,
Can you please check the patch once.
Here I removed the click() method of HTMLButton element from the public ObjC DOM bindings. It has a version attribute. Can you please check do we need to keep it?
Comment 8 Eric Seidel (no email) 2011-12-21 15:34:35 PST
xenon has historically been arbiter of the obj-c bindings.
Comment 9 Timothy Hatcher 2011-12-21 15:40:30 PST
Comment on attachment 115771 [details]
Updated patch

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

>> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:-421
>> -- (void)click AVAILABLE_WEBKIT_VERSION_3_0_AND_LATER;
> 
> Looks like we lost the version attribute.  Do we need to keep it?

This needs a new availability macro. And click likely should stay on the old classes with the old macro. This same thing came up with accessKey. See: https://bugs.webkit.org/show_bug.cgi?id=72756
Comment 10 Arko Saha 2012-02-11 01:11:36 PST
Created attachment 126624 [details]
Updated patch
Comment 11 WebKit Review Bot 2012-02-11 01:13:56 PST
Please wait for approval from timothy@apple.com (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.
Comment 12 Eric Seidel (no email) 2012-02-11 01:21:28 PST
Comment on attachment 126624 [details]
Updated patch

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

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:421
> +- (void)click AVAILABLE_WEBKIT_VERSION_3_0_AND_LATER_BUT_DEPRECATED_AFTER_WEBKIT_VERSION_5_1;

Are you sure you have to do this this way?  I doubt that any Obj-c program cares if -click is exposed on these subclasses or on the baseclass. Since you're adding it to HTMLElement, things should "just work" and I would expect we could remove this w/o deprecation warnings.
Comment 13 Arko Saha 2012-02-11 01:38:27 PST
Thanks Eric for the review.

(In reply to comment #12)
> (From update of attachment 126624 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126624&action=review
> 
> > Source/WebCore/bindings/objc/PublicDOMInterfaces.h:421
> > +- (void)click AVAILABLE_WEBKIT_VERSION_3_0_AND_LATER_BUT_DEPRECATED_AFTER_WEBKIT_VERSION_5_1;
> 
> Are you sure you have to do this this way?  I doubt that any Obj-c program cares if -click is exposed on these subclasses or on the baseclass. Since you're adding it to HTMLElement, things should "just work" and I would expect we could remove this w/o deprecation warnings.

Ok, so if we keep the -click in HTMLButtonElement and HTMLInputElement with old macro will it work on mac? Do we need to keep the method under LANGUAGE_OBJECTIVE_C in .idl files.
Comment 14 Arko Saha 2012-02-11 03:25:25 PST
Created attachment 126625 [details]
Updated patch

Incorporating Eric's review comments.
Comment 15 Arko Saha 2012-02-11 04:03:06 PST
Created attachment 126626 [details]
Updated patch

Resolving Mac build error.
Comment 16 Timothy Hatcher 2012-02-11 07:17:12 PST
Comment on attachment 126626 [details]
Updated patch

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

I supose you should do the same for accessKey then?

> LayoutTests/ChangeLog:54
> -        * platform/chromium-mac-snowleopard/svg/filters/filter-refresh-expected.txt:
> +        *youtTests/ChangeLog platform/chromium-mac-snowleopard/svg/filters/filter-refresh-expected.txt:

Please fix this before landing.
Comment 17 Arko Saha 2012-02-11 09:07:05 PST
Created attachment 126637 [details]
Updated patch
Comment 18 Arko Saha 2012-02-11 09:09:42 PST
Thanks Timothy for the review.

(In reply to comment #16)
> (From update of attachment 126626 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126626&action=review
> 
> I supose you should do the same for accessKey then?

Sure, I will fix the same for accessKey. Shall I log a new bug for this?
 
> > LayoutTests/ChangeLog:54
> > -        * platform/chromium-mac-snowleopard/svg/filters/filter-refresh-expected.txt:
> > +        *youtTests/ChangeLog platform/chromium-mac-snowleopard/svg/filters/filter-refresh-expected.txt:
> 
> Please fix this before landing.

Done!
Comment 19 Timothy Hatcher 2012-02-11 09:19:13 PST
(In reply to comment #18)
> Thanks Timothy for the review.
> 
> (In reply to comment #16)
> > (From update of attachment 126626 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=126626&action=review
> > 
> > I supose you should do the same for accessKey then?
> 
> Sure, I will fix the same for accessKey. Shall I log a new bug for this?

The old bug is fine.
Comment 20 WebKit Review Bot 2012-02-11 10:52:09 PST
Comment on attachment 126637 [details]
Updated patch

Clearing flags on attachment: 126637

Committed r107483: <http://trac.webkit.org/changeset/107483>
Comment 21 WebKit Review Bot 2012-02-11 10:52:17 PST
All reviewed patches have been landed.  Closing bug.