863430 - Add a reftest-spellchecker API

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[(no longer active)]

Over in bug 856270, Drew is making the spell checker more async.  The challenge is that means that the spellcheck won't get finished right away, and that causes our spellchecking reftests to fail.

One approach that we can pursue is to port those tests over to test_reftests_with_caret.html, but I don't really like that idea since that framework has been too flaky in the past, and these tests are extremely important in that they're pretty much the only thing which prevent us from completely breaking the spell checker by accident.

The other approach that I thought of was to add a reftest-spellchecker API, which would work similar to reftest-print in how the reftests use it.  You basically set class="reftest-spellchecker" on your root element, and the framework would detect it, and then listen for an internal observer notification to know when the spell checker is done initializing its dictionary etc. and then you would clear the class value, take your screenshot and move on.

David, do you think this is a good idea?

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I guess I'm having trouble seeing what would make it less flaky if it's in the harness than if it's being driven by a mochitest.  I've certainly had successful uses of reftest-in-mochitest, e.g., test_visited_reftests.html, and I'd prefer not to add features to the harness if possible.  But if there's a good reason it's more likely to be stable in the harness than outside of it I'd be open to it.

Comment 2

[(no longer active)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #1)
> I guess I'm having trouble seeing what would make it less flaky if it's in
> the harness than if it's being driven by a mochitest.  I've certainly had
> successful uses of reftest-in-mochitest, e.g., test_visited_reftests.html,
> and I'd prefer not to add features to the harness if possible.  But if
> there's a good reason it's more likely to be stable in the harness than
> outside of it I'd be open to it.

Maybe there's no "good reason", but see <https://bugzilla.mozilla.org/buglist.cgi?quicksearch=sum%3Atest_reftests_with_caret&list_id=6437064>.  There are also tons of tests there which are disabled on some platforms but not others.  I suppose we could spend the time to fix that mini-testsuite but I don't think that's necessarily superior than having a reftest-spellchecker API.  Do you agree?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

But what's the characteristic that makes them less flaky as a result of being in reftest.{xul,js} rather than test_reftests_with_caret.html?  (We haven't had that problem with the visited reftests, or with the font inflation reftests before I switched those to use the harness since the only reason they weren't in the reftest harness was to change prefs.)

Comment 4

[Drew Willcoxon :adw]

Attachment: example changes

These are my changes to the harness, changes to a couple of tests, and a helper file used by the harness (and other tests not shown here) to illustrate what we're talking about.

There are a lot of reftests -- 64 -- that are affected by spell checking, even if their primary purpose is not testing spell checking.  Those are just the ones I've found so far because they either happened to fail or had "spellcheck" in their names.  Most are in editor/reftests, but a couple are in layout/reftests/text-overflow.  I would guess that there are more that should be updated but just haven't failed yet on tryserver due to lucky timing.

I can't speak to the flakiness you guys are discussing, but potentially placing every reftest that uses a textarea, div[contenteditable], or input[spellcheck] under control of a special spell check mochitest driver doesn't seem like a great idea, since such tests are potentially anywhere in the tree and may not primarily be testing spell check.

Comment 5

[(no longer active)]

(In reply to comment #3)
> But what's the characteristic that makes them less flaky as a result of being
> in reftest.{xul,js} rather than test_reftests_with_caret.html?  (We haven't had
> that problem with the visited reftests, or with the font inflation reftests
> before I switched those to use the harness since the only reason they weren't
> in the reftest harness was to change prefs.)

I'm not sure where this flakiness is coming from, but I have no reason to believe that it's a fundamental problem.  It's just that so far we have been unsuccessful to figure it out, and I don't want to prevent Drew from making progress here until we do.

Comment 6

[Drew Willcoxon :adw]

Attachment: reftestWaitForSpellCheck

David, this is the same as the previous "example changes" patch, but it only contains changes to reftest-content.js and is ready for review.  It looks for a reftestWaitForSpellCheck attribute on the contentRootElement.  The value of that attribute is a query selector that selects an editable element.  The driver waits for spell check to finish on that element and then finishes the test.

onSpellCheck is defined in the example changes patch, but also in bug 856270 attachment 746687 [details] [diff] [review].

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 746695 [details] [diff] [review]
reftestWaitForSpellCheck

I'm working on a better patch.  It automatically waits for spell check on editable elements, so you don't have to update each reftest that has one, and it better integrates into the state machine used in reftest-content.js rather than being more its own thing.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Drew Willcoxon :adw from comment #7)
> It automatically waits for spell check on
> editable elements, so you don't have to update each reftest that has one,

I might actually prefer the manual annotation to putting more magic into the harness.  Though I have mixed feelings; it depends on how magical it is, I suppose.  (Does it involve walking through the DOM?  Might that actually mess with test behavior in some way?)

> and it better integrates into the state machine used in reftest-content.js
> rather than being more its own thing.

That sounds good.  Though I admit I don't really even know that code particularly well anymore.



To be clear:  the reason I'm hesitant to add features to the harness is that I'd like as many of the reftests that people write as possible to be tests that we can contribute to standards test suites and share with other implementors.  (Though I haven't been as good about encouraging people to actually do the sharing as I should be.)  This means that I want people to be somewhat bothered by depending on features that are specific to our test harness, and I don't want people to add that sort of dependency accidentally when it's not necessary.

That said, I guess I'm ok with having the feature in the harness.

Comment 9

[Drew Willcoxon :adw]

Thanks, David.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #8)
> Though I have mixed feelings; it depends on how magical it is, I
> suppose.  (Does it involve walking through the DOM?  Might that actually
> mess with test behavior in some way?)

It uses querySelectorAll to find elements that may trigger spell check:

>     querySelector = 'textarea:not([spellcheck="false"]),' +
>                     'input[spellcheck="true"],' +
>                     '*[contenteditable="true"]';

I don't know enough about layout, etc. to know whether that could potentially alter test behavior.

That may not catch everything, though.  For example, some tests use script to set contenteditable on elements, triggering spell checks that aren't known to the harness at load time.  (Further, tests could potentially do this at arbitrary points in time, meaning that the harness should wait for spell check not on load but at the point in time when the test triggers spell check.  I haven't come across any tests that need to be accommodated like that, though.)

To allow for those cases, the harness also recognizes a reftestWaitForSpellCheck attribute on the documentElement.  Its value is a query selector, and the referenced elements will be added to the list of elements for which the harness will wait for spell check.  This part is like the patch I already posted.

> To be clear:  the reason I'm hesitant to add features to the harness is that
> I'd like as many of the reftests that people write as possible to be tests
> that we can contribute to standards test suites and share with other
> implementors.  (Though I haven't been as good about encouraging people to
> actually do the sharing as I should be.)  This means that I want people to
> be somewhat bothered by depending on features that are specific to our test
> harness, and I don't want people to add that sort of dependency accidentally
> when it's not necessary.

I understand, and that's a good reason not to like reftestWaitForSpellCheck.  It means reftests have to be modified for our spell check implementation.  I don't really like it, but I can't think of a way to work with the implementation without having to modify some tests in some way.  Maybe instead of reftestWaitForSpellCheck on the document element, we could add class="will-spell-check" or class="will-become-editable" to elements that the test will modify in script?  At least we're not using a made-up attribute then.

On the other hand, tests that can rely on the wait-for-spell-check behavior built into the harness don't need to be modified at all, so they could hypothetically run as-is in someone else's browser using its own harness.  If their spell check doesn't work like ours, then their harness wouldn't wait like ours does.

Comment 10

[Drew Willcoxon :adw]

Attachment: wait for spell check automatically + class="spell-checked"

Please see comment 9, too.  This uses class="spell-checked" instead of the reftestWaitForSpellCheck attribute I mentioned there.

Comment 11

[Drew Willcoxon :adw]

Attachment: wait for spell check automatically + class="spell-checked"

Oh, I didn't realize that *[contenteditable="true"] doesn't match <div contenteditable>.  Makes sense.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 756889 [details] [diff] [review]
wait for spell check automatically + class="spell-checked"

+    var querySelector = '*[class~="spell-checked"],' +
+                        'textarea:not([spellcheck="false"]),' +
+                        'input[spellcheck]:not([spellcheck="false"]),' +
+                        '*[contenteditable]:not([spellcheck="false"])';

The fourth line here needs to have s/spellcheck/contenteditable/.

But technically the third line here is also wrong, since a spellcheck
value other than "" or "true" behaves as though the attribute was not
present.  So really it should just be
  'input[spellcheck]:-moz-any([spellcheck=""],[spellcheck="true"]),' +
given that
http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#spelling-and-grammar-checking
says:
  In addition, there is a third state, the default state, which is the
  missing value default (and the invalid value default).

And the same for the fourth line, since
http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#contenteditable
says:
  In addition, there is a third state, the inherit state, which is the
  missing value default (and the invalid value default).
so the fourth line should be:
  '*[contenteditable]:-moz-any([contenteditable=""],[contenteditable="true"]),' +

>+function WaitForTestEnd(contentRootElement, inPrintMode, spellCheckedElements=[]) {

Why the default value here?  (I've also never seen that syntax
before.)  Just change the other caller to pass an empty array.

>+        for (editable of spellCheckedElements) {

This should have var or let (probably let).

>+        CU.import("resource://gre/modules/AsyncSpellCheckTestHelper.jsm");

What are the implications of doing this important within a function?  Would
it make more sense to do it just once?



r=dbaron with those things fixed

Comment 13

[Drew Willcoxon :adw]

Fixed review comments on landing:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5138d0d6889

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b5138d0d6889