Bug 27880 - HTML 5: Support click() method on HTMLElement
Summary: HTML 5: Support click() method on HTMLElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 65215
  Show dependency treegraph
 
Reported: 2009-07-31 07:48 PDT by Antoine Quint
Modified: 2012-02-11 10:52 PST (History)
17 users (show)

See Also:


Attachments
Proposed patch (10.73 KB, patch)
2011-11-17 05:37 PST, Arko Saha
no flags Details | Formatted Diff | Diff
Updated patch (12.15 KB, patch)
2011-11-18 02:32 PST, Arko Saha
abarth: review+
Details | Formatted Diff | Diff
Updated patch (14.38 KB, patch)
2012-02-11 01:11 PST, Arko Saha
no flags Details | Formatted Diff | Diff
Updated patch (11.79 KB, patch)
2012-02-11 03:25 PST, Arko Saha
no flags Details | Formatted Diff | Diff
Updated patch (12.49 KB, patch)
2012-02-11 04:03 PST, Arko Saha
timothy: review+
timothy: commit-queue-
Details | Formatted Diff | Diff
Updated patch (11.81 KB, patch)
2012-02-11 09:07 PST, Arko Saha
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.