Bug 83758

Summary: Web Inspector: Node Value should not get corrupt while jump over different matches in search.
Product: WebKit Reporter: sam <dsam2912>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
sample.html for reproducing the bug.
none
Patch
none
Patch none

Description sam 2012-04-12 01:20:04 PDT
Steps to reproduce :
1). Open html file, as an attachment.
2). Get to the inspect element view/
3). Try for searching "we".
4). Jump over different search matches.
5). Observe the text value getting misplaced for anchor node.

Expected Behavior: text value should not get misplacd
Comment 1 sam 2012-04-12 01:34:19 PDT
Constructing highlight node for a matched element requires 2 modes of operations.
"added" -> for adding new span element for matched value with highlight class.
"changed" -> for appending/manipulating  prefix/suffix node to highlight node.

There are cases were there will be multiple instances of matches in a node.
This will follow the same mode of operations for other matches in a node, but now on manipulated nodes.

While exec updateSearchHighlight, for hiding the highlights for cached results, the order in which updateEntryHide is called should then work in rev order to preserve the semantic of node.

I am creating a patch for it. I will upload it soon!!
Comment 2 sam 2012-04-12 01:36:18 PDT
Created attachment 136851 [details]
sample.html for reproducing the bug.
Comment 3 sam 2012-04-12 02:55:49 PDT
Created attachment 136861 [details]
Patch
Comment 4 Alexander Pavlov (apavlov) 2012-04-12 03:11:02 PDT
Comment on attachment 136861 [details]
Patch

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

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:640
> +        if (show) {

Since this now falls into two branches, it would be better to remove the |updater| var (which was introduced to avoid branching) altogether and call updateEntryShow and updateEntryHide directly in the branches instead.
Comment 5 sam 2012-04-12 03:45:39 PDT
Created attachment 136872 [details]
Patch
Comment 6 sam 2012-04-12 03:47:35 PDT
(In reply to comment #4)
> (From update of attachment 136861 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136861&action=review
> 
> > Source/WebCore/inspector/front-end/ElementsTreeOutline.js:640
> > +        if (show) {
> 
> Since this now falls into two branches, it would be better to remove the |updater| var (which was introduced to avoid branching) altogether and call updateEntryShow and updateEntryHide directly in the branches instead.

Thank you Alexander for review! I have uploaded the patch.
Comment 7 Alexander Pavlov (apavlov) 2012-04-12 04:27:04 PDT
Comment on attachment 136872 [details]
Patch

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

I believe it's ready for landing!

> Source/WebCore/ChangeLog:7
> +        highlightResults is changed in "last changed first corrected" order to preserve the semantic of node.

optional: I'd replace the word "semantic" by "structure" for clarity.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:637
> +        // Preserve the semantic of node by following the order of updates for hide and show.

Ditto.
Comment 8 WebKit Review Bot 2012-04-12 06:40:11 PDT
Comment on attachment 136872 [details]
Patch

Clearing flags on attachment: 136872

Committed r113970: <http://trac.webkit.org/changeset/113970>
Comment 9 WebKit Review Bot 2012-04-12 06:40:17 PDT
All reviewed patches have been landed.  Closing bug.