Bug 17513 – Add ability for image maps to be focused via tabbing

• Component: Accessibility
• Status: RESOLVED FIXED

[Conner Mo]

In IE/Firefox, image map can be TAB focused with outline rendered. It behaves differently under WebKit based browsers.

[Gérard Talbot (no longer involved)]

Conner, even with Firefox, because of the black background of the image, it's difficult to see the outline focus (dotted black dots on a black background)

May I suggest this other testcase (which passes markup validation):
http://www.mozilla.org/access/qa/tiny/map_area.html

(adding my vote on this bug)

[Gérard Talbot (no longer involved)]

Latest HTML 5 specification states:

"One way that a user agent can enable users to follow hyperlinks is by allowing area elements to be clicked, or focussed and activated by the keyboard. This will cause the aforementioned activation behavior to be invoked."
http://www.whatwg.org/specs/web-apps/current-work/#coords

"User agents may also allow individual area elements representing hyperlinks to be selected and activated (e.g. using a keyboard)"
http://www.whatwg.org/specs/web-apps/current-work/#image-map

Tab-focusing to focus-select (moving from and to) individual areas 
in the webpage
http://www.mozilla.org/access/qa/tiny/map_area.html
works well for Firefox 2, Firefox 3, IE 7, IE 8 b1 and very well in Opera 9.50 build 9981

[Nicholas Shanks]

Created attachment 26787 [details]
test case

To test this, make sure you have enabled tabbing through all elements on a page, not just form fields.

[Deirdre Saoirse Moen]

<rdar://problem/6502465>

[Gérard Talbot (no longer involved)]

Deirdre Saoirse Moen, 
I believe 
http://www.mozilla.org/access/qa/tiny/map_area.html
meets quality and reduction criteria. Is keyword NeedsReduction still required? Just asking..

Regards, Gérard

[chris fleizach]

Created attachment 47250 [details]
Patch

[chris fleizach]

just added the patch.

this is my first attempt to go into renderer land, so not sure if everything is done the right way

the basic problem is that HTMLAreaElements don't have renderers, so they are automatically left out of the focus loop.

Adding them back in required to override a few of the isFocusable() type methods

Then to make it draw the highlight, i overrode some painting methods in RenderImage to check if it had an image map with an active link.

If so, it would draw the focus rect

Since area elements don't have to be rects, I had to modify drawFocusRects to also be able to take arbitrary Paths. 

Seems to work pretty well as far as I can tell. It passes all of the various test cases i've seen

[Darin Adler]

Comment on attachment 47250 [details]
Patch

> +        if (reinterpret_cast<AccessibilityImageMapLink*>(child)->areaElement() == areaElement)

This should be static_cast, not reinterpret_cast. We should almost never use a reinterpret_cast.

> +        return focusedImageMapUIElement(reinterpret_cast<HTMLAreaElement*>(focusedNode));

Ditto.

> +    static AccessibilityObject* focusedImageMapUIElement(HTMLAreaElement* areaElement);

This argument should not have a name.

> +    virtual bool isImageMapLink() const { return true; }

This sort of function should be private, because it’s a programming error to call it if you have a pointer of the specific type. 

> +    return reinterpret_cast<HTMLMapElement*>(mapElement)->imageElement();

static_cast

> +void HTMLAreaElement::updateFocusAppearance(bool restorePreviousSelection)
> +{
> +    Node* parent = parentNode();
> +    if (parent && parent->hasTagName(mapTag)) {
> +        HTMLImageElement* imageElement = reinterpret_cast<HTMLMapElement*>(parent)->imageElement();
> +
> +        // This will handle scrolling to the image if necessary.
> +        imageElement->updateFocusAppearance(restorePreviousSelection);
> +        
> +        if (imageElement) {
> +            RenderObject* imageRenderer = imageElement->renderer();
> +            if (imageRenderer)
> +                imageRenderer->setNeedsLayout(true);
> +        }
> +    }
> +}

We normally prefer early returns rather than nesting the function inside if statements.

The cast needs to be a static_cast.

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

Are you sure this can’t just be a call to tabIndex()? As far as I can tell it should give you the same value, and the code would be less strange.

I’m not sure this is the right rule. Will this change tabbing for all documents? Why is this different from the rule for links?

> +    Path getPath(RenderObject*) const;

