Bug 72281 – Fix find on web pages with -webkit-user-select: none for Chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Martin Kosiba]

[Chromium] fix searching for text on web pages that prevent text selection

[Martin Kosiba]

Created attachment 114962 [details]
Patch

[Martin Kosiba]

For an example of a webpage that actually exhibits this problem go to m.cnn.com.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/src/WebFrameImpl.cpp:1514:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 2 files


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

[Ryosuke Niwa]

So I think we should fix https://bugs.webkit.org/show_bug.cgi?id=40361 instead of fixing just Chromium port.

[Martin Kosiba]

Fixing 40361 would require making changes to all of the ports - essentially moving all of them onto the rangeOfString function.
I will try and factor out the common 'find and update markers' code and place that in a method on Editor.

[Martin Kosiba]

Created attachment 115627 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 115627 [details]
Patch

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

The overall approach looks good but needs more polishing.

> Source/WebCore/ChangeLog:6
> +        Adding findStringUsingMarkers to Editor. This new method uses markers
> +        to indicate the active find result.

These descriptions should appear below "Reviewed by"

> Source/WebCore/ChangeLog:8
> +        https://bugs.webkit.org/show_bug.cgi?id=72281

This line should appear immediately below the bug title.

> Source/WebKit/chromium/ChangeLog:14
> +        [Chromium] fix searching for text on web pages that prevent text selection
> +
> +        This will make it possible to search for text that has
> +        selection disabled (via the webkit-user-select attribute).
> +
> +        WebFrameImpl::find will use findStringUsingMarkers, which is similar
> +        to findString except that it manipulates markers directly rather
> +        than passing the find result in the active selection.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=72281
> +
> +        Reviewed by NOBODY (OOPS!).

Ditto about the format.

> Source/WebCore/editing/Editor.cpp:2776
> +        if (!nextMatch
> +            || (nextMatch->collapsed(ec)
> +                && !nextMatch->startContainer()->isInShadowTree()))

It seems like this fits in one line.

> Source/WebCore/editing/Editor.cpp:2783
> +            scrollRectToVisible(nextMatch->boundingBox(),

We don't wrap lines after -> line this.

> Source/WebCore/editing/Editor.cpp:2785
> +                                ScrollAlignment::alignCenterIfNeeded,
> +                                ScrollAlignment::alignCenterIfNeeded);

Wrong indentation these lines should be indented by exactly 8 spaces (4 spaces from where nextMatch is).

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1471
> +            (options.matchCase ? 0 : CaseInsensitive) |
> +            (wrapWithinFrame ? WrapAround : 0) |
> +            (!options.findNext ? StartInSelection : 0);

Wrong indentation.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1474
> +    m_activeMatch = frame()->editor()->findStringUsingMarkers(searchText,
> +                                                              m_activeMatch.get(),
> +                                                              findOptions);

Ditto.

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1818
> +    const bool findResult = frame->find(0,
> +                                    cppVariantToWebString(arguments[0]),
> +                                    findOptions,
> +                                    wrapAround,
> +                                    0);

Wrong indentation.

> LayoutTests/ChangeLog:9
> +        [Chromium] fix searching for text on web pages that prevent text selection
> +
> +        Adding a layout test to verify that find works correctly on pages
> +        that prevent text selection.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=72281
> +

Wrong format. See above.

> LayoutTests/editing/text-iterator/findString-selection-disabled-expected.txt:1
> +Searching for âoâ in âLorem ipsum dolor sit ametâ and with selection enabled:

Please use normal sentence instead of some latin.

> LayoutTests/editing/text-iterator/findString-selection-disabled-expected.txt:8
> +Searching for âipsumâ in âLorem ipsum dolor sit ametâ and with selection enabled:
> +PASS: Got true as expected.
> +
> +Searching for âxâ in âLorem ipsum dolor sit ametâ and with selection enabled:
> +PASS: Got false as expected.

It's hard to tell what's been tested by just looking at this result.

> LayoutTests/editing/text-iterator/findString-selection-disabled.html:12
> +    function log(message)
> +    {
> +        document.getElementById("console").appendChild(document.createTextNode(message + "\n"));
> +    }

We don't normally indent scripts inside script element. Please outdent them.

> LayoutTests/editing/text-iterator/findString-selection-disabled.html:18
> +        log("Searching for \u2018" + target + "\u2019 in \u2018" + text 
> +            + "\u2019 and with " + selectionStatus);

These Unicode characters don't show up properly in the patch so you should avoid using them.

> LayoutTests/editing/text-iterator/findString-selection-disabled.html:30
> +        if (!window.internals || !window.layoutTestController) {
> +            var found = expectedResult;
> +        } else {
> +            var found = layoutTestController.findString(target, []);
> +        }

I don't see any point in checking window.internals since we're not accessing internals in this test.

Also, instead of declaring found here, you can just do:
assertTrue("layoutTestController.findString('" + target + "', [])", expectedResult)

