Bug 143527

Summary:

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

Product:

WebKit

Reporter:

Chris Rebert <webkit>

Component:

CSS

Assignee:

zalan <zalan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, cdumez, commit-queue, dbates, eoconnor, esprehn+autocc, glenn, gyuyoung.kim, kondapallykalyan, mmaxfield, rniwa, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

HasReduction, InRadar

Version:

WebKit Nightly Build

Hardware:

Mac (Intel)

OS:

OS X 10.11

URL:

http://jsbin.com/tufoye/2/edit

Bug Depends on:

Bug Blocks:

159753

Attachments:

Description	Flags
Top focus ring location is correct, Bottom focus ring within modal is wrong	none
Copy of JS Bin example	none
Example	none
Example	none
Patch	buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-yosemite	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews124 for ios-simulator-wk2	none
Patch	none

Chris Rebert

Reported 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

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
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Chris Rebert

Comment 1 2015-04-08 11:36:00 PDT

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

Chris Rebert

Comment 2 2015-05-06 11:47:11 PDT

Created attachment 252501 [details] Copy of JS Bin example

Daniel Bates

Comment 3 2015-06-26 11:16:28 PDT

Created attachment 255651 [details] Example An example that demonstrates the issue.

Daniel Bates

Comment 4 2015-06-26 11:17:57 PDT

Created attachment 255652 [details] Example

Simon Fraser (smfr)

Comment 5 2015-06-26 11:52:44 PDT

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

Simon Fraser (smfr)

Comment 6 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.

Radar WebKit Bug Importer

Comment 7 2015-07-20 15:23:14 PDT

<rdar://problem/21908735>

zalan

Comment 8 2016-04-07 20:13:19 PDT

Created attachment 275968 [details] Patch

Build Bot

Comment 9 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

Build Bot

Comment 10 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

zalan

Comment 11 2016-04-07 21:29:48 PDT

Created attachment 275980 [details] Patch

Darin Adler

Comment 12 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?

zalan

Comment 13 2016-04-08 11:05:49 PDT

Created attachment 276017 [details] Patch

Simon Fraser (smfr)

Comment 14 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().

zalan

Comment 15 2016-04-08 11:31:15 PDT

Created attachment 276019 [details] Patch

Build Bot

Comment 16 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

Build Bot

Comment 17 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

zalan

Comment 18 2016-04-08 12:59:38 PDT

Created attachment 276028 [details] Patch

WebKit Commit Bot

Comment 19 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>

WebKit Commit Bot

Comment 20 2016-04-08 14:01:57 PDT

All reviewed patches have been landed. Closing bug.

Chris Rebert

Comment 21 2016-04-08 17:45:51 PDT

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

Note You need to log in before you can comment on or make changes to this bug.