Normally we don’t use the word “get” in the names of functions that return a value. We reserve it for functions that return values in unusual ways, with out arguments. I think the name of the function is not clear enough. Sure it's a path, but what does the path represent?

> +    String mapName = getName().string().lower();
> +    RefPtr<HTMLCollection> coll = renderer()->document()->images();
> +    for (Node* curr = coll->firstItem(); curr; curr = coll->nextItem()) {
> +        if (!curr->hasTagName(imgTag))
> +            continue;
> +        
> +        // The HTMLImageElement's useMap() value includes the '#' symbol at the beginning,
> +        // which has to be stripped off.
> +        HTMLImageElement* imageElement = static_cast<HTMLImageElement*>(curr);
> +        String useMapName = imageElement->getAttribute(usemapAttr).string().substring(1).lower();
> +        if (useMapName == mapName)
> +            return imageElement;
> +    }

I know this is not new code, but it seems strange to call lower on both strings instead of equalIgnoringCase.

> +    Document* doc = mapElement->document();
> +    if (!doc)
> +        return;

Please use the name "document" for the local variable instead of the abbreviation "doc".

> +    // FIXME: Clip the paths to the image bounding box.

Why aren't you doing that? Seems easy to do.

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

You don't need the local variable "path" here.

> +    virtual void paint(PaintInfo& paintInfo, int tx, int ty);

I would make this private unless there's a reason not to. Or protected at least.

Please omit the argument name "paintInfo".

> +    void paintFocusRings(PaintInfo&, const RenderStyle* style);

Please omit the argument name "style".

> -            graphicsContext->drawFocusRing(focusRingRects, ow, style()->outlineOffset(), oc);
> +            graphicsContext->drawFocusRingRects(focusRingRects, ow, style()->outlineOffset(), oc);

C++ has overloading, so you don't have to rename the function. You can just provide two versions, one that works with a vector of paths and the other that works with a vector of rects.

> +    friend class RenderImage;

This friendship seems really unfortunate. Can we avoid this somehow?

Did we discuss whether the change in tabbing behavior is good all the time? Did you talk to anyone else who works on WebKit about that change?

[chris fleizach]

Thanks for your comments Darin. Will address everything not mentioned here. Please see the below.  

> >  bool HTMLAreaElement::supportsFocus() const
> >  {
> > -    // Skip HTMLAnchorElements isLink() check.
> > -    return HTMLElement::supportsFocus();
> > +    // As long as this is a link and the tabIndex was not explicitly set to -1.
> > +    return isLink() && Node::tabIndex() != -1;
> >  }
> 
> Are you sure this can’t just be a call to tabIndex()? As far as I can tell it
> should give you the same value, and the code would be less strange.
> 

An AREA element might also have a nohref attribute or just no link. in that case, it should not be focusable or support focus.

> I’m not sure this is the right rule. Will this change tabbing for all
> documents? Why is this different from the rule for links?
> 

you're right. looks like the tabIndex is taken care of in Node::nextFocusableNode

> > +    Path getPath(RenderObject*) const;
> 
> Normally we don’t use the word “get” in the names of functions that return a
> value. We reserve it for functions that return values in unusual ways, with out
> arguments. I think the name of the function is not clear enough. Sure it's a
> path, but what does the path represent?

This method was right below 
IntRect getRect(RenderObject*), so was following convention. 

Do you think

getPathBoundary() is better. 
It's the path of the area element, since an area can be a polygon, circle, etc. Drawing a blank on other ideas. Perhaps a better comment?


> > +    // FIXME: Clip the paths to the image bounding box.
> 
> Why aren't you doing that? Seems easy to do.

Looked into it and its a lot harder than it sounds when you just have a CGPathRef. ImageKit takes a lot of code to implement IKPathClipToRect.


> Did we discuss whether the change in tabbing behavior is good all the time? Did
> you talk to anyone else who works on WebKit about that change?

There has been no discussion on this. I have assumed that this was just a bug, since you can always tab to links and these are links. This behavior also works in all other browsers. Should I bring this up on webkit-dev?

[chris fleizach]

Created attachment 47365 [details]
patch

Added patch to address all issues Darin mentioned, aside from things I noted in my response.

[Darin Adler]

Comment on attachment 47365 [details]
patch

> +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

[chris fleizach]

bombs away
http://trac.webkit.org/changeset/53857

[chris fleizach]

looks like tab wrap around behavior is different on gtk