Summary: | Refactoring inline_spelling_markers.html to use asynchronous spellchecking | ||
---|---|---|---|
Product: | WebKit | Reporter: | Grzegorz Czajkowski <g.czajkowski> |
Component: | HTML Editing | Assignee: | Grzegorz Czajkowski <g.czajkowski> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | buildbot, bunhere, cdumez, commit-queue, g.czajkowski, gyuyoung.kim, rakuco, rniwa, sergio |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Bug Depends on: | |||
Bug Blocks: | 125689 | ||
Attachments: |
Description
Grzegorz Czajkowski
2014-01-20 04:43:46 PST
Created attachment 221654 [details]
Patch
This test and its hdipi brother were rewritten considerably. That's why I decided to submit them separately. I'd appreciate your comments. 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!? Created attachment 224364 [details]
Took Ryosuke's comments into account
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. 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 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
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 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
(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. Created attachment 224600 [details]
make Mac bot green
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. Created attachment 225125 [details]
apply Ryosuke's comments
(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 Comment on attachment 225125 [details] apply Ryosuke's comments Clearing flags on attachment: 225125 Committed r164643: <http://trac.webkit.org/changeset/164643> All reviewed patches have been landed. Closing bug. |