Image maps should work for images with alt content

Image maps should work for images with alt content

If the image associated with an image-map does not load successfully the click
area defined by the map should still work. This requires us to watch for hits
on the image map from within the inline block used to display the alt content.

BUG=466116
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194542

[rhogan, 2015-04-24 15:54:00]

robhogan@gmail.com changed reviewers:
+ pdr@chromium.org

[pdr., 2015-04-24 21:48:23]

https://codereview.chromium.org/1094133004/diff/1/Source/core/layout/HitTestR...
File Source/core/layout/HitTestResult.cpp (right):

https://codereview.chromium.org/1094133004/diff/1/Source/core/layout/HitTestR...
Source/core/layout/HitTestResult.cpp:162: 
Instead of duplicating this logic here, what if we combined this with the
similar code in LayoutImage::nodeAtPoint into
HitTestResult::setToImageMapForImage (or similar). It looks like
setToShadowHostIfInUserAgentShadowRoot is already being called for this codepath
so you could either add a call to setToImageMapForImage after
setToShadowHostIfInUserAgentShadowRoot or create a combined function called
setToUserAgentShadowRootAndUseImageMapIfNeeded (or similar).

https://codereview.chromium.org/1094133004/diff/1/Source/core/layout/HitTestR...
File Source/core/layout/HitTestResult.h (right):

https://codereview.chromium.org/1094133004/diff/1/Source/core/layout/HitTestR...
Source/core/layout/HitTestResult.h:94: void setNodeAndPosition(Node* node, const
LayoutPoint& p) { m_localPoint = p; setInnerNode(node); }
I like this cleanup.

[rhogan, 2015-04-25 17:28:40]

On 2015/04/24 at 21:48:23, pdr wrote:
>
https://codereview.chromium.org/1094133004/diff/1/Source/core/layout/HitTestR...
> File Source/core/layout/HitTestResult.cpp (right):
> 
>
https://codereview.chromium.org/1094133004/diff/1/Source/core/layout/HitTestR...
> Source/core/layout/HitTestResult.cpp:162: 
> Instead of duplicating this logic here, what if we combined this with the
similar code in LayoutImage::nodeAtPoint into
HitTestResult::setToImageMapForImage (or similar). It looks like
setToShadowHostIfInUserAgentShadowRoot is already being called for this codepath
so you could either add a call to setToImageMapForImage after
setToShadowHostIfInUserAgentShadowRoot or create a combined function called
setToUserAgentShadowRootAndUseImageMapIfNeeded (or similar).

setToShadowHostIfInUserAgentShadowRoot is used only during mouse movements, so
the hit test result gets overwritten by subsequent hit tests initated by other
events. That has led me to keep my change in setInnerNode. Surprising
setToShadowHostIfInUserAgentShadowRoot goes back to 2007,
https://src.chromium.org/viewvc/blink/trunk/Source/core/page/EventHandler.cpp...
. It's not totally clear to me why it's necessary there but not for other events
that do hit testing. Any ideas?

My updated CL removes even more code but removal of imageMap() will have to be
done separately: it's used in the accsessibility code. I think I need to fix
that for alt content too so may find a way to refactor it then.

[pdr., 2015-04-26 02:59:15]

pdr@chromium.org changed reviewers:
+ rbyers@chromium.org

[pdr., 2015-04-26 02:59:16]

I see what you mean about setToShadowHostIfInUserAgentShadowRoot not getting
called. I'm pretty sure it needs to be called for all of these events since it
is responsible for adjusting the hit node outside the user agent shadow tree
underneath the <input type="search"> box. Lets not address that in this change.

I agree that this change is good now, just minor nits below.

https://codereview.chromium.org/1094133004/diff/20001/Source/core/layout/HitT...
File Source/core/layout/HitTestResult.cpp (right):

https://codereview.chromium.org/1094133004/diff/20001/Source/core/layout/HitT...
Source/core/layout/HitTestResult.cpp:156: HTMLAreaElement*
HitTestResult::imageAreaForImage()
Lets make this function const since it doesn't modify m_innerNode itself.
HTMLAreaElement* HitTestResult::imageAreaForImage() const

https://codereview.chromium.org/1094133004/diff/20001/Source/core/layout/HitT...
Source/core/layout/HitTestResult.cpp:158: HTMLImageElement* imageElement =
nullptr;
Even though we check it all over, I don't think m_innerNode can be null here.
Can you assert that?
ASSERT(m_innerNode);

https://codereview.chromium.org/1094133004/diff/20001/Source/core/layout/HitT...
Source/core/layout/HitTestResult.cpp:161: else if
(isHTMLImageElement(innerNode()->shadowHost()))
I think we need to limit this to user agent shadow hosts. For performance we may
want to check isInShadowTree too.

WDYT of the following?
if (isHTMLImageElement(m_innerNode)) {
    imageElement = toHTMLImageElement(innerNode());
} else if (m_innerNode->isInShadowTree()) {
    ShadowRoot* containingShadowRoot = m_innerNode->containingShadowRoot();
    if (containingShadowRoot->type() == ShadowRoot::UserAgentShadowRoot) {
        if (isHTMLImageElement(m_innerNode->shadowHost()))
            imageElement = toHTMLImageElement(m_innerNode->shadowHost());
    }
}

https://codereview.chromium.org/1094133004/diff/20001/Source/core/layout/HitT...
Source/core/layout/HitTestResult.cpp:167: LayoutBox* box =
toLayoutBox(layoutObject());
Nit: this can be moved inside the map conditional below.

[rhogan, 2015-04-27 16:56:26]

On 2015/04/26 at 02:59:16, pdr wrote:
> I see what you mean about setToShadowHostIfInUserAgentShadowRoot not getting
called. I'm pretty sure it needs to be called for all of these events since it
is responsible for adjusting the hit node outside the user agent shadow tree
underneath the <input type="search"> box. Lets not address that in this change.
> 
> I agree that this change is good now, just minor nits below.
> 

Nits addressed!

[pdr., 2015-04-27 17:05:03]

LGTM, thanks for sticking with this!

[pdr., 2015-04-27 17:05:05]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-04-27 17:05:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1094133004/40001

[commit-bot: I haz the power, 2015-04-27 19:39:24]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194542