Find In Page doesn't work properly when " " is searched

Find In Page doesn't work properly when " " is searched

In case of replaced elements or elements with white-space
property 'PRE', extra space is emitted only when their text
starts with a white-space which is collapsed because of the
preceding white-space.

BUG=510335
Committed: https://crrev.com/6a04368503ae7e48422a851d39afc920ee66502d
Cr-Commit-Position: refs/heads/master@{#370362}

[ramya.v, 2015-12-01 11:30:41]

Description was changed from

==========
Find In Page doesn't work properly when " " is searched

When whitespace spawns the shadow dom trees, range is
calculated as collapsed and further searches were also
ignored. Allowing to continue search if the search is
terminated before covering search range.

BUG=510335

Signed-off-by: Ramya Vadlamudi <ramya.v@samsung.com>
==========

to

==========
Find In Page doesn't work properly when " " is searched

When whitespace spawns the shadow dom trees, range is
calculated as collapsed and further searches were also
ignored. Allowing to continue search if the search is
terminated before covering search range.

BUG=510335
==========

[ramya.v, 2015-12-09 10:45:38]

ramya.v@samsung.com changed reviewers:
+ tkent@chromium.org, yosin@chromium.org

[ramya.v, 2015-12-09 10:45:39]

PTAL!

Thanks

[ramya.v, 2015-12-15 00:30:13]

On 2015/12/09 10:45:39, ramya.v wrote:
> PTAL!
> 
> Thanks

Friendly ping. PTAL!

Thanks!

[yosin_UTC9, 2016-01-05 08:30:40]

It seems once we use composed tree version of TextFinder, this issue goes away.

So, let's use composed tree version of scopeStringMatches() in TextFinder.cpp.
More explicit, please rewrite TextFinder.cpp as if
RuntimeEnabledFeatures::selectionForComposedTreeEnabled() is always true.

It is time to remove this runtime flag.

https://codereview.chromium.org/1483013003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/TextFinder.cpp (right):

https://codereview.chromium.org/1483013003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/TextFinder.cpp:310: // FIXME: Show such matches to
users.
Please use "TODO(email):" style.
https://google.github.io/styleguide/cppguide.html#TODO_Comments

[ramya.v, 2016-01-06 08:28:44]

@Yosin

Hi Yosin didn't get you exactly. Did you mean to say with composed tree version
of textFinder, without any changes this issue will be fixed?

I checked in android and linux, where selectionForComposedTreeEnabled returns
true always, without the changes in this patch, issue is still present.

Can you please clarify what needs to be done w.r.t TextFinder?

https://codereview.chromium.org/1483013003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/TextFinder.cpp (right):

https://codereview.chromium.org/1483013003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/TextFinder.cpp:310: // FIXME: Show such matches to
users.
On 2016/01/05 08:30:40, Yosi_UTC9 wrote:
> Please use "TODO(email):" style.
> https://google.github.io/styleguide/cppguide.html#TODO_Comments

Done.

[yosin_UTC9, 2016-01-07 08:10:48]

On 2016/01/06 at 08:28:44, ramya.v wrote:
> @Yosin
> 
> Hi Yosin didn't get you exactly. Did you mean to say with composed tree
version of textFinder, without any changes this issue will be fixed?
> 
> I checked in android and linux, where selectionForComposedTreeEnabled returns
true always, without the changes in this patch, issue is still present.
> 
> Can you please clarify what needs to be done w.r.t TextFinder?
> 
>
https://codereview.chromium.org/1483013003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/TextFinder.cpp (right):
> 
>
https://codereview.chromium.org/1483013003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/TextFinder.cpp:310: // FIXME: Show such matches
to users.
> On 2016/01/05 08:30:40, Yosi_UTC9 wrote:
> > Please use "TODO(email):" style.
> > https://google.github.io/styleguide/cppguide.html#TODO_Comments
> 
> Done.

Sorry, for confusion. Yes, |selectionForComposedTreeEnabled| is always true now.

So, my question is why |findPlainText()| stops, == returns collapsed range, for
shadow DOM tree according to your comment:
// result will be collapsed if whitespace spans over multiple TreeScopes.

Could you explain more details? It seems TextIterator handles <input
type="button"> as replaced element and TextIterator
doesn't traverse shadow DOM tree(= composed tree).

[ramya.v, 2016-01-07 09:00:05]

On 2016/01/07 08:10:48, Yosi_UTC9 wrote:
> On 2016/01/06 at 08:28:44, ramya.v wrote:
> > @Yosin
> > 
> > Hi Yosin didn't get you exactly. Did you mean to say with composed tree
> version of textFinder, without any changes this issue will be fixed?
> > 
> > I checked in android and linux, where selectionForComposedTreeEnabled
returns
> true always, without the changes in this patch, issue is still present.
> > 
> > Can you please clarify what needs to be done w.r.t TextFinder?
> > 
> >
>
https://codereview.chromium.org/1483013003/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/web/TextFinder.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1483013003/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/web/TextFinder.cpp:310: // FIXME: Show such
matches
> to users.
> > On 2016/01/05 08:30:40, Yosi_UTC9 wrote:
> > > Please use "TODO(email):" style.
> > > https://google.github.io/styleguide/cppguide.html#TODO_Comments
> > 
> > Done.
> 
> Sorry, for confusion. Yes, |selectionForComposedTreeEnabled| is always true
now.
> 
> So, my question is why |findPlainText()| stops, == returns collapsed range,
for
> shadow DOM tree according to your comment:
> // result will be collapsed if whitespace spans over multiple TreeScopes.
> 
> Could you explain more details? It seems TextIterator handles <input
> type="button"> as replaced element and TextIterator
> doesn't traverse shadow DOM tree(= composed tree).



Issue is happening only in case if whitespace is present before <input> element
and after text in first span element. If either of whitespace is not present
issue is not observed. Able to traverse shadow DOM as well as content after
that. Otherwise it is stopped at whitespace collapsing boundary (between span1
and input)

If we search for " " 

Ex1: search terminates before button.
<span>Some text </span> <input type='button' value='Button text'/><span>Some
more text</span>

Ex2: search continues till end.
<span>Some text</span> <input type='button' value='Button text'/><span>Some more
text</span>
<span>Some text </span><input type='button' value='Button text'/><span>Some more
text</span>
<span>Some text</span> <input type='button' value='Button text'/><span>Some more
text</span>

[yosin_UTC9, 2016-01-07 09:31:29]

yosin@chromium.org changed reviewers:
+ xiaochengh@chromium.org

[yosin_UTC9, 2016-01-07 09:31:44]

+xiaochengh@, he is an expert of TextIterator.

[Xiaocheng, 2016-01-08 08:07:32]

On 2016/01/07 at 09:31:44, yosin wrote:
> +xiaochengh@, he is an expert of TextIterator.

I'm confused by the behavior of TextIterator.

I added some code at the beginning of findPlainTextAlgorithm, which uses a
TextIterator to emit every character in inputRange. Then I found that a
seemingly extra space is emitted, which is deemed a match of " " by
findPlainTextAlgorithm. But this space is not visible at all! So a collapsed
range is created and returned, which causes the bug.

I'm not sure why TextIterator emits this extra space. At least in our case, the
emitting of this space is quite unreasonable. Is it for some other purposes in
some other scenario? Or is it just a bug...



The code I added at the beginning of findPlainTextAlgorithm:

    WTFLogAlways("Invoking findPlainTextAlgorithm with the following input
Range");
    inputRange.startPosition().showTreeForThis();
    inputRange.endPosition().showTreeForThis();
    {
        WTFLogAlways("Dumping text in the range...");
        TextIteratorBehaviorFlags behavior = iteratorFlagsForFindPlainText;
        if (options & FindAPICall)
            behavior |= TextIteratorForWindowFind;
        CharacterIteratorAlgorithm<Strategy> dumpIterator(inputRange, behavior);
        while (!dumpIterator.atEnd())
        {
            WTFLogAlways("%d", dumpIterator.characterAt(0));
            dumpIterator.advance(1);
        }
        WTFLogAlways("Dump end");
    }



When searching " " in Ex1 of #9, the above code prints the following. Note that
two consecutive "32"s (i.e., spaces) are emitted!

Invoking findPlainTextAlgorithm with the following input Range
*#document	0x5cbe7004010
	html	0x5cbe701c010
	HTML	0x5cbe7010120
		HEAD	0x5cbe7010098
		BODY	0x5cbe7010010
			SPAN	0x5cbe70101a8
				#text	0x5cbe7020010 "Some text "
			#text	0x5cbe7020080 " "
			INPUT	0x5cbe7030010
				#document-fragment	0x5cbe703c010
					#text	0x5cbe70200f0 "Button text"
			SPAN	0x5cbe7010230
				#text	0x5cbe7020160 "Some more text"
			#text	0x5cbe70201d0 "\n"
beforeChildren, offset:0
*#document	0x5cbe7004010
	html	0x5cbe701c010
	HTML	0x5cbe7010120
		HEAD	0x5cbe7010098
		BODY	0x5cbe7010010
			SPAN	0x5cbe70101a8
				#text	0x5cbe7020010 "Some text "
			#text	0x5cbe7020080 " "
			INPUT	0x5cbe7030010
				#document-fragment	0x5cbe703c010
					#text	0x5cbe70200f0 "Button text"
			SPAN	0x5cbe7010230
				#text	0x5cbe7020160 "Some more text"
			#text	0x5cbe70201d0 "\n"
afterChildren, offset:0
Dumping text in the range...
83
111
109
101
32
116
101
120
116
32
32
66
117
116
116
111
110
32
116
101
120
116
83
111
109
101
32
109
111
114
101
32
116
101
120
116
Dump end

[ramya.v, 2016-01-11 10:45:52]

On 2016/01/08 08:07:32, Xiaocheng wrote:
> On 2016/01/07 at 09:31:44, yosin wrote:
> > +xiaochengh@, he is an expert of TextIterator.
> 
> I'm confused by the behavior of TextIterator.
> 
> I added some code at the beginning of findPlainTextAlgorithm, which uses a
> TextIterator to emit every character in inputRange. Then I found that a
> seemingly extra space is emitted, which is deemed a match of " " by
> findPlainTextAlgorithm. But this space is not visible at all! So a collapsed
> range is created and returned, which causes the bug.
> 
> I'm not sure why TextIterator emits this extra space. At least in our case,
the
> emitting of this space is quite unreasonable. Is it for some other purposes in
> some other scenario? Or is it just a bug...
> 
> 
> 
> The code I added at the beginning of findPlainTextAlgorithm:
> 
>     WTFLogAlways("Invoking findPlainTextAlgorithm with the following input
> Range");
>     inputRange.startPosition().showTreeForThis();
>     inputRange.endPosition().showTreeForThis();
>     {
>         WTFLogAlways("Dumping text in the range...");
>         TextIteratorBehaviorFlags behavior = iteratorFlagsForFindPlainText;
>         if (options & FindAPICall)
>             behavior |= TextIteratorForWindowFind;
>         CharacterIteratorAlgorithm<Strategy> dumpIterator(inputRange,
behavior);
>         while (!dumpIterator.atEnd())
>         {
>             WTFLogAlways("%d", dumpIterator.characterAt(0));
>             dumpIterator.advance(1);
>         }
>         WTFLogAlways("Dump end");
>     }
> 
> 
> 
> When searching " " in Ex1 of #9, the above code prints the following. Note
that
> two consecutive "32"s (i.e., spaces) are emitted!
> 
> Invoking findPlainTextAlgorithm with the following input Range
> *#document	0x5cbe7004010
> 	html	0x5cbe701c010
> 	HTML	0x5cbe7010120
> 		HEAD	0x5cbe7010098
> 		BODY	0x5cbe7010010
> 			SPAN	0x5cbe70101a8
> 				#text	0x5cbe7020010 "Some text "
> 			#text	0x5cbe7020080 " "
> 			INPUT	0x5cbe7030010
> 				#document-fragment	0x5cbe703c010
> 					#text	0x5cbe70200f0 "Button text"
> 			SPAN	0x5cbe7010230
> 				#text	0x5cbe7020160 "Some more text"
> 			#text	0x5cbe70201d0 "\n"
> beforeChildren, offset:0
> *#document	0x5cbe7004010
> 	html	0x5cbe701c010
> 	HTML	0x5cbe7010120
> 		HEAD	0x5cbe7010098
> 		BODY	0x5cbe7010010
> 			SPAN	0x5cbe70101a8
> 				#text	0x5cbe7020010 "Some text "
> 			#text	0x5cbe7020080 " "
> 			INPUT	0x5cbe7030010
> 				#document-fragment	0x5cbe703c010
> 					#text	0x5cbe70200f0 "Button text"
> 			SPAN	0x5cbe7010230
> 				#text	0x5cbe7020160 "Some more text"
> 			#text	0x5cbe70201d0 "\n"
> afterChildren, offset:0
> Dumping text in the range...
> 83
> 111
> 109
> 101
> 32
> 116
> 101
> 120
> 116
> 32
> 32
> 66
> 117
> 116
> 116
> 111
> 110
> 32
> 116
> 101
> 120
> 116
> 83
> 111
> 109
> 101
> 32
> 109
> 111
> 114
> 101
> 32
> 116
> 101
> 120
> 116
> Dump end




Ex1: <span>Some text </span> <img src="abc.png"/>
Ex2: <span>Some text </span> <input type="button" value=" Button text"/>

Extra space is emitted from the below line
(TextIterator::handleReplacedElement). Not sure of the exact reason why this
part of code is added
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

If we comment above block Ex1 works fine. For Ex2: even if we comment, extra
space is emitted from below location (EWhitespace is calculated as PRE)
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Xiaocheng, 2016-01-14 01:13:58]

On 2016/01/11 at 10:45:52, ramya.v wrote:
> On 2016/01/08 08:07:32, Xiaocheng wrote:
> > On 2016/01/07 at 09:31:44, yosin wrote:
> > > +xiaochengh@, he is an expert of TextIterator.
> > 
> > I'm confused by the behavior of TextIterator.
> > 
> > I added some code at the beginning of findPlainTextAlgorithm, which uses a
> > TextIterator to emit every character in inputRange. Then I found that a
> > seemingly extra space is emitted, which is deemed a match of " " by
> > findPlainTextAlgorithm. But this space is not visible at all! So a collapsed
> > range is created and returned, which causes the bug.
> > 
> > I'm not sure why TextIterator emits this extra space. At least in our case,
the
> > emitting of this space is quite unreasonable. Is it for some other purposes
in
> > some other scenario? Or is it just a bug...
> > 
> > 
> > 
> > The code I added at the beginning of findPlainTextAlgorithm:
> > 
> >     WTFLogAlways("Invoking findPlainTextAlgorithm with the following input
> > Range");
> >     inputRange.startPosition().showTreeForThis();
> >     inputRange.endPosition().showTreeForThis();
> >     {
> >         WTFLogAlways("Dumping text in the range...");
> >         TextIteratorBehaviorFlags behavior = iteratorFlagsForFindPlainText;
> >         if (options & FindAPICall)
> >             behavior |= TextIteratorForWindowFind;
> >         CharacterIteratorAlgorithm<Strategy> dumpIterator(inputRange,
behavior);
> >         while (!dumpIterator.atEnd())
> >         {
> >             WTFLogAlways("%d", dumpIterator.characterAt(0));
> >             dumpIterator.advance(1);
> >         }
> >         WTFLogAlways("Dump end");
> >     }
> > 
> > 
> > 
> > When searching " " in Ex1 of #9, the above code prints the following. Note
that
> > two consecutive "32"s (i.e., spaces) are emitted!
> > 
> > Invoking findPlainTextAlgorithm with the following input Range
> > *#document	0x5cbe7004010
> > 	html	0x5cbe701c010
> > 	HTML	0x5cbe7010120
> > 		HEAD	0x5cbe7010098
> > 		BODY	0x5cbe7010010
> > 			SPAN	0x5cbe70101a8
> > 				#text	0x5cbe7020010 "Some text "
> > 			#text	0x5cbe7020080 " "
> > 			INPUT	0x5cbe7030010
> > 				#document-fragment	0x5cbe703c010
> > 					#text	0x5cbe70200f0 "Button text"
> > 			SPAN	0x5cbe7010230
> > 				#text	0x5cbe7020160 "Some more text"
> > 			#text	0x5cbe70201d0 "\n"
> > beforeChildren, offset:0
> > *#document	0x5cbe7004010
> > 	html	0x5cbe701c010
> > 	HTML	0x5cbe7010120
> > 		HEAD	0x5cbe7010098
> > 		BODY	0x5cbe7010010
> > 			SPAN	0x5cbe70101a8
> > 				#text	0x5cbe7020010 "Some text "
> > 			#text	0x5cbe7020080 " "
> > 			INPUT	0x5cbe7030010
> > 				#document-fragment	0x5cbe703c010
> > 					#text	0x5cbe70200f0 "Button text"
> > 			SPAN	0x5cbe7010230
> > 				#text	0x5cbe7020160 "Some more text"
> > 			#text	0x5cbe70201d0 "\n"
> > afterChildren, offset:0
> > Dumping text in the range...
> > 83
> > 111
> > 109
> > 101
> > 32
> > 116
> > 101
> > 120
> > 116
> > 32
> > 32
> > 66
> > 117
> > 116
> > 116
> > 111
> > 110
> > 32
> > 116
> > 101
> > 120
> > 116
> > 83
> > 111
> > 109
> > 101
> > 32
> > 109
> > 111
> > 114
> > 101
> > 32
> > 116
> > 101
> > 120
> > 116
> > Dump end
> 
> 
> 
> 
> Ex1: <span>Some text </span> <img src="abc.png"/>
> Ex2: <span>Some text </span> <input type="button" value=" Button text"/>
> 
> Extra space is emitted from the below line
(TextIterator::handleReplacedElement). Not sure of the exact reason why this
part of code is added
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> If we comment above block Ex1 works fine. For Ex2: even if we comment, extra
space is emitted from below location (EWhitespace is calculated as PRE)
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Thanks for finding these two lines.

So it seems that the emitting of the extra space is an intended, though quite
counterintuitive, behavior.

Maybe we should figure out the purpose of these two lines later to see if there
is any mistake.

For now, the approach of this path (having an extra check when |findPlainText|
returns a collapsed range) looks like a good enough quick fix to me.

[ramya.v, 2016-01-18 04:19:44]

On 2016/01/14 01:13:58, Xiaocheng wrote:
> On 2016/01/11 at 10:45:52, ramya.v wrote:
> > On 2016/01/08 08:07:32, Xiaocheng wrote:
> > > On 2016/01/07 at 09:31:44, yosin wrote:
> > > > +xiaochengh@, he is an expert of TextIterator.
> > > 
> > > I'm confused by the behavior of TextIterator.
> > > 
> > > I added some code at the beginning of findPlainTextAlgorithm, which uses a
> > > TextIterator to emit every character in inputRange. Then I found that a
> > > seemingly extra space is emitted, which is deemed a match of " " by
> > > findPlainTextAlgorithm. But this space is not visible at all! So a
collapsed
> > > range is created and returned, which causes the bug.
> > > 
> > > I'm not sure why TextIterator emits this extra space. At least in our
case,
> the
> > > emitting of this space is quite unreasonable. Is it for some other
purposes
> in
> > > some other scenario? Or is it just a bug...
> > > 
> > > 
> > > 
> > > The code I added at the beginning of findPlainTextAlgorithm:
> > > 
> > >     WTFLogAlways("Invoking findPlainTextAlgorithm with the following input
> > > Range");
> > >     inputRange.startPosition().showTreeForThis();
> > >     inputRange.endPosition().showTreeForThis();
> > >     {
> > >         WTFLogAlways("Dumping text in the range...");
> > >         TextIteratorBehaviorFlags behavior =
iteratorFlagsForFindPlainText;
> > >         if (options & FindAPICall)
> > >             behavior |= TextIteratorForWindowFind;
> > >         CharacterIteratorAlgorithm<Strategy> dumpIterator(inputRange,
> behavior);
> > >         while (!dumpIterator.atEnd())
> > >         {
> > >             WTFLogAlways("%d", dumpIterator.characterAt(0));
> > >             dumpIterator.advance(1);
> > >         }
> > >         WTFLogAlways("Dump end");
> > >     }
> > > 
> > > 
> > > 
> > > When searching " " in Ex1 of #9, the above code prints the following. Note
> that
> > > two consecutive "32"s (i.e., spaces) are emitted!
> > > 
> > > Invoking findPlainTextAlgorithm with the following input Range
> > > *#document	0x5cbe7004010
> > > 	html	0x5cbe701c010
> > > 	HTML	0x5cbe7010120
> > > 		HEAD	0x5cbe7010098
> > > 		BODY	0x5cbe7010010
> > > 			SPAN	0x5cbe70101a8
> > > 				#text	0x5cbe7020010 "Some text "
> > > 			#text	0x5cbe7020080 " "
> > > 			INPUT	0x5cbe7030010
> > > 				#document-fragment	0x5cbe703c010
> > > 					#text	0x5cbe70200f0 "Button text"
> > > 			SPAN	0x5cbe7010230
> > > 				#text	0x5cbe7020160 "Some more text"
> > > 			#text	0x5cbe70201d0 "\n"
> > > beforeChildren, offset:0
> > > *#document	0x5cbe7004010
> > > 	html	0x5cbe701c010
> > > 	HTML	0x5cbe7010120
> > > 		HEAD	0x5cbe7010098
> > > 		BODY	0x5cbe7010010
> > > 			SPAN	0x5cbe70101a8
> > > 				#text	0x5cbe7020010 "Some text "
> > > 			#text	0x5cbe7020080 " "
> > > 			INPUT	0x5cbe7030010
> > > 				#document-fragment	0x5cbe703c010
> > > 					#text	0x5cbe70200f0 "Button text"
> > > 			SPAN	0x5cbe7010230
> > > 				#text	0x5cbe7020160 "Some more text"
> > > 			#text	0x5cbe70201d0 "\n"
> > > afterChildren, offset:0
> > > Dumping text in the range...
> > > 83
> > > 111
> > > 109
> > > 101
> > > 32
> > > 116
> > > 101
> > > 120
> > > 116
> > > 32
> > > 32
> > > 66
> > > 117
> > > 116
> > > 116
> > > 111
> > > 110
> > > 32
> > > 116
> > > 101
> > > 120
> > > 116
> > > 83
> > > 111
> > > 109
> > > 101
> > > 32
> > > 109
> > > 111
> > > 114
> > > 101
> > > 32
> > > 116
> > > 101
> > > 120
> > > 116
> > > Dump end
> > 
> > 
> > 
> > 
> > Ex1: <span>Some text </span> <img src="abc.png"/>
> > Ex2: <span>Some text </span> <input type="button" value=" Button text"/>
> > 
> > Extra space is emitted from the below line
> (TextIterator::handleReplacedElement). Not sure of the exact reason why this
> part of code is added
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > If we comment above block Ex1 works fine. For Ex2: even if we comment, extra
> space is emitted from below location (EWhitespace is calculated as PRE)
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Thanks for finding these two lines.
> 
> So it seems that the emitting of the extra space is an intended, though quite
> counterintuitive, behavior.
> 
> Maybe we should figure out the purpose of these two lines later to see if
there
> is any mistake.
> 
> For now, the approach of this path (having an extra check when |findPlainText|
> returns a collapsed range) looks like a good enough quick fix to me.



Thanks Xiaocheng for the review comments.
@Yosin/tkent can you please let me know your opinion about taking this patch for
now?

[yosin_UTC9, 2016-01-18 04:54:20]

On 2016/01/18 at 04:19:44, ramya.v wrote:
> On 2016/01/14 01:13:58, Xiaocheng wrote:
> > On 2016/01/11 at 10:45:52, ramya.v wrote:
> > > On 2016/01/08 08:07:32, Xiaocheng wrote:
> > > > On 2016/01/07 at 09:31:44, yosin wrote:
> > > > > +xiaochengh@, he is an expert of TextIterator.
> > > > 
> > > > I'm confused by the behavior of TextIterator.
> > > > 
> > > > I added some code at the beginning of findPlainTextAlgorithm, which uses
a
> > > > TextIterator to emit every character in inputRange. Then I found that a
> > > > seemingly extra space is emitted, which is deemed a match of " " by
> > > > findPlainTextAlgorithm. But this space is not visible at all! So a
collapsed
> > > > range is created and returned, which causes the bug.
> > > > 
> > > > I'm not sure why TextIterator emits this extra space. At least in our
case,
> > the
> > > > emitting of this space is quite unreasonable. Is it for some other
purposes
> > in
> > > > some other scenario? Or is it just a bug...
> > > > 
> > > > 
> > > > 
> > > > The code I added at the beginning of findPlainTextAlgorithm:
> > > > 
> > > >     WTFLogAlways("Invoking findPlainTextAlgorithm with the following
input
> > > > Range");
> > > >     inputRange.startPosition().showTreeForThis();
> > > >     inputRange.endPosition().showTreeForThis();
> > > >     {
> > > >         WTFLogAlways("Dumping text in the range...");
> > > >         TextIteratorBehaviorFlags behavior =
iteratorFlagsForFindPlainText;
> > > >         if (options & FindAPICall)
> > > >             behavior |= TextIteratorForWindowFind;
> > > >         CharacterIteratorAlgorithm<Strategy> dumpIterator(inputRange,
> > behavior);
> > > >         while (!dumpIterator.atEnd())
> > > >         {
> > > >             WTFLogAlways("%d", dumpIterator.characterAt(0));
> > > >             dumpIterator.advance(1);
> > > >         }
> > > >         WTFLogAlways("Dump end");
> > > >     }
> > > > 
> > > > 
> > > > 
> > > > When searching " " in Ex1 of #9, the above code prints the following.
Note
> > that
> > > > two consecutive "32"s (i.e., spaces) are emitted!
> > > > 
> > > > Invoking findPlainTextAlgorithm with the following input Range
> > > > *#document	0x5cbe7004010
> > > > 	html	0x5cbe701c010
> > > > 	HTML	0x5cbe7010120
> > > > 		HEAD	0x5cbe7010098
> > > > 		BODY	0x5cbe7010010
> > > > 			SPAN	0x5cbe70101a8
> > > > 				#text	0x5cbe7020010 "Some text "
> > > > 			#text	0x5cbe7020080 " "
> > > > 			INPUT	0x5cbe7030010
> > > > 				#document-fragment	0x5cbe703c010
> > > > 					#text	0x5cbe70200f0 "Button text"
> > > > 			SPAN	0x5cbe7010230
> > > > 				#text	0x5cbe7020160 "Some more text"
> > > > 			#text	0x5cbe70201d0 "\n"
> > > > beforeChildren, offset:0
> > > > *#document	0x5cbe7004010
> > > > 	html	0x5cbe701c010
> > > > 	HTML	0x5cbe7010120
> > > > 		HEAD	0x5cbe7010098
> > > > 		BODY	0x5cbe7010010
> > > > 			SPAN	0x5cbe70101a8
> > > > 				#text	0x5cbe7020010 "Some text "
> > > > 			#text	0x5cbe7020080 " "
> > > > 			INPUT	0x5cbe7030010
> > > > 				#document-fragment	0x5cbe703c010
> > > > 					#text	0x5cbe70200f0 "Button text"
> > > > 			SPAN	0x5cbe7010230
> > > > 				#text	0x5cbe7020160 "Some more text"
> > > > 			#text	0x5cbe70201d0 "\n"
> > > > afterChildren, offset:0
> > > > Dumping text in the range...
> > > > 83
> > > > 111
> > > > 109
> > > > 101
> > > > 32
> > > > 116
> > > > 101
> > > > 120
> > > > 116
> > > > 32
> > > > 32
> > > > 66
> > > > 117
> > > > 116
> > > > 116
> > > > 111
> > > > 110
> > > > 32
> > > > 116
> > > > 101
> > > > 120
> > > > 116
> > > > 83
> > > > 111
> > > > 109
> > > > 101
> > > > 32
> > > > 109
> > > > 111
> > > > 114
> > > > 101
> > > > 32
> > > > 116
> > > > 101
> > > > 120
> > > > 116
> > > > Dump end
> > > 
> > > 
> > > 
> > > 
> > > Ex1: <span>Some text </span> <img src="abc.png"/>
> > > Ex2: <span>Some text </span> <input type="button" value=" Button text"/>
> > > 
> > > Extra space is emitted from the below line
> > (TextIterator::handleReplacedElement). Not sure of the exact reason why this
> > part of code is added
> > >
> >
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > If we comment above block Ex1 works fine. For Ex2: even if we comment,
extra
> > space is emitted from below location (EWhitespace is calculated as PRE)
> > >
> >
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Thanks for finding these two lines.
> > 
> > So it seems that the emitting of the extra space is an intended, though
quite
> > counterintuitive, behavior.
> > 
> > Maybe we should figure out the purpose of these two lines later to see if
there
> > is any mistake.
> > 
> > For now, the approach of this path (having an extra check when
|findPlainText|
> > returns a collapsed range) looks like a good enough quick fix to me.
> 
> 
> 
> Thanks Xiaocheng for the review comments.
> @Yosin/tkent can you please let me know your opinion about taking this patch
for now?

Sorry for late response.
I thinks we should change |TextIterator| to handling this situation. I know
|TextIterator| is very complicated but now is time to make |TextIterator| more
clean and work correctly.

[ramya.v, 2016-01-19 04:31:46]

On 2016/01/18 04:54:20, Yosi_UTC9 wrote:
> On 2016/01/18 at 04:19:44, ramya.v wrote:
> > On 2016/01/14 01:13:58, Xiaocheng wrote:
> > > On 2016/01/11 at 10:45:52, ramya.v wrote:
> > > > On 2016/01/08 08:07:32, Xiaocheng wrote:
> > > > > On 2016/01/07 at 09:31:44, yosin wrote:
> > > > > > +xiaochengh@, he is an expert of TextIterator.
> > > > > 
> > > > > I'm confused by the behavior of TextIterator.
> > > > > 
> > > > > I added some code at the beginning of findPlainTextAlgorithm, which
uses
> a
> > > > > TextIterator to emit every character in inputRange. Then I found that
a
> > > > > seemingly extra space is emitted, which is deemed a match of " " by
> > > > > findPlainTextAlgorithm. But this space is not visible at all! So a
> collapsed
> > > > > range is created and returned, which causes the bug.
> > > > > 
> > > > > I'm not sure why TextIterator emits this extra space. At least in our
> case,
> > > the
> > > > > emitting of this space is quite unreasonable. Is it for some other
> purposes
> > > in
> > > > > some other scenario? Or is it just a bug...
> > > > > 
> > > > > 
> > > > > 
> > > > > The code I added at the beginning of findPlainTextAlgorithm:
> > > > > 
> > > > >     WTFLogAlways("Invoking findPlainTextAlgorithm with the following
> input
> > > > > Range");
> > > > >     inputRange.startPosition().showTreeForThis();
> > > > >     inputRange.endPosition().showTreeForThis();
> > > > >     {
> > > > >         WTFLogAlways("Dumping text in the range...");
> > > > >         TextIteratorBehaviorFlags behavior =
> iteratorFlagsForFindPlainText;
> > > > >         if (options & FindAPICall)
> > > > >             behavior |= TextIteratorForWindowFind;
> > > > >         CharacterIteratorAlgorithm<Strategy> dumpIterator(inputRange,
> > > behavior);
> > > > >         while (!dumpIterator.atEnd())
> > > > >         {
> > > > >             WTFLogAlways("%d", dumpIterator.characterAt(0));
> > > > >             dumpIterator.advance(1);
> > > > >         }
> > > > >         WTFLogAlways("Dump end");
> > > > >     }
> > > > > 
> > > > > 
> > > > > 
> > > > > When searching " " in Ex1 of #9, the above code prints the following.
> Note
> > > that
> > > > > two consecutive "32"s (i.e., spaces) are emitted!
> > > > > 
> > > > > Invoking findPlainTextAlgorithm with the following input Range
> > > > > *#document	0x5cbe7004010
> > > > > 	html	0x5cbe701c010
> > > > > 	HTML	0x5cbe7010120
> > > > > 		HEAD	0x5cbe7010098
> > > > > 		BODY	0x5cbe7010010
> > > > > 			SPAN	0x5cbe70101a8
> > > > > 				#text	0x5cbe7020010 "Some text "
> > > > > 			#text	0x5cbe7020080 " "
> > > > > 			INPUT	0x5cbe7030010
> > > > > 				#document-fragment	0x5cbe703c010
> > > > > 					#text	0x5cbe70200f0 "Button text"
> > > > > 			SPAN	0x5cbe7010230
> > > > > 				#text	0x5cbe7020160 "Some more text"
> > > > > 			#text	0x5cbe70201d0 "\n"
> > > > > beforeChildren, offset:0
> > > > > *#document	0x5cbe7004010
> > > > > 	html	0x5cbe701c010
> > > > > 	HTML	0x5cbe7010120
> > > > > 		HEAD	0x5cbe7010098
> > > > > 		BODY	0x5cbe7010010
> > > > > 			SPAN	0x5cbe70101a8
> > > > > 				#text	0x5cbe7020010 "Some text "
> > > > > 			#text	0x5cbe7020080 " "
> > > > > 			INPUT	0x5cbe7030010
> > > > > 				#document-fragment	0x5cbe703c010
> > > > > 					#text	0x5cbe70200f0 "Button text"
> > > > > 			SPAN	0x5cbe7010230
> > > > > 				#text	0x5cbe7020160 "Some more text"
> > > > > 			#text	0x5cbe70201d0 "\n"
> > > > > afterChildren, offset:0
> > > > > Dumping text in the range...
> > > > > 83
> > > > > 111
> > > > > 109
> > > > > 101
> > > > > 32
> > > > > 116
> > > > > 101
> > > > > 120
> > > > > 116
> > > > > 32
> > > > > 32
> > > > > 66
> > > > > 117
> > > > > 116
> > > > > 116
> > > > > 111
> > > > > 110
> > > > > 32
> > > > > 116
> > > > > 101
> > > > > 120
> > > > > 116
> > > > > 83
> > > > > 111
> > > > > 109
> > > > > 101
> > > > > 32
> > > > > 109
> > > > > 111
> > > > > 114
> > > > > 101
> > > > > 32
> > > > > 116
> > > > > 101
> > > > > 120
> > > > > 116
> > > > > Dump end
> > > > 
> > > > 
> > > > 
> > > > 
> > > > Ex1: <span>Some text </span> <img src="abc.png"/>
> > > > Ex2: <span>Some text </span> <input type="button" value=" Button text"/>
> > > > 
> > > > Extra space is emitted from the below line
> > > (TextIterator::handleReplacedElement). Not sure of the exact reason why
this
> > > part of code is added
> > > >
> > >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > 
> > > > If we comment above block Ex1 works fine. For Ex2: even if we comment,
> extra
> > > space is emitted from below location (EWhitespace is calculated as PRE)
> > > >
> > >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > Thanks for finding these two lines.
> > > 
> > > So it seems that the emitting of the extra space is an intended, though
> quite
> > > counterintuitive, behavior.
> > > 
> > > Maybe we should figure out the purpose of these two lines later to see if
> there
> > > is any mistake.
> > > 
> > > For now, the approach of this path (having an extra check when
> |findPlainText|
> > > returns a collapsed range) looks like a good enough quick fix to me.
> > 
> > 
> > 
> > Thanks Xiaocheng for the review comments.
> > @Yosin/tkent can you please let me know your opinion about taking this patch
> for now?
> 
> Sorry for late response.
> I thinks we should change |TextIterator| to handling this situation. I know
> |TextIterator| is very complicated but now is time to make |TextIterator| more
> clean and work correctly.





@Yosin/Xiaocheng

Added new patchset4 where in tried changing TextIterator to handle this
situation. As of now even if the text inside button or img doesnot start with "
" if white space is collapsed before the replaced element one extra space is
inserted. To avoid this incorporated changes so as to check if the character at
current offset is space and only then emit an extra space.

Please let me know if this approach is fine?


Trybot results on linux shows total 519 text failures. (All are related to
document element innerText white space difference, not valid failures of
testcases)
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...


Following 24 testcases passed unexpectedly.

virtual/rootlayerscrolls/fast/scrolling/fractional-scroll-offset-fixed-position-non-composited.html
virtual/spv2/paint/paintlayer/non-self-painting-layer-overrides-visibility.html
virtual/spv2/paint/invalidation/invalidate-when-receiving-paint-layer.html
virtual/spv2/paint/invalidation/repaint-subsequence-on-ancestor-clip-change.html
http/tests/security/contentTypeOptions/nosniff-script-without-content-type-blocked.html
editing/pasteboard/4944770-2.html
http/tests/inspector/service-workers/user-agent-override.html
fast/css/absolute-inline-alignment.html
http/tests/media/progress-events-generated-correctly.html
fast/encoding/invalid-UTF-8.html
fast/forms/month/month-appearance-l10n.html
fast/inline/nested-text-descendants.html
http/tests/xmlviewer/dumpAsText/svg.xml
fast/text/international/bdi-dir-default-to-auto.html
fast/text/ellipsis-stroked.html
fast/text/emoticons.html
fast/text/international/hindi-spacing.html
fast/text/international/inline-plaintext-is-isolated.html
fast/text/selection-multiple-runs.html
ietestcenter/css3/multicolumn/column-width-applies-to-007.htm
ietestcenter/css3/multicolumn/column-width-applies-to-009.htm
ietestcenter/css3/multicolumn/column-width-applies-to-012.htm
ietestcenter/css3/multicolumn/column-width-applies-to-015.htm
fast/text/unicode-variation-selector.html

[yosin_UTC9, 2016-01-19 09:49:28]

On 2016/01/19 at 04:31:46, ramya.v wrote:
> On 2016/01/18 04:54:20, Yosi_UTC9 wrote:
> > On 2016/01/18 at 04:19:44, ramya.v wrote:
> > > On 2016/01/14 01:13:58, Xiaocheng wrote:
> > > > On 2016/01/11 at 10:45:52, ramya.v wrote:
> > > > > On 2016/01/08 08:07:32, Xiaocheng wrote:
> > > > > > On 2016/01/07 at 09:31:44, yosin wrote:
> > > > > > > +xiaochengh@, he is an expert of TextIterator.
> > > > > > 
> > > > > > I'm confused by the behavior of TextIterator.
> > > > > > 
> > > > > > I added some code at the beginning of findPlainTextAlgorithm, which
uses
> > a
> > > > > > TextIterator to emit every character in inputRange. Then I found
that a
> > > > > > seemingly extra space is emitted, which is deemed a match of " " by
> > > > > > findPlainTextAlgorithm. But this space is not visible at all! So a
> > collapsed
> > > > > > range is created and returned, which causes the bug.
> > > > > > 
> > > > > > I'm not sure why TextIterator emits this extra space. At least in
our
> > case,
> > > > the
> > > > > > emitting of this space is quite unreasonable. Is it for some other
> > purposes
> > > > in
> > > > > > some other scenario? Or is it just a bug...
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > The code I added at the beginning of findPlainTextAlgorithm:
> > > > > > 
> > > > > >     WTFLogAlways("Invoking findPlainTextAlgorithm with the following
> > input
> > > > > > Range");
> > > > > >     inputRange.startPosition().showTreeForThis();
> > > > > >     inputRange.endPosition().showTreeForThis();
> > > > > >     {
> > > > > >         WTFLogAlways("Dumping text in the range...");
> > > > > >         TextIteratorBehaviorFlags behavior =
> > iteratorFlagsForFindPlainText;
> > > > > >         if (options & FindAPICall)
> > > > > >             behavior |= TextIteratorForWindowFind;
> > > > > >         CharacterIteratorAlgorithm<Strategy>
dumpIterator(inputRange,
> > > > behavior);
> > > > > >         while (!dumpIterator.atEnd())
> > > > > >         {
> > > > > >             WTFLogAlways("%d", dumpIterator.characterAt(0));
> > > > > >             dumpIterator.advance(1);
> > > > > >         }
> > > > > >         WTFLogAlways("Dump end");
> > > > > >     }
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > When searching " " in Ex1 of #9, the above code prints the
following.
> > Note
> > > > that
> > > > > > two consecutive "32"s (i.e., spaces) are emitted!
> > > > > > 
> > > > > > Invoking findPlainTextAlgorithm with the following input Range
> > > > > > *#document	0x5cbe7004010
> > > > > > 	html	0x5cbe701c010
> > > > > > 	HTML	0x5cbe7010120
> > > > > > 		HEAD	0x5cbe7010098
> > > > > > 		BODY	0x5cbe7010010
> > > > > > 			SPAN	0x5cbe70101a8
> > > > > > 				#text	0x5cbe7020010 "Some text "
> > > > > > 			#text	0x5cbe7020080 " "
> > > > > > 			INPUT	0x5cbe7030010
> > > > > > 				#document-fragment	0x5cbe703c010
> > > > > > 					#text	0x5cbe70200f0 "Button text"
> > > > > > 			SPAN	0x5cbe7010230
> > > > > > 				#text	0x5cbe7020160 "Some more text"
> > > > > > 			#text	0x5cbe70201d0 "\n"
> > > > > > beforeChildren, offset:0
> > > > > > *#document	0x5cbe7004010
> > > > > > 	html	0x5cbe701c010
> > > > > > 	HTML	0x5cbe7010120
> > > > > > 		HEAD	0x5cbe7010098
> > > > > > 		BODY	0x5cbe7010010
> > > > > > 			SPAN	0x5cbe70101a8
> > > > > > 				#text	0x5cbe7020010 "Some text "
> > > > > > 			#text	0x5cbe7020080 " "
> > > > > > 			INPUT	0x5cbe7030010
> > > > > > 				#document-fragment	0x5cbe703c010
> > > > > > 					#text	0x5cbe70200f0 "Button text"
> > > > > > 			SPAN	0x5cbe7010230
> > > > > > 				#text	0x5cbe7020160 "Some more text"
> > > > > > 			#text	0x5cbe70201d0 "\n"
> > > > > > afterChildren, offset:0
> > > > > > Dumping text in the range...
> > > > > > 83
> > > > > > 111
> > > > > > 109
> > > > > > 101
> > > > > > 32
> > > > > > 116
> > > > > > 101
> > > > > > 120
> > > > > > 116
> > > > > > 32
> > > > > > 32
> > > > > > 66
> > > > > > 117
> > > > > > 116
> > > > > > 116
> > > > > > 111
> > > > > > 110
> > > > > > 32
> > > > > > 116
> > > > > > 101
> > > > > > 120
> > > > > > 116
> > > > > > 83
> > > > > > 111
> > > > > > 109
> > > > > > 101
> > > > > > 32
> > > > > > 109
> > > > > > 111
> > > > > > 114
> > > > > > 101
> > > > > > 32
> > > > > > 116
> > > > > > 101
> > > > > > 120
> > > > > > 116
> > > > > > Dump end
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > Ex1: <span>Some text </span> <img src="abc.png"/>
> > > > > Ex2: <span>Some text </span> <input type="button" value=" Button
text"/>
> > > > > 
> > > > > Extra space is emitted from the below line
> > > > (TextIterator::handleReplacedElement). Not sure of the exact reason why
this
> > > > part of code is added
> > > > >
> > > >
> >
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > > 
> > > > > If we comment above block Ex1 works fine. For Ex2: even if we comment,
> > extra
> > > > space is emitted from below location (EWhitespace is calculated as PRE)
> > > > >
> > > >
> >
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > 
> > > > Thanks for finding these two lines.
> > > > 
> > > > So it seems that the emitting of the extra space is an intended, though
> > quite
> > > > counterintuitive, behavior.
> > > > 
> > > > Maybe we should figure out the purpose of these two lines later to see
if
> > there
> > > > is any mistake.
> > > > 
> > > > For now, the approach of this path (having an extra check when
> > |findPlainText|
> > > > returns a collapsed range) looks like a good enough quick fix to me.
> > > 
> > > 
> > > 
> > > Thanks Xiaocheng for the review comments.
> > > @Yosin/tkent can you please let me know your opinion about taking this
patch
> > for now?
> > 
> > Sorry for late response.
> > I thinks we should change |TextIterator| to handling this situation. I know
> > |TextIterator| is very complicated but now is time to make |TextIterator|
more
> > clean and work correctly.
> 
> 
> 
> 
> 
> @Yosin/Xiaocheng
> 
> Added new patchset4 where in tried changing TextIterator to handle this
situation. As of now even if the text inside button or img doesnot start with "
" if white space is collapsed before the replaced element one extra space is
inserted. To avoid this incorporated changes so as to check if the character at
current offset is space and only then emit an extra space.
> 
> Please let me know if this approach is fine?
> 
> 
> Trybot results on linux shows total 519 text failures. (All are related to
document element innerText white space difference, not valid failures of
testcases)
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
> 
> 
> Following 24 testcases passed unexpectedly.
> 
>
virtual/rootlayerscrolls/fast/scrolling/fractional-scroll-offset-fixed-position-non-composited.html
>
virtual/spv2/paint/paintlayer/non-self-painting-layer-overrides-visibility.html
> virtual/spv2/paint/invalidation/invalidate-when-receiving-paint-layer.html
>
virtual/spv2/paint/invalidation/repaint-subsequence-on-ancestor-clip-change.html
>
http/tests/security/contentTypeOptions/nosniff-script-without-content-type-blocked.html
> editing/pasteboard/4944770-2.html
> http/tests/inspector/service-workers/user-agent-override.html
> fast/css/absolute-inline-alignment.html
> http/tests/media/progress-events-generated-correctly.html
> fast/encoding/invalid-UTF-8.html
> fast/forms/month/month-appearance-l10n.html
> fast/inline/nested-text-descendants.html
> http/tests/xmlviewer/dumpAsText/svg.xml
> fast/text/international/bdi-dir-default-to-auto.html
> fast/text/ellipsis-stroked.html
> fast/text/emoticons.html
> fast/text/international/hindi-spacing.html
> fast/text/international/inline-plaintext-is-isolated.html
> fast/text/selection-multiple-runs.html
> ietestcenter/css3/multicolumn/column-width-applies-to-007.htm
> ietestcenter/css3/multicolumn/column-width-applies-to-009.htm
> ietestcenter/css3/multicolumn/column-width-applies-to-012.htm
> ietestcenter/css3/multicolumn/column-width-applies-to-015.htm
> fast/text/unicode-variation-selector.html

Sounds good! This patch produces better output than before.

Since, there are too many test failures, we may want to introduce new behavior
flag, I don't want to say so, but,...
like TextIteratorBehaviorEmitExtraSpaceDeprecated and change content_shell to
call TextIterator with it.
Or, do opposite to change findString() with flag.

[ramya.v, 2016-01-19 14:42:27]

Description was changed from

==========
Find In Page doesn't work properly when " " is searched

When whitespace spawns the shadow dom trees, range is
calculated as collapsed and further searches were also
ignored. Allowing to continue search if the search is
terminated before covering search range.

BUG=510335
==========

to

==========
Find In Page doesn't work properly when " " is searched

Extra space is emitted if whitespace is collapsed before
the replaced element irrespective of element's text 
starts with whitespace. Since this extra space is not 
visible collapsed range is returned and the search 
terminates. Added changes to emit extra space only in 
case of element starting with a whitespace.

BUG=510335
==========

[ramya.v, 2016-01-19 14:43:14]

On 2016/01/19 09:49:28, Yosi_UTC9 wrote:
> On 2016/01/19 at 04:31:46, ramya.v wrote:
> > On 2016/01/18 04:54:20, Yosi_UTC9 wrote:
> > > On 2016/01/18 at 04:19:44, ramya.v wrote:
> > > > On 2016/01/14 01:13:58, Xiaocheng wrote:
> > > > > On 2016/01/11 at 10:45:52, ramya.v wrote:
> > > > > > On 2016/01/08 08:07:32, Xiaocheng wrote:
> > > > > > > On 2016/01/07 at 09:31:44, yosin wrote:
> > > > > > > > +xiaochengh@, he is an expert of TextIterator.
> > > > > > > 
> > > > > > > I'm confused by the behavior of TextIterator.
> > > > > > > 
> > > > > > > I added some code at the beginning of findPlainTextAlgorithm,
which
> uses
> > > a
> > > > > > > TextIterator to emit every character in inputRange. Then I found
> that a
> > > > > > > seemingly extra space is emitted, which is deemed a match of " "
by
> > > > > > > findPlainTextAlgorithm. But this space is not visible at all! So a
> > > collapsed
> > > > > > > range is created and returned, which causes the bug.
> > > > > > > 
> > > > > > > I'm not sure why TextIterator emits this extra space. At least in
> our
> > > case,
> > > > > the
> > > > > > > emitting of this space is quite unreasonable. Is it for some other
> > > purposes
> > > > > in
> > > > > > > some other scenario? Or is it just a bug...
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > The code I added at the beginning of findPlainTextAlgorithm:
> > > > > > > 
> > > > > > >     WTFLogAlways("Invoking findPlainTextAlgorithm with the
following
> > > input
> > > > > > > Range");
> > > > > > >     inputRange.startPosition().showTreeForThis();
> > > > > > >     inputRange.endPosition().showTreeForThis();
> > > > > > >     {
> > > > > > >         WTFLogAlways("Dumping text in the range...");
> > > > > > >         TextIteratorBehaviorFlags behavior =
> > > iteratorFlagsForFindPlainText;
> > > > > > >         if (options & FindAPICall)
> > > > > > >             behavior |= TextIteratorForWindowFind;
> > > > > > >         CharacterIteratorAlgorithm<Strategy>
> dumpIterator(inputRange,
> > > > > behavior);
> > > > > > >         while (!dumpIterator.atEnd())
> > > > > > >         {
> > > > > > >             WTFLogAlways("%d", dumpIterator.characterAt(0));
> > > > > > >             dumpIterator.advance(1);
> > > > > > >         }
> > > > > > >         WTFLogAlways("Dump end");
> > > > > > >     }
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > When searching " " in Ex1 of #9, the above code prints the
> following.
> > > Note
> > > > > that
> > > > > > > two consecutive "32"s (i.e., spaces) are emitted!
> > > > > > > 
> > > > > > > Invoking findPlainTextAlgorithm with the following input Range
> > > > > > > *#document	0x5cbe7004010
> > > > > > > 	html	0x5cbe701c010
> > > > > > > 	HTML	0x5cbe7010120
> > > > > > > 		HEAD	0x5cbe7010098
> > > > > > > 		BODY	0x5cbe7010010
> > > > > > > 			SPAN	0x5cbe70101a8
> > > > > > > 				#text	0x5cbe7020010 "Some text "
> > > > > > > 			#text	0x5cbe7020080 " "
> > > > > > > 			INPUT	0x5cbe7030010
> > > > > > > 				#document-fragment	0x5cbe703c010
> > > > > > > 					#text	0x5cbe70200f0 "Button text"
> > > > > > > 			SPAN	0x5cbe7010230
> > > > > > > 				#text	0x5cbe7020160 "Some more text"
> > > > > > > 			#text	0x5cbe70201d0 "\n"
> > > > > > > beforeChildren, offset:0
> > > > > > > *#document	0x5cbe7004010
> > > > > > > 	html	0x5cbe701c010
> > > > > > > 	HTML	0x5cbe7010120
> > > > > > > 		HEAD	0x5cbe7010098
> > > > > > > 		BODY	0x5cbe7010010
> > > > > > > 			SPAN	0x5cbe70101a8
> > > > > > > 				#text	0x5cbe7020010 "Some text "
> > > > > > > 			#text	0x5cbe7020080 " "
> > > > > > > 			INPUT	0x5cbe7030010
> > > > > > > 				#document-fragment	0x5cbe703c010
> > > > > > > 					#text	0x5cbe70200f0 "Button text"
> > > > > > > 			SPAN	0x5cbe7010230
> > > > > > > 				#text	0x5cbe7020160 "Some more text"
> > > > > > > 			#text	0x5cbe70201d0 "\n"
> > > > > > > afterChildren, offset:0
> > > > > > > Dumping text in the range...
> > > > > > > 83
> > > > > > > 111
> > > > > > > 109
> > > > > > > 101
> > > > > > > 32
> > > > > > > 116
> > > > > > > 101
> > > > > > > 120
> > > > > > > 116
> > > > > > > 32
> > > > > > > 32
> > > > > > > 66
> > > > > > > 117
> > > > > > > 116
> > > > > > > 116
> > > > > > > 111
> > > > > > > 110
> > > > > > > 32
> > > > > > > 116
> > > > > > > 101
> > > > > > > 120
> > > > > > > 116
> > > > > > > 83
> > > > > > > 111
> > > > > > > 109
> > > > > > > 101
> > > > > > > 32
> > > > > > > 109
> > > > > > > 111
> > > > > > > 114
> > > > > > > 101
> > > > > > > 32
> > > > > > > 116
> > > > > > > 101
> > > > > > > 120
> > > > > > > 116
> > > > > > > Dump end
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > Ex1: <span>Some text </span> <img src="abc.png"/>
> > > > > > Ex2: <span>Some text </span> <input type="button" value=" Button
> text"/>
> > > > > > 
> > > > > > Extra space is emitted from the below line
> > > > > (TextIterator::handleReplacedElement). Not sure of the exact reason
why
> this
> > > > > part of code is added
> > > > > >
> > > > >
> > >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > > > 
> > > > > > If we comment above block Ex1 works fine. For Ex2: even if we
comment,
> > > extra
> > > > > space is emitted from below location (EWhitespace is calculated as
PRE)
> > > > > >
> > > > >
> > >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > > 
> > > > > Thanks for finding these two lines.
> > > > > 
> > > > > So it seems that the emitting of the extra space is an intended,
though
> > > quite
> > > > > counterintuitive, behavior.
> > > > > 
> > > > > Maybe we should figure out the purpose of these two lines later to see
> if
> > > there
> > > > > is any mistake.
> > > > > 
> > > > > For now, the approach of this path (having an extra check when
> > > |findPlainText|
> > > > > returns a collapsed range) looks like a good enough quick fix to me.
> > > > 
> > > > 
> > > > 
> > > > Thanks Xiaocheng for the review comments.
> > > > @Yosin/tkent can you please let me know your opinion about taking this
> patch
> > > for now?
> > > 
> > > Sorry for late response.
> > > I thinks we should change |TextIterator| to handling this situation. I
know
> > > |TextIterator| is very complicated but now is time to make |TextIterator|
> more
> > > clean and work correctly.
> > 
> > 
> > 
> > 
> > 
> > @Yosin/Xiaocheng
> > 
> > Added new patchset4 where in tried changing TextIterator to handle this
> situation. As of now even if the text inside button or img doesnot start with
"
> " if white space is collapsed before the replaced element one extra space is
> inserted. To avoid this incorporated changes so as to check if the character
at
> current offset is space and only then emit an extra space.
> > 
> > Please let me know if this approach is fine?
> > 
> > 
> > Trybot results on linux shows total 519 text failures. (All are related to
> document element innerText white space difference, not valid failures of
> testcases)
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
> > 
> > 
> > Following 24 testcases passed unexpectedly.
> > 
> >
>
virtual/rootlayerscrolls/fast/scrolling/fractional-scroll-offset-fixed-position-non-composited.html
> >
>
virtual/spv2/paint/paintlayer/non-self-painting-layer-overrides-visibility.html
> > virtual/spv2/paint/invalidation/invalidate-when-receiving-paint-layer.html
> >
>
virtual/spv2/paint/invalidation/repaint-subsequence-on-ancestor-clip-change.html
> >
>
http/tests/security/contentTypeOptions/nosniff-script-without-content-type-blocked.html
> > editing/pasteboard/4944770-2.html
> > http/tests/inspector/service-workers/user-agent-override.html
> > fast/css/absolute-inline-alignment.html
> > http/tests/media/progress-events-generated-correctly.html
> > fast/encoding/invalid-UTF-8.html
> > fast/forms/month/month-appearance-l10n.html
> > fast/inline/nested-text-descendants.html
> > http/tests/xmlviewer/dumpAsText/svg.xml
> > fast/text/international/bdi-dir-default-to-auto.html
> > fast/text/ellipsis-stroked.html
> > fast/text/emoticons.html
> > fast/text/international/hindi-spacing.html
> > fast/text/international/inline-plaintext-is-isolated.html
> > fast/text/selection-multiple-runs.html
> > ietestcenter/css3/multicolumn/column-width-applies-to-007.htm
> > ietestcenter/css3/multicolumn/column-width-applies-to-009.htm
> > ietestcenter/css3/multicolumn/column-width-applies-to-012.htm
> > ietestcenter/css3/multicolumn/column-width-applies-to-015.htm
> > fast/text/unicode-variation-selector.html
> 
> Sounds good! This patch produces better output than before.
> 
> Since, there are too many test failures, we may want to introduce new behavior
> flag, I don't want to say so, but,...
> like TextIteratorBehaviorEmitExtraSpaceDeprecated and change content_shell to
> call TextIterator with it.
> Or, do opposite to change findString() with flag.



Updated with flag. PTAL!

Thanks

[yosin_UTC9, 2016-01-20 01:40:23]

Please describe what this patch changes the code rather than analysis of the
bug. Analysis of the bug should be "background" or "analysis" section of path
description.

We are very close to finish this patch. Rest of things are just polish.

https://codereview.chromium.org/1483013003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right):

https://codereview.chromium.org/1483013003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:675: if
(m_lastTextNodeEndedWithCollapsedSpace && m_offset > 0 && str[m_offset-1] == '
') {
Please inserts spaces around '-'
nit: str[m_offset - 1]

https://codereview.chromium.org/1483013003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/iterators/SearchBuffer.cpp (right):

https://codereview.chromium.org/1483013003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/SearchBuffer.cpp:417: static
const TextIteratorBehaviorFlags iteratorFlagsForFindPlainText =
TextIteratorEntersTextControls | TextIteratorEntersOpenShadowRoots |
TextIteratorDoesNotBreakAtReplacedElement |
TextIteratorEmitExtraSpaceDeprecated;
Sorry for confusion, the flag name |TextIteratorEmitExtraSpaceDeprecated| means
current behavior and for content_shell.
We should name |TextIteratorCollapseTrailingSpace| or better one.

https://codereview.chromium.org/1483013003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right):

https://codereview.chromium.org/1483013003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:684: if
(m_lastTextNodeEndedWithCollapsedSpace && m_offset > 0 && str[m_offset-1] == '
') {
nit: need spaces around '-'

[ramya.v, 2016-01-20 03:36:56]

Description was changed from

==========
Find In Page doesn't work properly when " " is searched

Extra space is emitted if whitespace is collapsed before
the replaced element irrespective of element's text 
starts with whitespace. Since this extra space is not 
visible collapsed range is returned and the search 
terminates. Added changes to emit extra space only in 
case of element starting with a whitespace.

BUG=510335
==========

to

==========
Find In Page doesn't work properly when " " is searched

In case of replaced elements or elements with white-space
property 'PRE', extra space is emitted only when their text
starts with a white-space which is collapsed because of the
preceding white-space.

BUG=510335
==========

[ramya.v, 2016-01-20 03:42:56]

Updated as per review comments. PTAL!


Thanks.

https://codereview.chromium.org/1483013003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right):

https://codereview.chromium.org/1483013003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:675: if
(m_lastTextNodeEndedWithCollapsedSpace && m_offset > 0 && str[m_offset-1] == '
') {
On 2016/01/20 01:40:22, Yosi_UTC9 wrote:
> Please inserts spaces around '-'
> nit: str[m_offset - 1]

Done.

https://codereview.chromium.org/1483013003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/iterators/SearchBuffer.cpp (right):

https://codereview.chromium.org/1483013003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/SearchBuffer.cpp:417: static
const TextIteratorBehaviorFlags iteratorFlagsForFindPlainText =
TextIteratorEntersTextControls | TextIteratorEntersOpenShadowRoots |
TextIteratorDoesNotBreakAtReplacedElement |
TextIteratorEmitExtraSpaceDeprecated;
On 2016/01/20 01:40:22, Yosi_UTC9 wrote:
> Sorry for confusion, the flag name |TextIteratorEmitExtraSpaceDeprecated|
means
> current behavior and for content_shell.
> We should name |TextIteratorCollapseTrailingSpace| or better one.

Done.

https://codereview.chromium.org/1483013003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right):

https://codereview.chromium.org/1483013003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:684: if
(m_lastTextNodeEndedWithCollapsedSpace && m_offset > 0 && str[m_offset-1] == '
') {
On 2016/01/20 01:40:23, Yosi_UTC9 wrote:
> nit: need spaces around '-'

Done.

[yosin_UTC9, 2016-01-20 05:33:11]

https://codereview.chromium.org/1483013003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right):

https://codereview.chromium.org/1483013003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:463:
emitCharacter(spaceCharacter, textNode, 0, runStart, runStart);
Off-topic:
It seems we should rename |emitCharacter()| to |spliceBuffer()| or better, since
it does both inserting character and removing characters.
"splice" is inspired by |Array.prototype.splice()| in JavaScript.
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects...
Let's do in another patch.

https://codereview.chromium.org/1483013003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/TextFinderTest.cpp (right):

https://codereview.chromium.org/1483013003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/TextFinderTest.cpp:436:
TEST_F(TextFinderTest, WhitespaceMatches)
Can we put test in |TextIteratorTest| rather than for TextFinder?
Since, this patch changes |TextIterator| and |TextFinder| is very far from
|TextIterator| in our implementation.

[ramya.v, 2016-01-20 05:59:11]

Updated as per review comments. PTAL!

Thanks.

https://codereview.chromium.org/1483013003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp (right):

https://codereview.chromium.org/1483013003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/iterators/TextIterator.cpp:463:
emitCharacter(spaceCharacter, textNode, 0, runStart, runStart);
On 2016/01/20 05:33:11, Yosi_UTC9 wrote:
> Off-topic:
> It seems we should rename |emitCharacter()| to |spliceBuffer()| or better,
since
> it does both inserting character and removing characters.
> "splice" is inspired by |Array.prototype.splice()| in JavaScript.
>
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects...
> Let's do in another patch.

Sure will upload a separate patch for same.

https://codereview.chromium.org/1483013003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/TextFinderTest.cpp (right):

https://codereview.chromium.org/1483013003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/TextFinderTest.cpp:436:
TEST_F(TextFinderTest, WhitespaceMatches)
On 2016/01/20 05:33:11, Yosi_UTC9 wrote:
> Can we put test in |TextIteratorTest| rather than for TextFinder?
> Since, this patch changes |TextIterator| and |TextFinder| is very far from
> |TextIterator| in our implementation.

Done.

[yosin_UTC9, 2016-01-20 06:33:22]

lgtm


Thanks for fixing this issue and patient for long iteration!

[yosin_UTC9, 2016-01-20 06:33:25]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2016-01-20 06:34:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1483013003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1483013003/120001

[commit-bot: I haz the power, 2016-01-20 06:37:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-20 06:37:21]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[ramya.v, 2016-01-20 08:49:15]

The CQ bit was checked by ramya.v@samsung.com

[ramya.v, 2016-01-20 08:49:15]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/1483013003/#ps140001
(title: "Rebasing the patch")

[commit-bot: I haz the power, 2016-01-20 08:49:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1483013003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1483013003/140001

[commit-bot: I haz the power, 2016-01-20 10:39:23]

Description was changed from

==========
Find In Page doesn't work properly when " " is searched

In case of replaced elements or elements with white-space
property 'PRE', extra space is emitted only when their text
starts with a white-space which is collapsed because of the
preceding white-space.

BUG=510335
==========

to

==========
Find In Page doesn't work properly when " " is searched

In case of replaced elements or elements with white-space
property 'PRE', extra space is emitted only when their text
starts with a white-space which is collapsed because of the
preceding white-space.

BUG=510335
==========

[commit-bot: I haz the power, 2016-01-20 10:39:24]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-01-20 10:40:28]

Description was changed from

==========
Find In Page doesn't work properly when " " is searched

In case of replaced elements or elements with white-space
property 'PRE', extra space is emitted only when their text
starts with a white-space which is collapsed because of the
preceding white-space.

BUG=510335
==========

to

==========
Find In Page doesn't work properly when " " is searched

In case of replaced elements or elements with white-space
property 'PRE', extra space is emitted only when their text
starts with a white-space which is collapsed because of the
preceding white-space.

BUG=510335

Committed: https://crrev.com/6a04368503ae7e48422a851d39afc920ee66502d
Cr-Commit-Position: refs/heads/master@{#370362}
==========

[commit-bot: I haz the power, 2016-01-20 10:40:29]

Patchset 8 (id:??) landed as
https://crrev.com/6a04368503ae7e48422a851d39afc920ee66502d
Cr-Commit-Position: refs/heads/master@{#370362}