Bug 143527 – focus ring drawn at incorrect location on image map with CSS transform

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Chris Rebert]

Created attachment 250363 [details]
Top focus ring location is correct, Bottom focus ring within modal is wrong

Environment:
Safari Version 8.0.4 (10600.4.10.7)

Steps to reproduce:
1. Open http://jsbin.com/tufoye/2/edit
2. Resize the Output pane to be at least 800px wide.
3. Hover over the lower-left part of the gray rectangle where the mouse cursor becomes the gloved-hand pointer.
   (Which is to say that you're hovering over the image map's <area>.)
4. Click.
5. Note the location of the circular blue focus ring that appears.
6. Click the "Launch demo modal" button.
7. In the modal box that appears, click the same location in its gray rectangle image map.
8. Note the location of the circular blue focus ring that appears.

Expected result:
The locations of the focus rings in steps 5 & 8 should be the same relative to their respective gray rectangle image maps.

Actual result:
The location of the focus ring in step 8 is incorrect and differs from that of the focus ring in step 5.

Original Bootstrap bug report:
https://github.com/twbs/bootstrap/issues/16180

[Chris Rebert]

Corresponding Chrome/Blink bug:
https://code.google.com/p/chromium/issues/detail?id=475128

[Chris Rebert]

Created attachment 252501 [details]
Copy of JS Bin example

[Daniel Bates]

Created attachment 255651 [details]
Example

An example that demonstrates the issue.

[Daniel Bates]

Created attachment 255652 [details]
Example

[Simon Fraser (smfr)]

HTMLAreaElement::computePath() has:
    // FIXME: This doesn't work correctly with transforms.
    FloatPoint absPos = obj->localToAbsolute();

[Simon Fraser (smfr)]

HTMLAreaElement needs to not blithely return absolute paths; I think the caller should do the absolute conversion if they need to.

[Radar WebKit Bug Importer]

<rdar://problem/21908735>

[zalan]

Created attachment 275968 [details]
Patch

[Build Bot]

Comment on attachment 275968 [details]
Patch

Attachment 275968 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1118347

New failing tests:
fast/images/image-map-outline.html
fast/images/imagemap-focus-ring-zoom-style.html

[Build Bot]

Created attachment 275977 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[zalan]

Created attachment 275980 [details]
Patch

[Darin Adler]

Comment on attachment 275980 [details]
Patch

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

Looks like all non-Mac builds are failing.

> Source/WebCore/html/HTMLAreaElement.h:48
> +    Path pathForFocusRing(const LayoutSize& elementSize) const { return getRegion(m_shape == Default ? elementSize : m_lastSize); }

Does this really have to be inlined? I suggest defining it in the cpp file instead of here.

Can we paragraph this with computePath rather than with imageElement?

Why is this function named just “path” when the other function is named “computePath”?

> Source/WebCore/rendering/RenderImage.cpp:494
>      UNUSED_PARAM(paintInfo);

Need UNUSED_PARAM for paintOffset too to keep iOS building.

> Source/WebCore/rendering/RenderImage.cpp:510
> +    const auto* areaElementStyle = areaElement.computedStyle();

Why the const here? Needed?

> Source/WebCore/rendering/RenderImage.cpp:533
> +    paintInfo.context().drawFocusRing(path, document().page()->focusController().timeSinceFocusWasSet(), needsRepaint);

This overload of drawFocusRing is defined for PLATFORM(MAC) only, but this is platform independent code.

What guarantees that document().page() is not null?

[zalan]

Created attachment 276017 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 276017 [details]
Patch

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

> Source/WebCore/html/HTMLAreaElement.h:28
> +#include "Path.h"

Why do you need this include here?

> Source/WebCore/rendering/RenderImage.cpp:543
> +#if PLATFORM(MAC)
> +    bool needsRepaint;
> +    paintInfo.context().drawFocusRing(path, document().page()->focusController().timeSinceFocusWasSet(), needsRepaint);
> +    if (needsRepaint)
> +        document().page()->focusController().setFocusedElementNeedsRepaint();
> +#else
> +    paintInfo.context().drawFocusRing(path, outlineWidth, areaElementStyle->outlineOffset(), areaElementStyle->visitedDependentColor(CSSPropertyOutlineColor));
> +#endif

Maybe we can clean this up in a later patch.

> LayoutTests/fast/images/image-map-outline-in-positioned-container.html:24
> +    if (!window.eventSender)
> +        return;
> +    eventSender.mouseMoveTo(35, 35);
> +    eventSender.mouseDown();
> +    eventSender.mouseUp();

Maybe just use foo.focus().

[zalan]

Created attachment 276019 [details]
Patch

[Build Bot]

Comment on attachment 276019 [details]
Patch

Attachment 276019 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1121869

New failing tests:
fast/images/image-map-outline-with-scale-transform.html

[Build Bot]

Created attachment 276025 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[zalan]

Created attachment 276028 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 276028 [details]
Patch

Clearing flags on attachment: 276028

Committed r199247: <http://trac.webkit.org/changeset/199247>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Rebert]

Thanks!
https://github.com/twbs/bootstrap/commit/62c07b67bffd4af51de354299eac01d905323c03