Bug 35125 – [Qt] Web Inspector does not highlight elements

• Component: WebKit Qt
• Status: RESOLVED FIXED

The Web Inspector in web browsers using QtWebKit, does not *highlight
elements on a page* when the mouse is hovered over the element's node in the
DOM inspector. Apperently the Web Inpector can do this, because it did in e.g. Safari
(http://www.codeofficer.com/gallery/image_full/75/). So this appears to be
specific to QtWebKit.

original bug report:
https://bugs.kde.org/show_bug.cgi?id=227547

[Benjamin Poulain]

This is not a bug per se, but that would be a nice feature to have.

I would not enable that be default but instead provide an API to know which QWebElement is hovered by the mouse (signal webElementHovered/webElementSelected(const QWebElement&)?). With this, any browser could implement fancy effects for the hovered element.

[Tor Arne Vestbø]

Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should only be used for bugs/features in the
    public QtWebKit API layer, not to signify that the bug is specific to
    the Qt port of WebKit

      http://trac.webkit.org/wiki/QtWebKitBugs#Component

  - Add the keyword 'Qt' to signal that it's a Qt-related bug

      http://trac.webkit.org/wiki/QtWebKitBugs#Keywords

[Ragner Magalhaes]

Created attachment 74311 [details]
Patch 01

Web inspector does highlight elements on the page when the mouse is hovered over the element's node in the DOM inspector.

FIXME: we should be able to only invalidate the rects of he new and old elements.

[Ragner Magalhaes]

Created attachment 74312 [details]
Patch result

[Benjamin Poulain]

I remember Jocelyn suggested having an API to control the highlight. Something like a signal inspectorHighlight(const QWebElement&). I can't remember the use case, you should ask his opinion on this.

[Jocelyn Turcotte]

(In reply to comment #5)

The goal was to allow some WYSIWYG editor component to highlight elements by itself, but this is a kind of narrow use case and highlighting from the inspector is far more useful.

[Ragner Magalhaes]

Does anyone could help me to fix this bug!?
I would like to get some review ... :P

[Antonio Gomes]

Comment on attachment 74311 [details]
Patch 01

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

> WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:214
> +        // FIXME: we should be able to only invalidate the rects of
> +        // the new and old nodes.

Why can not you just do it?

[Ragner Magalhaes]

(In reply to comment #8)
> (From update of attachment 74311 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74311&action=review
> 
> > WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:214
> > +        // FIXME: we should be able to only invalidate the rects of
> > +        // the new and old nodes.
> 
> Why can not you just do it?
Thank you Tonikito for your reply! :D

I just would like to know how could I do it?

How could I take that elements?

I just need some start point ;) 
thanks ..

[Antonio Gomes]

Comment on attachment 74311 [details]
Patch 01

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

> WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:217
> +        QRect rect = m_inspectedWebPage->mainFrame()->geometry();
> +        if (!rect.isEmpty())
> +            frame->view()->invalidateRect(rect);

what if you pass node in ::hidelight down to ::hideHighlight

and QRect rect = node->getRect()?

[Ragner Magalhaes]

(In reply to comment #10)

> 
> what if you pass node in ::hidelight down to ::hideHighlight
> 
> and QRect rect = node->getRect()?

I did this :D
But it is not working well :( because the inspector controller draws a label out of the rect on top or on bottom. how you can see on https://bug-35125-attachments.webkit.org/attachment.cgi?id=74312.

Sometimes this label can be greater than node's width and I did not find a simple way to get/calculate it!

To calculate the rect of the label I need to duplicate a lot of code from inspector controller. I don't think it is a good idea!

How this patch is just for inspector debug,
I don't think we should worry about performance.

We could just invalidate the visible area from main frame and we could have a inspector debug with highlight working :D how is implemented on chromium and gtk port!

[Antonio Gomes]

Ok, I understand.

two other questions:

1) how are other ports doing this?
2) what if you inflate the rect got from node::getRect a bit?

(In reply to comment #11)
> (In reply to comment #10)
> 
> > 
> > what if you pass node in ::hidelight down to ::hideHighlight
> > 
> > and QRect rect = node->getRect()?
> 
> I did this :D
> But it is not working well :( because the inspector controller draws a label out of the rect on top or on bottom. how you can see on https://bug-35125-attachments.webkit.org/attachment.cgi?id=74312.
> 
> Sometimes this label can be greater than node's width and I did not find a simple way to get/calculate it!
> 
> To calculate the rect of the label I need to duplicate a lot of code from inspector controller. I don't think it is a good idea!
> 
> How this patch is just for inspector debug,
> I don't think we should worry about performance.
> 
> We could just invalidate the visible area from main frame and we could have a inspector debug with highlight working :D how is implemented on chromium and gtk port!

[Ragner Magalhaes]

(In reply to comment #12)
> Ok, I understand.
> 
> two other questions:
> 
> 1) how are other ports doing this?

They are invalidating all the mainframe's view. 

So I think we could do like them and invalidate just the visible area
of the frame's view!

> 2) what if you inflate the rect got from node::getRect a bit?

I tried this,
but how I don't know the label's width I don't know how many pixels I need to expand the rect! 

Look for the FIXME on my code bellow:

void InspectorClientQt::hideHighlight()
{
    if (!m_highlightedNode)
        return;

    RenderObject* renderer = m_highlightedNode->renderer();
    Frame* frame = m_highlightedNode->document()->frame();
    if (!renderer || !frame || !renderer->isBox())
        return;

    RenderBox *renderBox = m_highlightedNode->renderBox();
    QRect contentBox = m_highlightedNode->getRect();

    // Adjust padding
    contentBox.adjust(-renderBox->paddingLeft(), -renderBox->paddingTop(),
            renderBox->paddingRight(), renderBox->paddingBottom());
    // Adjust border
    contentBox.adjust(-renderBox->borderLeft(), -renderBox->borderTop(),
            renderBox->borderRight(), renderBox->borderBottom());
    // Adjust margin
    contentBox.adjust(-renderBox->marginLeft(), -renderBox->marginTop(),
            renderBox->marginRight(), renderBox->marginBottom());

 
   // FIXME: It is not possible to get the size of the label yet!
   // Adjust for label and box lines
   // contentBox.adjust(-2, ?, ?, ?);

    if (!contentBox.isEmpty())
        frame->view()->invalidateRect(contentBox);

}

Thanks,
Tonikitoo

[Eugene Ostroukhov]

I would like to suggest make a public API so this highlight might be triggered not only by Inspector.

I.e. something like QElementHighlight* QWebElement::highlight(<color or some other way to customize highlighting>) and QWebElement::removeHighlight(QElementHighlight*)

In this case web inspector would be just another client of the API. Other users might be some IDEs that may want to highlight several elements (i.e. imagine an CSS editor that highlights elements that match the rule).

[Eugene Ostroukhov]

*** Bug 47224 has been marked as a duplicate of this bug. ***

[Antonio Gomes]

> In this case web inspector would be just another client of the API. Other users might be some IDEs that may want to highlight several elements (i.e. imagine an CSS editor that highlights elements that match the rule).

It is possible already with QWebElement. see http://doc.trolltech.com/4.5/webkit-fancybrowser.html

[Eugene Ostroukhov]

The usecases I see come mostly from the tools space (same goes for Web Inspector requirements) so changing the DOM state (including styles) does not seem to be the right approach.

All that I am asking for is not to keep the drawing code in the Web Inspector-specific areas but to make it more general.

[Simon Hausmann]

Comment on attachment 74311 [details]
Patch 01

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

r- because of the rect issue Antonio pointed out and because of the save() / restore() that should be optimized to not be executed in the common case when there is no highlighted node. Otherwise the patch is going in the right direction I'd say :)

> WebKit/qt/Api/qwebframe.cpp:415
> +#if ENABLE(INSPECTOR)
> +    context->save();
> +    frame->page()->inspectorController()->drawNodeHighlight(*context);
> +    context->restore();
> +#endif

It appears that this code is in QWebFramePrivate::renderRelativeCoords. In the _common_ case we would now end up calling save() and restore() on the graphics context each time we render content, where _most_ of the time drawNodeHighlight() returns immediately. save() and restore() are expensive operations we should avoid if necessary.

I suggest to either add a function tin InspectorController that allows us to perform a check here to see if we need to call drawNodeHighlight() and only then do save() / restore() or alternatively change the implementation of drawNodeHighligh() to do the save() / restore() pair.

[Yael]

(In reply to comment #18)
> (From update of attachment 74311 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74311&action=review
> 
> r- because of the rect issue Antonio pointed out and because of the save() / restore() that should be optimized to not be executed in the common case when there is no highlighted node. Otherwise the patch is going in the right direction I'd say :)
> 
I don't think there is a solution for the rect issue. Please don't let that block this patch.
A I mentioned on IRC, other ports invalidate everything as well.

[Ragner Magalhaes]

Hi Simon,
thank you for your review.

(In reply to comment #18)
> (From update of attachment 74311 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74311&action=review
> 
> r- because of the rect issue Antonio pointed out and because of the save() / restore() that should be optimized to not be executed in the common case when there is no highlighted node. Otherwise the patch is going in the right direction I'd say :)

The inspector controller draws a label out of the highlighted rect on top or on bottom(https://bug-35125-attachments.webkit.org/attachment.cgi?id=74312)
 I don't know how to fix the rect issue because is not possible
to get the label's size/position, the controller just build it in the draw method.

What could I do about the label issue?

> 
> > WebKit/qt/Api/qwebframe.cpp:415
> > +#if ENABLE(INSPECTOR)
> > +    context->save();
> > +    frame->page()->inspectorController()->drawNodeHighlight(*context);
> > +    context->restore();
> > +#endif
> 
> It appears that this code is in QWebFramePrivate::renderRelativeCoords. In the _common_ case we would now end up calling save() and restore() on the graphics context each time we render content, where _most_ of the time drawNodeHighlight() returns immediately. save() and restore() are expensive operations we should avoid if necessary.
> 
> I suggest to either add a function tin InspectorController that allows us to perform a check here to see if we need to call drawNodeHighlight() and only then do save() / restore() or alternatively change the implementation of drawNodeHighligh() to do the save() / restore() pair.

Ok,
I will think about and propose a new patch :D

Thank you

[Ragner Magalhaes]

Created attachment 80193 [details]
Patch 02

[Antonio Gomes]

Comment on attachment 80193 [details]
Patch 02

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

Looks good, just needs a better changelog.

> Source/WebCore/ChangeLog:8
> +        In the most of the time drawNodeHighlight() returns immediately because doesn't exist

1) "In the most" sounds bad.
2) "because no highlighted node exists" might read better?
3) "just when a highlighted node exists" ?

[Ragner Magalhaes]

Created attachment 80205 [details]
Patch 03

[Alexis Menard (darktears)]

Comment on attachment 80205 [details]
Patch 03

The patch looks ok to me and I think the feature is quite useful. I just found cumbersome that void InspectorClientQt::highlight(Node*) actually call hideHighlight() (I know they share the implementation :)).

[Antonio Gomes]

Comment on attachment 80205 [details]
Patch 03

r=me

[Yael]

(In reply to comment #25)
> (From update of attachment 80205 [details])
> r=me

yey! thanks:)

[Ragner Magalhaes]

Created attachment 83511 [details]
Patch 04

Sorry tonikitoo,
In the previous patch I forgot to add highlightedNode() method for InspectorController class
so it was not compiling ... :(

Now it is ok ...

Could you review it for me again?

Sorry my mistake guys.

[WebKit Commit Bot]

Comment on attachment 83511 [details]
Patch 04

Rejecting attachment 83511 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'ap..." exit_code: 2

Last 500 characters of output:

patching file Source/WebCore/inspector/InspectorController.cpp
Hunk #1 succeeded at 211 with fuzz 1 (offset 3 lines).
patching file Source/WebCore/inspector/InspectorController.h
patching file Source/WebKit/qt/Api/qwebframe.cpp
patching file Source/WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/qt/WebCoreSupport/InspectorClientQt.cpp

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--reviewer', u'Antonio Gomes', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7985563

[Eric Seidel (no email)]

Comment on attachment 80205 [details]
Patch 03

Cleared Antonio Gomes's review+ from obsolete attachment 80205 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Antonio Gomes]

Ragner's patch is good to go. Just needs to be rebased.

[Alexis Menard (darktears)]

Committed r83892: <http://trac.webkit.org/changeset/83892>