This makes the test output much more descriptive.

> LayoutTests/editing/text-iterator/findString-selection-disabled.html:35
> +        if (found != expectedResult)
> +            log("FAIL: Got " + found + " but expected " + expectedResult + ".");
> +        else
> +            log("PASS: Got " + found + " as expected.");

It seems like this test should be a script test.

[Ryosuke Niwa]

Add a bunch of people who have been working on markers & find feature.

[Ryosuke Niwa]

Removing [chromium] since this patch adds new method to Editor.

[Martin Kosiba]

Created attachment 115832 [details]
Patch

[Darin Adler]

Comment on attachment 115832 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:2785
> +PassRefPtr<Range> Editor::findStringUsingMarkers(const String& target, Range* previousMatch, FindOptions options)
> +{
> +    ExceptionCode ec = 0;
> +
> +    if (previousMatch)
> +        m_frame->document()->markers()->setMarkersActive(previousMatch, false);
> +
> +    RefPtr<Range> nextMatch;
> +    do {
> +        nextMatch = rangeOfString(target, previousMatch, options);
> +        if (!nextMatch || (nextMatch->collapsed(ec) && !nextMatch->startContainer()->isInShadowTree()))
> +            return 0;
> +    } while (!insideVisibleArea(nextMatch.get()));
> +
> +    // Scroll the result into view.
> +    nextMatch->firstNode()->renderer()->enclosingLayer()->scrollRectToVisible(
> +        nextMatch->boundingBox(),
> +        ScrollAlignment::alignCenterIfNeeded,
> +        ScrollAlignment::alignCenterIfNeeded);
> +    m_frame->document()->markers()->setMarkersActive(nextMatch.get(), true);
> +
> +    return nextMatch.release();
> +}

Why does this code need to be inside Editor?

The caller is going to have to manipulate the markers directly anyway if it wants to, say, “leave find mode”, so encapsulating the creation and destruction of these markers here inside this function does not provide a useful abstraction.

[Martin Kosiba]

Darin,
    My first patch only modified only Chromium WebFrameImpl, but Ryosuke suggested that I move some of this into WebCore so that we encapsulate the 'scroll into view' details from the port.
    I imagine Chrome and Safari have some duplication of code in the find implementations (btw. is the Safari find implementation in WebKit? I couldn't find it anywhere so I assume no). Ideally I'd like to move this in a direction that allows more sharing.
    I can remove the code that touches the markers and rename the method to findStringAndScrollIntoView, would that be better?

[Martin Kosiba]

Created attachment 117220 [details]
Patch

[Martin Kosiba]

Created attachment 117625 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 117625 [details]
Patch

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

New code has way too much comment! The preferred way of documenting code in WebKit is to name functions, variables, etc... to be self-descriptive; e.g. by introducing inline helper functions. It appears that you might want to restructure your code so that you wouldn't need so much comments. Also you might want to consider replacing some comments by assertions to check the conditions.

> Source/WebCore/ChangeLog:4
> +

This blank line should be removed.

> Source/WebCore/ChangeLog:10
> +        Adding findStringUsingMarkers to Editor. This new method uses markers
> +        to indicate the active find result.

This comment is no longer accurate. Please update.

> Source/WebKit/chromium/ChangeLog:4
> +

Ditto.

> Source/WebCore/editing/Editor.cpp:2781
> +        if (!nextMatch || (nextMatch->collapsed(ec) && !nextMatch->startContainer()->isInShadowTree()))

Why is range being inside the shadow DOM or not important? That seems like a strange condition to check here.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1519
> +        // Active match is changing.

Again, this comment seems useless. Please remove.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1537
> +    const FindOptions findOptions = (options.forward ? 0 : Backwards) |
> +        (options.matchCase ? 0 : CaseInsensitive) |
> +        (wrapWithinFrame ? WrapAround : 0) |
> +        (!options.findNext ? StartInSelection : 0);

WebKit style is to put at the beginning of next line.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1541
> +    m_activeMatch = frame()->editor()->findStringAndScrollToVisible(
> +        searchText,
> +        m_activeMatch.get(),
> +        findOptions);

Seems strange to put these arguments in separate lines. Since WebKit doesn't have 80-column restriction, you should put them all in one line.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1545
> +        // Erase all previous tickmarks and highlighting.
> +        invalidateArea(InvalidateAll);

The comment and the code does two different things. I suggest you remove the comment.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1551
> +    // Store which frame was active. This will come in handy later when we
> +    // change the active match ordinal below.

This is self-evident from the code. No need to add a comment.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1553
> +    // Set this frame as the active frame (the one with the active highlight).

Again, this comment repeats the code. Please remove.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1562
> +        // This is either a Find operation or a Find-next from a new start point
> +        // due to a selection, so we set the flag to ask the scoping effort
> +        // to find the active rect for us so we can update the ordinal (n of m).

What are you referring to by the ordinal (n of m) ?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1574
> +            // We are still the active frame, so increment (or decrement) the

This is so self-evident from the code.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1575
> +            // |m_activeMatchIndex|, wrapping if needed (on single frame pages).

The problem is with this m_activeMatchIndex. It's not obvious that this index is per frame. So you probably want to rename this variable to be self-evident.

[Martin Kosiba]

Ryosuke,
    The code with comments isn't new new code - I simply moved a block of code out of an else block, it popped up as new because I changed indent on those lines. I do agree that most of the comments were pointless so I removed most of them.

>> Source/WebCore/editing/Editor.cpp:2781
>> +        if (!nextMatch || (nextMatch->collapsed(ec) && !nextMatch->>startContainer()->isInShadowTree()))

>Why is range being inside the shadow DOM or not important? That seems like a 
>strange condition to check here.

It seems to be important for some reason: Editor::countMatchesForText will stop searching if this condition is met, so will the Chromium scoping code. After looking at this a bit more it turns out that rangeOfString already covers the cases I was checking so I can simplify that piece of code more.

Thanks!
    Martin

[Martin Kosiba]

Created attachment 118066 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebKit/chromium/src/WebFrameImpl.cpp:1526:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 21 files


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

[Ryosuke Niwa]

Comment on attachment 118066 [details]
Patch

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

r- due to various nits.

> Source/WebCore/editing/Editor.cpp:2785
> +    nextMatch->firstNode()->renderer()->enclosingLayer()->scrollRectToVisible(
> +        nextMatch->boundingBox(),
> +        ScrollAlignment::alignCenterIfNeeded,
> +        ScrollAlignment::alignCenterIfNeeded);

We don't line break per argument. WebKit style is to do:
    nextMatch->firstNode()->renderer()->enclosingLayer()->scrollRectToVisible(nextMatch->boundingBox(),
        ScrollAlignment::alignCenterIfNeeded, ScrollAlignment::alignCenterIfNeeded);

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1526
>> +    } else {
>> +        setMarkerActive(m_activeMatch.get(), false);
>> +    }
> 
> One line control clauses should not use braces.  [whitespace/braces] [4]

