[Webkit-unassigned] [Bug 17513] Add ability for image maps to be focused via tabbing

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=17513


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #47365|review?                     |review+
               Flag|                            |




--- Comment #11 from Darin Adler <darin at apple.com>  2010-01-25 17:14:32 PST ---
(From update of attachment 47365)
> +void HTMLAreaElement::dispatchBlurEvent()
> +{
> +    Node::dispatchBlurEvent();
> +    
> +    // On a blur, we might need to remove our focus rings by repainting.
> +    updateFocusAppearance(false);
> +}

If you are intentionally skipping base class implementations of
dispatchBlurEvent, then you should include a comment saying why.

If you are not, then the call should be to
HTMLAnchorElement::dispatchBlurEvent, rather than Node::dispatchBlurEvent, even
though it will end up calling the same function. In case someone overrides
later in between.

>  bool HTMLAreaElement::supportsFocus() const
>  {
> +    // As long as this is a link and the tabIndex was not explicitly set to -1.
> +    return isLink();
>  }

The comment here is inconsistent with the code. Please fix that by changing the
comment somehow. The comment should briefly explain both why isLink is the
right function to call and why we can't just use the version of the
supportsFocus function we inherit from the base class.

> +    String mapName = getName().string();

Since this is a member, I suggest just using m_name instead of getName(). And I
don't think you need a local variable for it.

> +    CGContextBeginTransparencyLayer(context, NULL);

WebKit code uses 0 instead of NULL.

> +    RetainPtr<CGColorRef> colorRef;
> +    if (color.isValid())
> +        colorRef.adoptCF(createCGColor(color));

We really should change createCGColor to return a RetainPtr.

> +        Path path = areaElement->getPath(this);
> +        Vector<Path> focusRingPaths;
> +        focusRingPaths.append(path);

I don't think we need the local variable "path" here.

r=me

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.