Bug 21095 – Web Inspector: Search should jump in all matches in Elements panel

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

[Anthony Ricaud]

Steps to reproduce
1: Go to http://webkit.org
2: Open the Elements panel
3: Search "we" and hit Return
4: Hit Return again

Actual behaviour:
Search jumps to "WebKit is an open source web browser engine"

Expected behaviour:
Search should stay on the same line and highlight the "we" part of "website"

[Sammy]

The problem here seems to be in 2 parts
1). Individual matches in a node are not calculated hence giving wrong count.
    - I have done the changes for it and will upload the patch soon.
2). Jumping of matched element is not exclusive.
    - This seems to be an issue in javascript which is handling highlighting search results. The js is actually wrapping full element node as well as text nodes in a single outer-span-element. Reg-ex search is then performed for the text value (which is "element-node" + "text-node")of this outer-span-element (which eventually will always be same for matches in element node or text node).
For ex: <div id="website"> we all love web </div>
In inpector, this gets wrapped in outer span as 
  <span class="highlight">
     <span class="webkit-html-tag"> "<" </span>
     <span class="webkit-html-tag-name"> div </span>
     <span class="webkit-html-tag-attribute"> 
        <span class="webkit-html-attribute-name">id</span>
        <span class="webkit-html-attribute-value"> website </span>
     </span> 
     <span class="webkit-html-text-node"> we all love website </span>
     <span class="webkit-html-tag"> "<" <span>/div</span> </span>
  </span> 

Actual text which gets search, for highlighting,  is the text value of outer span - "<div id="website"> we all love web </div>". So eventhough DOM Agent's perform search returns back actual node (text, comment, element) ; regex for highlighting happens for all instances of match in this text. Hence not jumping to individual matches.

I am looking into the js changes needed. Will upload the patch for it soon!!

[Sammy]

Created attachment 135272 [details]
Patch

[WebKit Review Bot]

Attachment 135272 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 4 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 135272 [details]
Patch

Attachment 135272 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12307980

[Build Bot]

Comment on attachment 135272 [details]
Patch

Attachment 135272 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12309957

[Sammy]

Created attachment 135274 [details]
Patch

[WebKit Review Bot]

Comment on attachment 135274 [details]
Patch

Attachment 135274 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12310894

New failing tests:
inspector/elements/elements-panel-search.html

[WebKit Review Bot]

Created attachment 135277 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[sam]

