Bug 113042 – Web Inspector: expose nodes with pointer-events: none to node search tool.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED WONTFIX

[Dmitry Gozman]

Currently, hovering over element with pointer-events:none does not highlight it while in node search inspector mode.

[Dmitry Gozman]

Created attachment 194521 [details]
Patch

[Pavel Feldman]

Comment on attachment 194521 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194521&action=review

Before it gets reviewed, does it work in accelerated compositing mode? That mode might do its own hit testing.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1050
> +    return m_searchingForNode;

What does it mean?

> Source/WebCore/page/EventHandler.cpp:1662
> +    if (m_frame->document()->renderer() && InspectorInstrumentation::willHandleMouseMove(m_frame->page())) {

Can you move mouseDidMoveOverElement call here and do the rest in it? Pass whatever you need into it.

> Source/WebCore/rendering/RenderObject.h:977
> +    bool visibleToRegularHitTesting() const { return style()->visibility() == VISIBLE && style()->pointerEvents() != PE_NONE; }

I would leave this name and introduce visibleToHitTestingIgnoringPointerEvents

[PhistucK]

I would highlight pointer-events: none elements with a slightly different color (maybe just a lighter/less opaque version of the current color), because up until now, the current behavior was equally annoying (I could not select these elements) and useful (I immediately knew they have pointer-events: none).
What do you think?

[Pavel Feldman]

That's a very good point. But I'd rather paint a border around the node that captures pointer events in case it is different.

[Dmitry Gozman]

Comment on attachment 194521 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194521&action=review

>> Before it gets reviewed, does it work in accelerated compositing mode? That mode might do its own hit testing.
Compositing mode does its own hit testing to choose the layer which receives the event. Then WebCore hit testing kicks in. No need to change anything in compositing.

>> Source/WebCore/inspector/InspectorDOMAgent.cpp:1050
>> +    return m_searchingForNode;
> 
> What does it mean?

Removed.

>> Source/WebCore/page/EventHandler.cpp:1662
>> +    if (m_frame->document()->renderer() && InspectorInstrumentation::willHandleMouseMove(m_frame->page())) {
> 
> Can you move mouseDidMoveOverElement call here and do the rest in it? Pass whatever you need into it.

Done.

>> Source/WebCore/rendering/RenderObject.h:977
>> +    bool visibleToRegularHitTesting() const { return style()->visibility() == VISIBLE && style()->pointerEvents() != PE_NONE; }
> 
> I would leave this name and introduce visibleToHitTestingIgnoringPointerEvents

I left this name as is, and added visibleToHitTestRequest instead.

[Dmitry Gozman]

Created attachment 195092 [details]
Patch

[Pavel Feldman]

Comment on attachment 195092 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195092&action=review

Overall looks good, couple of nits inside. Please add someone familiar with hit testing code in render object to review for blessing.

> Source/WebCore/page/EventHandler.cpp:1662
> +    if (m_frame->contentRenderer()) {

These two lines should be a part of inspector instrumentation.

> Source/WebCore/rendering/HitTestRequest.h:42
> +        IgnorePointerEvents = 1 << 12

IgnorePointerEventsNone

[Dmitry Gozman]

Created attachment 195246 [details]
Patch

[Dmitry Gozman]

Comment on attachment 195092 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=195092&action=review

>> Source/WebCore/page/EventHandler.cpp:1662
>> +    if (m_frame->contentRenderer()) {
> 
> These two lines should be a part of inspector instrumentation.

Moved.

>> Source/WebCore/rendering/HitTestRequest.h:42
>> +        IgnorePointerEvents = 1 << 12
> 
> IgnorePointerEventsNone

Done.

[Dmitry Gozman]

Allan, could you please take a look at hit testing change?

[Dmitry Gozman]

Created attachment 196118 [details]
Patch

[Pavel Feldman]

Comment on attachment 196118 [details]
Patch

This change looks good to me, but we still need to show the element that gets mouse events. Otherwise, it will regress the "searching for the glass pane" scenario.