Bug 80795 – Remove vestigal abortEvent from image attribute.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gavin Peters]

Remove vestigal abortEvent from image attribute.

[Gavin Peters]

Created attachment 131256 [details]
Patch

[Adam Barth]

Comment on attachment 131256 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        It seems we installed a listener for the abort event on images,
> +        but never ever raised them.  So this patch removes it.  This could
> +        cause different behaviour if the user sends abort events directly
> +        at an image element, although addEventListener will still work.

Do we need to remove any onabort attributes from the HTMLImageElement.idl ?  Do we understand why this code was added to WebKit?  Do other browsers let you install "abort" handlers using the onabort attribute (e.g., to receive custom abort events)?

[Gavin Peters]

(In reply to comment #2)
> (From update of attachment 131256 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131256&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        It seems we installed a listener for the abort event on images,
> > +        but never ever raised them.  So this patch removes it.  This could
> > +        cause different behaviour if the user sends abort events directly
> > +        at an image element, although addEventListener will still work.
> 
> Do we need to remove any onabort attributes from the HTMLImageElement.idl ?

No.  The onabort event handler comes from HTMLElement.idl; lots of other events don't set up an abortEvent listener without problem.  See HTMLScriptElement.

> Do we understand why this code was added to WebKit?

No.  The line was last editted in 2009 by eseidel when reorganising the source tree, and carried this over.  I believe it to be completely vestigal, but I haven't rooted back farther than that.

>  Do other browsers let you install "abort" handlers using the onabort attribute (e.g., to receive custom abort events)?

I believe my comment above was premature above.  I'm having a lot of trouble even constructing an event exception, and throwing it at an element.  So now I'm not so sure I've changed behaviour.

[Adam Barth]

> > Do we understand why this code was added to WebKit?
> 
> No.  The line was last editted in 2009 by eseidel when reorganising the source tree, and carried this over.  I believe it to be completely vestigal, but I haven't rooted back farther than that.

It would probably be worth digging back further in the history to understand why it was added in the first place.

> >  Do other browsers let you install "abort" handlers using the onabort attribute (e.g., to receive custom abort events)?
> 
> I believe my comment above was premature above.  I'm having a lot of trouble even constructing an event exception, and throwing it at an element.  So now I'm not so sure I've changed behaviour.

You should be able to test this by creating an event, initializing with an eventType of "abort" and calling dispatchEvent at the appropriate element.

[Gavin Peters]

Indeed, I was able to create and dispatch events in Firefox and Chrome for comparison.

Firefox seems to install handlers on all elements.  I successfully sent an abort event to a <script> in Firefox, and it ran the onabort code.  Then I got silly, and tried it with a <b>, and that worked too.  The onabort attr was installed as an handler.

We only install onabort handlers from the attribute in HTMLImageElement, HTMLMediaElement and SVGSVGElement.

In Media & SVG elements, we actually raise abort events sometimes.  In Image, we do not.

So either way, we're being inconsistant.  Either we should install them on everything ala Firefox, or only install them where we fire the event ourselves.  I'm not sure which is better.

[Gavin Peters]

As for the history, this handler goes back to the initial revision. Behold WebCore/khtml/html/html_imageimpl.cpp as of 2001-08-24.

166	    case ATTR_ONABORT: // ### add support for this
167	        setHTMLEventListener(EventImpl::ABORT_EVENT,
168	            ownerDocument()->createHTMLEventListener(attr->value().string()));
169	        break;
170	    case ATTR_ONERROR: // ### add support for this
171	        setHTMLEventListener(EventImpl::ERROR_EVENT,
172	            ownerDocument()->createHTMLEventListener(attr->value().string()));
173	        break;
174	    case ATTR_ONLOAD:
175	        setHTMLEventListener(EventImpl::LOAD_EVENT,
176	            ownerDocument()->createHTMLEventListener(attr->value().string()));
177	        break;

In revision @2855 (2002-11-24), onerror was implemented, and that comment was removed, as that changeset sent out the error event from the resource on notifyFinished().

In 9824 in 2005-07-18, a refactoring changed events from being an enumeration to having an attribute class, and the comment was removed:

@@ -227,51 +213,40 @@
         }
         m_isLink = !attr->isNull();
-    case ATTR_ISMAP:
+    } else if (attr->name() == HTMLAttributes::ismap()) {
         ismap = true;
-        break;
-    case ATTR_ONABORT: // ### add support for this
+    } else if (attr->name() == HTMLAttributes::onabort()) {
         setHTMLEventListener(EventImpl::ABORT_EVENT,
-            getDocument()->createHTMLEventListener(attr->value().string(), this));
-        break;
-    case ATTR_ONERROR:
+                             getDocument()->createHTMLEventListener(attr->value().string(), this));
+    } else if (attr->name() == HTMLAttributes::onerror()) {
         setHTMLEventListener(EventImpl::ERROR_EVENT,
-            getDocument()->createHTMLEventListener(attr->value().string(), this));
-        break;

9824 didn't add any causes of abort, and it was a pretty big change (patch is ~35,000 lines).

[Adam Barth]

Comment on attachment 131256 [details]
Patch

Sounds like this is just vestigial.  In principle, we could add a test showing that it's not there, but I don't should add that test so we don't bias future developers against adding the event (if they want to use it for something).

[WebKit Review Bot]

Comment on attachment 131256 [details]
Patch

Clearing flags on attachment: 131256

Committed r110618: <http://trac.webkit.org/changeset/110618>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.