Bug 143527 - focus ring drawn at incorrect location on image map with CSS transform
Summary: focus ring drawn at incorrect location on image map with CSS transform
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Macintosh Intel OS X 10.11
: P2 Normal
Assignee: zalan
URL: http://jsbin.com/tufoye/2/edit
Keywords: HasReduction, InRadar
Depends on:
Blocks: 159753
  Show dependency treegraph
 
Reported: 2015-04-08 11:22 PDT by Chris Rebert
Modified: 2016-08-29 23:50 PDT (History)
14 users (show)

See Also:


Attachments
Top focus ring location is correct, Bottom focus ring within modal is wrong (90.01 KB, image/png)
2015-04-08 11:22 PDT, Chris Rebert
no flags Details
Copy of JS Bin example (1.79 KB, text/html)
2015-05-06 11:47 PDT, Chris Rebert
no flags Details
Example (964 bytes, text/html)
2015-06-26 11:16 PDT, Daniel Bates
no flags Details
Example (962 bytes, text/html)
2015-06-26 11:17 PDT, Daniel Bates
no flags Details
Patch (17.64 KB, patch)
2016-04-07 20:13 PDT, zalan
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (858.56 KB, application/zip)
2016-04-07 21:00 PDT, Build Bot
no flags Details
Patch (16.25 KB, patch)
2016-04-07 21:29 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (17.68 KB, patch)
2016-04-08 11:05 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (17.12 KB, patch)
2016-04-08 11:31 PDT, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (638.20 KB, application/zip)
2016-04-08 12:26 PDT, Build Bot
no flags Details
Patch (17.96 KB, patch)
2016-04-08 12:59 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rebert 2015-04-08 11:22:35 PDT
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
Comment 1 Chris Rebert 2015-04-08 11:36:00 PDT
Corresponding Chrome/Blink bug:
https://code.google.com/p/chromium/issues/detail?id=475128
Comment 2 Chris Rebert 2015-05-06 11:47:11 PDT
Created attachment 252501 [details]
Copy of JS Bin example
Comment 3 Daniel Bates 2015-06-26 11:16:28 PDT
Created attachment 255651 [details]
Example

An example that demonstrates the issue.
Comment 4 Daniel Bates 2015-06-26 11:17:57 PDT
Created attachment 255652 [details]
Example
Comment 5 Simon Fraser (smfr) 2015-06-26 11:52:44 PDT
HTMLAreaElement::computePath() has:
    // FIXME: This doesn't work correctly with transforms.
    FloatPoint absPos = obj->localToAbsolute();
Comment 6 Simon Fraser (smfr) 2015-06-26 11:59:36 PDT
HTMLAreaElement needs to not blithely return absolute paths; I think the caller should do the absolute conversion if they need to.
Comment 7 Radar WebKit Bug Importer 2015-07-20 15:23:14 PDT
<rdar://problem/21908735>
Comment 8 zalan 2016-04-07 20:13:19 PDT
Created attachment 275968 [details]
Patch
Comment 9 Build Bot 2016-04-07 21:00:05 PDT
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
Comment 10 Build Bot 2016-04-07 21:00:10 PDT
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
Comment 11 zalan 2016-04-07 21:29:48 PDT
Created attachment 275980 [details]
Patch
Comment 12 Darin Adler 2016-04-07 23:22:21 PDT
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?
Comment 13 zalan 2016-04-08 11:05:49 PDT
Created attachment 276017 [details]
Patch
Comment 14 Simon Fraser (smfr) 2016-04-08 11:18:53 PDT
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().
Comment 15 zalan 2016-04-08 11:31:15 PDT
Created attachment 276019 [details]
Patch
Comment 16 Build Bot 2016-04-08 12:26:08 PDT
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
Comment 17 Build Bot 2016-04-08 12:26:13 PDT
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
Comment 18 zalan 2016-04-08 12:59:38 PDT
Created attachment 276028 [details]
Patch
Comment 19 WebKit Commit Bot 2016-04-08 14:01:50 PDT
Comment on attachment 276028 [details]
Patch

Clearing flags on attachment: 276028

Committed r199247: <http://trac.webkit.org/changeset/199247>
Comment 20 WebKit Commit Bot 2016-04-08 14:01:57 PDT
All reviewed patches have been landed.  Closing bug.