Bug 127284 – Refactoring inline_spelling_markers.html to use asynchronous spellchecking

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Grzegorz Czajkowski]

Use asynchronous text checking in:
 - inline_spelling_markers.html,
 - inline-spelling_markers-hidpi.html.

[Grzegorz Czajkowski]

Created attachment 221654 [details]
Patch

[Grzegorz Czajkowski]

This test and its hdipi brother were rewritten considerably. That's why I decided to submit them separately.

I'd appreciate your comments.

[Ryosuke Niwa]

Comment on attachment 221654 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        inline-spelling_markers-hidpi.html

Nit: should read inline-spelling-markers-hidpi.html

> LayoutTests/ChangeLog:19
> +        * editing/spelling/inline_spelling_markers-expected.txt: Added.
> +        Make cross platform text expectation by dumping spelling/grammar markers
> +        instead of whole tree which in terms of spellchecking says nothing.
> +
> +        * editing/spelling/inline_spelling_markers.html:
> +        Remove unnecessary new lines from div elements so position of markers can be given from 0.
> +        Activate text checking by adding a word separator so WebKit starts performing spell and grammar
> +        checking. Selection change does not invoke grammar checking unless we start editing the input
> +        element's content.

Please rename the test to inline-spelling-markers.html to match the naming convention.

> LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:63
> +function typeIncorrectPhrase(element)
> +{
> +    element.focus();
> +    document.execCommand("InsertText", false, incorrectPhrase);
> +}

This function seems to hide what we're really doing. It's better to put in runTextCheckingTestFor directly.

> LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:70
> +function performTextChecking()
> +{
> +    const wordSeparator = " ";
> +    // Add a word separator so both spelling and grammar markers will appear.
> +    document.execCommand("InsertText", false, wordSeparator);
> +}

Ditto.

> LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:76
> +function verifyMarkers()
> +{
> +    if (!window.internals)
> +        return done();
>  

We can't test this at all in browser!?

[Grzegorz Czajkowski]

Created attachment 224364 [details]
Took Ryosuke's comments into account

[Grzegorz Czajkowski]

Comment on attachment 221654 [details]
Patch

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

>> LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:76
>>  
> 
> We can't test this at all in browser!?

This function only verifies markers using shouldBecomeEqual*. All desired elements and their content with incorrect phrase have been already created, done() here just allows to move to the next test case. Opening it with any Browses gives a reasonable content.

[Build Bot]

Comment on attachment 224364 [details]
Took Ryosuke's comments into account

Attachment 224364 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6153928395718656

New failing tests:
editing/spelling/inline-spelling-markers.html

[Build Bot]

Created attachment 224373 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 224364 [details]
Took Ryosuke's comments into account

Attachment 224364 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6054065842683904

New failing tests:
editing/spelling/inline-spelling-markers.html

[Build Bot]

Created attachment 224388 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Grzegorz Czajkowski]

(In reply to comment #6)
> (From update of attachment 224364 [details])
> Attachment 224364 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.appspot.com/results/6153928395718656
> 
> New failing tests:
> editing/spelling/inline-spelling-markers.html

By chance, I removed 'debug(div.id)' from the test. That's why the result is different.

[Grzegorz Czajkowski]

Created attachment 224600 [details]
make Mac bot green

[Ryosuke Niwa]

Comment on attachment 224600 [details]
make Mac bot green

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

> LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:71
> +    shouldBecomeEqual('internals.hasSpellingMarker(8, 4)', 'true', function() { // Verifies 'adlj'.
> +        shouldBecomeEqual('internals.hasSpellingMarker(13, 6)', 'true', function() { // Verifies 'adaasj'.
> +            shouldBecomeEqual('internals.hasSpellingMarker(20, 5)', 'true', function() {  // Verifies 'sdklj'.
> +                verifyGrammarMarkers();

Instead of adding a comment in the code like this, include it in the eval itself so that the expected result doesn't look so cryptic.

> LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:79
> +        shouldBecomeEqual('internals.hasGrammarMarker(4, 3)', 'true', function() { // Verifies duplicated 'the'.
> +            shouldBecomeEqual('internals.hasGrammarMarker(33, 5)', 'true', function() { // Verifies duplicated 'there'.

Ditto.

> LayoutTests/editing/spelling/inline-spelling-markers.html:86
> +    // Take care of spelling markers first.
> +    shouldBecomeEqual('internals.hasSpellingMarker(8, 4)', 'true', function() { // Verifies 'adlj'.
> +        shouldBecomeEqual('internals.hasSpellingMarker(13, 6)', 'true', function() { // Verifies 'adaasj'.
> +            shouldBecomeEqual('internals.hasSpellingMarker(20, 5)', 'true', function() {  // Verifies 'sdklj'.
> +                verifyGrammarMarkers();
> +            });
> +         });
> +    });
> +
> +    function verifyGrammarMarkers()
> +    {
> +        shouldBecomeEqual('internals.hasGrammarMarker(4, 3)', 'true', function() { // Verifies duplicated 'the'.
> +            shouldBecomeEqual('internals.hasGrammarMarker(33, 5)', 'true', function() { // Verifies duplicated 'there'.
> +                debug("");
> +                done();
> +            });
> +        });
> +    }

Ditto.

[Grzegorz Czajkowski]

Created attachment 225125 [details]
apply Ryosuke's comments

[Grzegorz Czajkowski]

(In reply to comment #12)
> (From update of attachment 224600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224600&action=review
> 
> > LayoutTests/editing/spelling/inline-spelling-markers-hidpi.html:71
> > +    shouldBecomeEqual('internals.hasSpellingMarker(8, 4)', 'true', function() { // Verifies 'adlj'.
> > +        shouldBecomeEqual('internals.hasSpellingMarker(13, 6)', 'true', function() { // Verifies 'adaasj'.
> > +            shouldBecomeEqual('internals.hasSpellingMarker(20, 5)', 'true', function() {  // Verifies 'sdklj'.
> > +                verifyGrammarMarkers();
> 
> Instead of adding a comment in the code like this, include it in the eval itself so that the expected result doesn't look so cryptic.
> 

I moved those comments into debug() for inline-spelling-markers.html only, hidpi test still contains those comments in html file due to the output of messages produced by shouldBecome* covers whole visible view and as a result input elements are placed outside so that pixel tests do no catch them. 

Feel free to reject commit-queue if we should keep those messages in hidpi. One option that comes to mind is to scroll the page to desired position. I

[WebKit Commit Bot]

Comment on attachment 225125 [details]
apply Ryosuke's comments

Clearing flags on attachment: 225125

Committed r164643: <http://trac.webkit.org/changeset/164643>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.