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: | P2 | 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
Chris Rebert
2015-04-08 11:22:35 PDT
Corresponding Chrome/Blink bug: https://code.google.com/p/chromium/issues/detail?id=475128 Created attachment 252501 [details]
Copy of JS Bin example
Created attachment 255651 [details]
Example
An example that demonstrates the issue.
Created attachment 255652 [details]
Example
HTMLAreaElement::computePath() has: // FIXME: This doesn't work correctly with transforms. FloatPoint absPos = obj->localToAbsolute(); HTMLAreaElement needs to not blithely return absolute paths; I think the caller should do the absolute conversion if they need to. Created attachment 275968 [details]
Patch
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 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
Created attachment 275980 [details]
Patch
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? Created attachment 276017 [details]
Patch
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(). Created attachment 276019 [details]
Patch
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 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
Created attachment 276028 [details]
Patch
Comment on attachment 276028 [details] Patch Clearing flags on attachment: 276028 Committed r199247: <http://trac.webkit.org/changeset/199247> All reviewed patches have been landed. Closing bug. |