As style bot pointed out, you shouldn't have { } around a single-line statement.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1542
> +            | (options.matchCase ? 0 : CaseInsensitive)
> +            | (wrapWithinFrame ? WrapAround : 0)
> +            | (!options.findNext ? StartInSelection : 0);

These lines are indented too much. | should appear exactly 4 spaces to the right of const.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1565
> +                m_activeMatchIndexInActiveMatchFrame = 0;

I don't think "InActiveMatchFrame" is helpful. It's probably better to say m_activeMatchIndexInCurrentFrame and rename m_activeMatchFrame to m_currentActiveMatchFrame.

[Martin Kosiba]

Created attachment 118562 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 118562 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1568
> +            options.forward ? ++m_activeMatchIndexInCurrentFrame : --m_activeMatchIndexInCurrentFrame;

We require a line per statement. Please use a regular if statement.

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1986
> +    const bool findResult = frame->find(0,
> +        cppVariantToWebString(arguments[0]),
> +        findOptions,
> +        wrapAround,
> +        0);

Wrong style again. Please fix it before landing the patch.

[Martin Kosiba]

Created attachment 119006 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 118562 [details]
Patch

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

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1534
> +        executeCommand(WebString::fromUTF8("Unselect"));

Wait, I must have missed in my previews reviews. Why are we not calling frame()->selection()->clear() ?

[Martin Kosiba]

> > Source/WebKit/chromium/src/WebFrameImpl.cpp:1534
> > +        executeCommand(WebString::fromUTF8("Unselect"));

> Wait, I must have missed in my previews reviews. Why are we not calling 
> frame()->selection()->clear() ?

Good catch! The only reason I used that is because this is what the original implementation used.

[Martin Kosiba]

Created attachment 119202 [details]
Patch

[Martin Kosiba]

Comment on attachment 119202 [details]
Patch

Thanks for the review Ryosuke!

[WebKit Review Bot]

Comment on attachment 119202 [details]
Patch

Rejecting attachment 119202 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 119202 [details]
Patch

Rejecting attachment 119202 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
kipped.rej
patching file LayoutTests/platform/qt-5.0/Skipped
Hunk #1 succeeded at 3681 (offset -3302 lines).
patching file LayoutTests/platform/qt-mac/Skipped
patching file LayoutTests/platform/qt-wk2/Skipped
Hunk #1 FAILED at 1869.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt-wk2/Skipped.rej
patching file LayoutTests/platform/wk2/Skipped

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Ryosuke Niwa', u'--for..." exit_code: 1

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

[Martin Kosiba]

Created attachment 119436 [details]
Patch

[Martin Kosiba]

Created attachment 119437 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119437 [details]
Patch

Clearing flags on attachment: 119437

Committed r102955: <http://trac.webkit.org/changeset/102955>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.