Fix image map focus ring painting
Use correct paint offset and clip, and apply zoom factor.
Some changes are based on https://bugs.webkit.org/show_bug.cgi?id=143527.
BUG=475128
TEST=fast/images/imagemap-focus-ring-in-positioned-container.html
TEST=fast/images/imagemap-focus-ring-with-paint-root-offset.html
TEST=fast/images/imagemap-focus-ring-with-scale-transform.html
TBR=dmazzoni@chromium.org (passive change under Source/modules/accessibility due to function renaming)
Committed: https://crrev.com/3086ff0e791a9ed5d08f8a1c817eb3da36d19992
Cr-Commit-Position: refs/heads/master@{#389883}
4 years, 8 months ago
(2016-04-21 23:02:57 UTC)
#2
pdr.
I like this approach overall as it's a nice cleanup. Just some small questions. https://codereview.chromium.org/1912863002/diff/20001/third_party/WebKit/LayoutTests/fast/images/imagemap-focus-ring-in-positioned-container.html ...
4 years, 8 months ago
(2016-04-23 03:32:23 UTC)
#3
+chrishtr in case pdr is on trip. https://codereview.chromium.org/1912863002/diff/20001/third_party/WebKit/LayoutTests/fast/images/imagemap-focus-ring-in-positioned-container.html File third_party/WebKit/LayoutTests/fast/images/imagemap-focus-ring-in-positioned-container.html (right): https://codereview.chromium.org/1912863002/diff/20001/third_party/WebKit/LayoutTests/fast/images/imagemap-focus-ring-in-positioned-container.html#newcode1 third_party/WebKit/LayoutTests/fast/images/imagemap-focus-ring-in-positioned-container.html:1: <!DOCTYPE html> ...
4 years, 8 months ago
(2016-04-25 23:18:16 UTC)
#6
+chrishtr in case pdr is on trip.
https://codereview.chromium.org/1912863002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/images/imagemap-focus-ring-in-positioned-container.html
(right):
https://codereview.chromium.org/1912863002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/images/imagemap-focus-ring-in-positioned-container.html:1:
<!DOCTYPE html>
On 2016/04/23 03:32:22, pdr wrote:
> Is it possible to test these with reftests instead of pixel tests?
> imagemap-focus-ring-zoom-style-expected.html looks like one example that does
> this.
Converted 2 of the 3 tests into reftests. Not using reftest for the remaining
one (with-transform) because I can't avoid small pixel differences except I also
use transform in the reference which will reduce the test coverage (because the
test and the ref use too much the same code path).
https://codereview.chromium.org/1912863002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/images/imagemap-focus-ring-in-positioned-container.html:15:
window.onload = function() {
On 2016/04/23 03:40:54, pdr wrote:
> Nit: No need for onload here, you can just put the following at the bottom of
> this file:
> <script>
> area.focus();
> </script>
Done but still use document.getElementById('area').
Just curious about the usage of "area" instead of
"document.getElementById('area')". Is this a part of open web API of chrome?
https://codereview.chromium.org/1912863002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXImageMapLink.cpp (left):
https://codereview.chromium.org/1912863002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXImageMapLink.cpp:108:
HTMLMapElement* map = mapElement();
I gave up changing this in this CL. Filed bug 606506 instead.
https://codereview.chromium.org/1912863002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLAreaElement.h (right):
https://codereview.chromium.org/1912863002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLAreaElement.h:45: LayoutRect
computeAbsoluteRect(const LayoutObject*) const;
I added back the parameter because the parameter is needed to identify the
container object in case that a map is referenced by multiple images.
pdr.
On 2016/04/25 at 23:18:16, wangxianzhu wrote: > https://codereview.chromium.org/1912863002/diff/20001/third_party/WebKit/LayoutTests/fast/images/imagemap-focus-ring-in-positioned-container.html#newcode15 > third_party/WebKit/LayoutTests/fast/images/imagemap-focus-ring-in-positioned-container.html:15: window.onload = function() { > ...
4 years, 8 months ago
(2016-04-25 23:44:15 UTC)
#7
On 2016/04/25 at 23:18:16, wangxianzhu wrote:
>
https://codereview.chromium.org/1912863002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/fast/images/imagemap-focus-ring-in-positioned-container.html:15:
window.onload = function() {
> On 2016/04/23 03:40:54, pdr wrote:
> > Nit: No need for onload here, you can just put the following at the bottom
of
> > this file:
> > <script>
> > area.focus();
> > </script>
>
> Done but still use document.getElementById('area').
>
> Just curious about the usage of "area" instead of
"document.getElementById('area')". Is this a part of open web API of chrome?
It's an old quirk that ended up getting spec'd:
https://html.spec.whatwg.org/#dom-window-nameditem
Absolutely OK to leave this as document.getElementById() though. You can just
save typing in the future if you want.
pdr.
https://codereview.chromium.org/1912863002/diff/60001/third_party/WebKit/Source/core/html/HTMLAreaElement.cpp File third_party/WebKit/Source/core/html/HTMLAreaElement.cpp (right): https://codereview.chromium.org/1912863002/diff/60001/third_party/WebKit/Source/core/html/HTMLAreaElement.cpp#newcode109 third_party/WebKit/Source/core/html/HTMLAreaElement.cpp:109: Path p = getPath(containerObject); Nit: p -> path https://codereview.chromium.org/1912863002/diff/60001/third_party/WebKit/Source/core/html/HTMLAreaElement.cpp#newcode111 ...
4 years, 8 months ago
(2016-04-26 01:29:35 UTC)
#8
No Ptal yet. I need to fix the zoom issue before uploading the new patch. ...
4 years, 8 months ago
(2016-04-26 17:42:27 UTC)
#9
No Ptal yet. I need to fix the zoom issue before uploading the new patch.
https://codereview.chromium.org/1912863002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLAreaElement.cpp (right):
https://codereview.chromium.org/1912863002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLAreaElement.cpp:126: Path path;
On 2016/04/26 01:29:35, pdr wrote:
> Invalidate m_path for this codepath?
Done.
https://codereview.chromium.org/1912863002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLAreaElement.cpp:137: if
(m_coords.isEmpty() && m_shape != Default)
On 2016/04/26 01:29:35, pdr wrote:
> m_shape can't be Default here so this can just be:
> if (m_coords.isEmpty())
> return *m_path;
Done.
https://codereview.chromium.org/1912863002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/ImagePainter.cpp (right):
https://codereview.chromium.org/1912863002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ImagePainter.cpp:57: Path path =
areaElement.getPath(m_layoutImage.size());
On 2016/04/26 01:29:35, pdr wrote:
> I think this needs to be areaElement.getPath(&m_layoutImage);
Done.
I remember I had changed this before uploading. Don't know how I made this wrong
here :(
https://codereview.chromium.org/1912863002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/ImagePainter.cpp:61: AffineTransform
zoomTransform;
On 2016/04/26 01:29:35, pdr wrote:
> Why is it correct to zoom here? There are almost no calls to effectiveZoom in
> core/paint/ so it feels like we may have this in the wrong place.
>
> Does contentBoxRect (used below for focusRect) have zoom applied? If so, maybe
> the whole path should always have zoom applied like it was before, and then
you
> could remove the inverse zoom in HitTestResult::imageAreaForImage.
Thanks for the finding. I didn't carefully think about zoom when I applied the
webkit patch. Now I found the problem of the webkit patch: it applies zoom
inconsistently for default shape and non-default shape, and didn't test default
shape with zoom (which is problematic). A non-default shape doesn't have zoom
applied in HTMLAreaElement::getPath(), so need to apply zoom here, but a default
shape will have zoom applied twice.
Xianzhu
Done fixing zoom issue. Ptal.
4 years, 8 months ago
(2016-04-26 18:27:27 UTC)
#10
Done fixing zoom issue. Ptal.
pdr.
This is getting really close. Just two small comments. https://codereview.chromium.org/1912863002/diff/100001/third_party/WebKit/Source/core/html/HTMLAreaElement.cpp File third_party/WebKit/Source/core/html/HTMLAreaElement.cpp (right): https://codereview.chromium.org/1912863002/diff/100001/third_party/WebKit/Source/core/html/HTMLAreaElement.cpp#newcode124 third_party/WebKit/Source/core/html/HTMLAreaElement.cpp:124: ...
4 years, 8 months ago
(2016-04-26 18:47:29 UTC)
#11
Have to use pixel test for all of the new tests. On Mac normal focus ...
4 years, 8 months ago
(2016-04-26 19:25:03 UTC)
#12
Have to use pixel test for all of the new tests. On Mac normal focus rings has
outset, but we can't apply outset for imagemap focus rings because we can't
paint outside of the overflow rect of the image otherwise there would be under
invalidation. It would bring much complexity if we include the area focus ring
into image's visual overflow.
https://codereview.chromium.org/1912863002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLAreaElement.cpp (right):
https://codereview.chromium.org/1912863002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLAreaElement.cpp:124:
path.addRect(FloatRect(toLayoutBox(containerObject)->borderBoxRect()));
On 2016/04/26 18:47:29, pdr wrote:
> This doesn't need the zoom below because it already includes zoom?
>
> If so, could you add a short comment?
Done.
https://codereview.chromium.org/1912863002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLAreaElement.h (right):
https://codereview.chromium.org/1912863002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLAreaElement.h:48: // and effective zoom
factor of the container object will be applied on non-default shape.
On 2016/04/26 18:47:29, pdr wrote:
> Excellent comment, thank you for writing it. Can you remove the space between
> the comment and the function though?
Done.
pdr.
On 2016/04/26 at 19:25:03, wangxianzhu wrote: > Have to use pixel test for all of ...
4 years, 8 months ago
(2016-04-26 19:59:23 UTC)
#13
On 2016/04/26 at 19:25:03, wangxianzhu wrote:
> Have to use pixel test for all of the new tests. On Mac normal focus rings has
outset, but we can't apply outset for imagemap focus rings because we can't
paint outside of the overflow rect of the image otherwise there would be under
invalidation. It would bring much complexity if we include the area focus ring
into image's visual overflow.
>
>
https://codereview.chromium.org/1912863002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/HTMLAreaElement.cpp (right):
>
>
https://codereview.chromium.org/1912863002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/HTMLAreaElement.cpp:124:
path.addRect(FloatRect(toLayoutBox(containerObject)->borderBoxRect()));
> On 2016/04/26 18:47:29, pdr wrote:
> > This doesn't need the zoom below because it already includes zoom?
> >
> > If so, could you add a short comment?
>
> Done.
>
>
https://codereview.chromium.org/1912863002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/HTMLAreaElement.h (right):
>
>
https://codereview.chromium.org/1912863002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/HTMLAreaElement.h:48: // and effective
zoom factor of the container object will be applied on non-default shape.
> On 2016/04/26 18:47:29, pdr wrote:
> > Excellent comment, thank you for writing it. Can you remove the space
between
> > the comment and the function though?
>
> Done.
LGTM!
"This'll be a simple merge" haha :)
Xianzhu
The CQ bit was checked by wangxianzhu@chromium.org
4 years, 8 months ago
(2016-04-26 20:34:43 UTC)
#14
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1912863002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912863002/120001
4 years, 8 months ago
(2016-04-26 20:35:04 UTC)
#15
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/173463)
4 years, 8 months ago
(2016-04-26 20:42:50 UTC)
#17
Description was changed from ========== Fix image map focus ring painting Use correct paint offset ...
4 years, 8 months ago
(2016-04-26 21:04:13 UTC)
#18
Description was changed from
==========
Fix image map focus ring painting
Use correct paint offset and clip, and apply zoom factor.
Some changes are based on https://bugs.webkit.org/show_bug.cgi?id=143527.
BUG=475128
TEST=fast/images/imagemap-focus-ring-in-positioned-container.html
TEST=fast/images/imagemap-focus-ring-with-paint-root-offset.html
TEST=fast/images/imagemap-focus-ring-with-scale-transform.html
==========
to
==========
Fix image map focus ring painting
Use correct paint offset and clip, and apply zoom factor.
Some changes are based on https://bugs.webkit.org/show_bug.cgi?id=143527.
BUG=475128
TEST=fast/images/imagemap-focus-ring-in-positioned-container.html
TEST=fast/images/imagemap-focus-ring-with-paint-root-offset.html
TEST=fast/images/imagemap-focus-ring-with-scale-transform.html
TBR=dmazzoni@chromium.org (passive change under Source/modules/accessibility due
to function renaming)
==========
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1912863002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912863002/120001
4 years, 8 months ago
(2016-04-26 21:04:57 UTC)
#21
Description was changed from ========== Fix image map focus ring painting Use correct paint offset ...
4 years, 8 months ago
(2016-04-26 21:11:02 UTC)
#22
Message was sent while issue was closed.
Description was changed from
==========
Fix image map focus ring painting
Use correct paint offset and clip, and apply zoom factor.
Some changes are based on https://bugs.webkit.org/show_bug.cgi?id=143527.
BUG=475128
TEST=fast/images/imagemap-focus-ring-in-positioned-container.html
TEST=fast/images/imagemap-focus-ring-with-paint-root-offset.html
TEST=fast/images/imagemap-focus-ring-with-scale-transform.html
TBR=dmazzoni@chromium.org (passive change under Source/modules/accessibility due
to function renaming)
==========
to
==========
Fix image map focus ring painting
Use correct paint offset and clip, and apply zoom factor.
Some changes are based on https://bugs.webkit.org/show_bug.cgi?id=143527.
BUG=475128
TEST=fast/images/imagemap-focus-ring-in-positioned-container.html
TEST=fast/images/imagemap-focus-ring-with-paint-root-offset.html
TEST=fast/images/imagemap-focus-ring-with-scale-transform.html
TBR=dmazzoni@chromium.org (passive change under Source/modules/accessibility due
to function renaming)
==========
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 8 months ago
(2016-04-26 21:11:03 UTC)
#23
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
commit-bot: I haz the power
Description was changed from ========== Fix image map focus ring painting Use correct paint offset ...
4 years, 8 months ago
(2016-04-26 21:12:27 UTC)
#24
Message was sent while issue was closed.
Description was changed from
==========
Fix image map focus ring painting
Use correct paint offset and clip, and apply zoom factor.
Some changes are based on https://bugs.webkit.org/show_bug.cgi?id=143527.
BUG=475128
TEST=fast/images/imagemap-focus-ring-in-positioned-container.html
TEST=fast/images/imagemap-focus-ring-with-paint-root-offset.html
TEST=fast/images/imagemap-focus-ring-with-scale-transform.html
TBR=dmazzoni@chromium.org (passive change under Source/modules/accessibility due
to function renaming)
==========
to
==========
Fix image map focus ring painting
Use correct paint offset and clip, and apply zoom factor.
Some changes are based on https://bugs.webkit.org/show_bug.cgi?id=143527.
BUG=475128
TEST=fast/images/imagemap-focus-ring-in-positioned-container.html
TEST=fast/images/imagemap-focus-ring-with-paint-root-offset.html
TEST=fast/images/imagemap-focus-ring-with-scale-transform.html
TBR=dmazzoni@chromium.org (passive change under Source/modules/accessibility due
to function renaming)
Committed: https://crrev.com/3086ff0e791a9ed5d08f8a1c817eb3da36d19992
Cr-Commit-Position: refs/heads/master@{#389883}
==========
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/3086ff0e791a9ed5d08f8a1c817eb3da36d19992 Cr-Commit-Position: refs/heads/master@{#389883}
4 years, 8 months ago
(2016-04-26 21:12:29 UTC)
#25
Issue 1912863002: Fix image map focus ring painting
(Closed)
Created 4 years, 8 months ago by Xianzhu
Modified 4 years, 8 months ago
Reviewers: pdr., chrishtr, dmazzoni
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Comments: 22
Chromium Code Reviews
Issue
1912863002:
Fix image map focus ring painting (Closed)
