Bug 95252 – Web Inspector: node search does not work with elements on touch start listener

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Hanna]

the early return voids it

[Hanna]

Created attachment 161471 [details]
Patch

[Antonio Gomes]

Comment on attachment 161471 [details]
Patch

Lets add this method to WebPagePrivate instead, so it does not became a public API.

[Antonio Gomes]

Comment on attachment 161471 [details]
Patch

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

> Source/WebKit/blackberry/Api/WebPage.cpp:4100
> +    // FIXME this checks if node search on inspector is enabled, though it might not be optimized

nit1: FIXME:

nit2: lacks a dot at the end.

[Hanna]

Created attachment 161474 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161474 [details]
Patch

Clearing flags on attachment: 161474

Committed r127146: <http://trac.webkit.org/changeset/127146>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Konrad Piascik]

Reopening to attach new patch.

[Konrad Piascik]

Created attachment 162857 [details]
Patch

Re-opening since the old patch doesn't solve the issue

[Antonio Gomes]

Comment on attachment 162857 [details]
Patch

I would feel more comfortable if we get some Inspector guys' review here (and also unprefix the bug title).

[Konrad Piascik]

Remove BlackBerry from title since this fix requires cross-platform changes.

[Konrad Piascik]

update component

[Yury Semikhatsky]

Comment on attachment 162857 [details]
Patch

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

> Source/WebKit/blackberry/Api/WebPage.cpp:4111
> +    if (InspectorInstrumentation::searchingForNode(d->m_mainFrame->page()))

Could you provide more context on why it requires platform-specific handling for Blackberry? Also how is InspectorDOMAgent::handleMousePress called after this change?

[Pavel Feldman]

Can this be implemented entirely on the WebCore level (as in the mouse case) so that all ports could use it?

[Pavel Feldman]

Comment on attachment 162857 [details]
Patch

r- for the port-specific nature.

[Konrad Piascik]

The event handling model of the BlackBerry is different than other ports and that is why there is port specific behaviour here.

The issue in WebCore is that touch events fail to do 2 things when m_searchingForNode is true:
1) m_overlay->highlightedNode() is not set (in InspectorDOMAgent.cpp)
2) the touch event not prevented from firing

There would need to be instrumentation added at the WebCore level to detect when touch events are fired and see if the InspectorDOMAgent is searching for a node.

[Konrad Piascik]

Created attachment 166499 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 166499 [details]
Patch

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

The approach looks sane to me. Could you rebase the patch, since this one is not applicable to the tip of tree now?

> Source/WebKit/blackberry/ChangeLog:3
> +        node search does not work with elements on touch start listener

I'm not quite familiar with the issue. Does this mean it is impossible to inspect elements that have the ontouchstart listener attached?

[Konrad Piascik]

Created attachment 168063 [details]
Patch

[Pavel Feldman]

Comment on attachment 168063 [details]
Patch

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

> Source/WebCore/inspector/InspectorInstrumentation.h:-111
> -    static void mouseDidMoveOverElement(Page*, const HitTestResult&, unsigned modifierFlags);

New names are confusing. I would leave them as is and add new signal "handleTouchEvent" instead.

> Source/WebCore/page/EventHandler.cpp:3771
> +    if (InspectorInstrumentation::platformEventHandled(m_frame->page()))

There is no need to generate two signals, I'd rather call InspectorInstrumentation::handleTouchEvent and let inspector code handle it in case of inspect element mode.

[Konrad Piascik]

As per the discussion with Pavel Feldman on IRC here's the plan:
add InspectorInstrumentation::handleTouchEvent(Page*, Node*) and call this method only once in EventHandler::handleTouchEvent() avoiding 2 calls to InspectorInstruementation (which my current patch has)

[Konrad Piascik]

Created attachment 168250 [details]
Patch

[Pavel Feldman]

Comment on attachment 168250 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        node search does not work with elements on touch start listener

Web Inspector: prefix missing

[Konrad Piascik]

Update bug title.

[Konrad Piascik]

Created attachment 168252 [details]
Patch

[WebKit Review Bot]

Comment on attachment 168252 [details]
Patch

Clearing flags on attachment: 168252

Committed r131083: <http://trac.webkit.org/changeset/131083>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.