(In reply to comment #7)
> (From update of attachment 135274 [details])
> Attachment 135274 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12310894
> 
> New failing tests:
> inspector/elements/elements-panel-search.html

I guess the failure of test case for partial tag is expected here as in there are 2 matches found in <input value="InputVal"> for "nput" and it should be printed twice.
Please correct me if I am getting it wrong!

[Pavel Feldman]

Comment on attachment 135274 [details]
Patch

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

My general concern is Gmail-size apps. Gmail ends up having 5Mb of script tags in its DOM. Searching for "for" or "a" will be creating millions of node ids as results. We are currently bound with the number of nodes as search results, but after your change we become bound to the number of characters in DOM. Please keep it in mind when applying the front-end changes.

> Source/WebCore/ChangeLog:3
> +        Search should jump in all matches in Elements panel

Please prefix title with "Web Inspector:"

> Source/WebCore/ChangeLog:6
> +        This patch is to get the correct count of matched values

You could be more specific here.

> Source/WebCore/ChangeLog:11
> +        No new tests required as this is the backend part of the fix.

You should be able to test this: see LayoutTests/inspector/elements/elements-panel-search.html

> Source/WebCore/inspector/InspectorDOMAgent.cpp:830
> +                while ((queryIndex = text.findIgnoringCase(whitespaceTrimmedQuery, queryIndex + 1)) != static_cast<int>(notFound))

I would introduce a static function above this method to re-use the code more aggressively, something like

populateNodeResults(Vector<Node*>& resultCollector, const String& str)

> Source/WebCore/inspector/InspectorDOMAgent.cpp:890
> +      resultsIt->second.append(*it);

So you need to have a front-end code that would iterate through individual matches too. You should make it a part of the same change.

[Sammy]

Created attachment 136221 [details]
Patch

[Sammy]

(In reply to comment #10)
Thank you Pavel for review and inputs.
I have incorporated the review comments and have added js changes along with this patch.

> (From update of attachment 135274 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135274&action=review
> 
> My general concern is Gmail-size apps. Gmail ends up having 5Mb of script tags in its DOM. Searching for "for" or "a" will be creating millions of node ids as results. We are currently bound with the number of nodes as search results, but after your change we become bound to the number of characters in DOM. Please keep it in mind when applying the front-end changes.
> 
Thanks for the inputs. I have tried to preserve backing up of highlightResults of for all matches of one element node. Highlighting individual matches, the next time, then happens from this. 

> > Source/WebCore/ChangeLog:3
> > +        Search should jump in all matches in Elements panel
> 
> Please prefix title with "Web Inspector:"
> 
Have added the prefix, thanks.
> > Source/WebCore/ChangeLog:6
> > +        This patch is to get the correct count of matched values
> 
> You could be more specific here.
> 
Have elaborated on change log comments. Plz suggest if its ok.
> > Source/WebCore/ChangeLog:11
> > +        No new tests required as this is the backend part of the fix.
> 
> You should be able to test this: see LayoutTests/inspector/elements/elements-panel-search.html
> 
Layout test expected result has been updated with desired value. For testPartialTag, there are 2 matches for "nput" in <input>input value</input> hence domAgent.searchResult returns with 2 as result count.
> > Source/WebCore/inspector/InspectorDOMAgent.cpp:830
> > +                while ((queryIndex = text.findIgnoringCase(whitespaceTrimmedQuery, queryIndex + 1)) != static_cast<int>(notFound))
> 
> I would introduce a static function above this method to re-use the code more aggressively, something like
> 
> populateNodeResults(Vector<Node*>& resultCollector, const String& str)
> 
Thanks for the input. I have introduced the function for code re-use.

> > Source/WebCore/inspector/InspectorDOMAgent.cpp:890
> > +      resultsIt->second.append(*it);
> 
> So you need to have a front-end code that would iterate through individual matches too. You should make it a part of the same change.

I have added js changes with this patch. Have tried to preserve the backing up of highlightResults for all matches in one element node. Iteration through individual matches, next time , happens over this. 

I found a issue with string corruption while applying the updateHighlights(). Kindly suggest, if I should file a new bug for this.

[Sammy]

Created attachment 136456 [details]
Patch

[WebKit Review Bot]

Attachment 136456 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1
Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sammy]

Created attachment 136459 [details]
Patch

[Pavel Feldman]

Comment on attachment 136459 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        The fix calculates number of matches within the node as well while

Make sure you address the style bot comments on the tab characters here.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:801
> +void InspectorDOMAgent::populateNodeResults(Vector<Node*>& resultCollector, Node* node, const String& searchString, const String& queryString)

I don't think it should be public, I would not even mention it in the header. What I was suggesting was a static function within cpp:

static void populateNodeResults(...)


I don't really like the changes to the semantics of the protocol that happen here: performSearch returns the number of matches, getSearchResults returns ranges of matches. But for the node that contains multiple instances of "foo", we will get a number of identical results in the range. Given that getSearchResults is hidden in the protocol, we could tweak it to optionally return ordinal of the text occurrence within the node. I.e. each search match is not a nodeId, but the {nodeId: NodeId, ordinal: number} pair. Then traversing the results in the front-end would be much more straightforward and other clients would be able to leverage the strictness.

> Source/WebCore/inspector/front-end/ElementsPanel.js:475
> +        this.searchResultJumpDirection = "next";    

We prefix private fields with "_".

[Brian Burg]

There is a different search implementation now. Find in resources implements this.