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
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.
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
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.
Created attachment 74312 [details] Patch result
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.
(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.
Does anyone could help me to fix this bug!? I would like to get some review ... :P
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?
(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 ..
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()?
(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!
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!
(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
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).
*** Bug 47224 has been marked as a duplicate of this bug. ***
> 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
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.
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.
(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.
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
Created attachment 80193 [details] Patch 02
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" ?
Created attachment 80205 [details] Patch 03
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 :)).
Comment on attachment 80205 [details] Patch 03 r=me
(In reply to comment #25) > (From update of attachment 80205 [details]) > r=me yey! thanks:)
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.
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
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.
Ragner's patch is good to go. Just needs to be rebased.
Committed r83892: <http://trac.webkit.org/changeset/83892>