Add a reftest-spellchecker API

RESOLVED FIXED in mozilla24

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: adw)

Tracking

Trunk
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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?
Flags: needinfo?(dbaron)
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.
Flags: needinfo?(dbaron)
(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?
Flags: needinfo?(dbaron)
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.)
Flags: needinfo?(dbaron)
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.
Attachment #744273 - Flags: feedback?(dbaron)
(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.
Attachment #744273 - Flags: feedback?(dbaron)
Posted patch reftestWaitForSpellCheck (obsolete) — Splinter Review
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].
Attachment #746695 - Flags: review?(dbaron)
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.
Attachment #746695 - Flags: review?(dbaron)
(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.
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.
Flags: needinfo?(dbaron)
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(dbaron)
OS: Mac OS X → All
Hardware: x86 → All
Please see comment 9, too.  This uses class="spell-checked" instead of the reftestWaitForSpellCheck attribute I mentioned there.
Attachment #746695 - Attachment is obsolete: true
Attachment #756772 - Flags: review?(dbaron)
Oh, I didn't realize that *[contenteditable="true"] doesn't match <div contenteditable>.  Makes sense.
Attachment #756772 - Attachment is obsolete: true
Attachment #756772 - Flags: review?(dbaron)
Attachment #756889 - Flags: review?(dbaron)
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
Attachment #756889 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/b5138d0d6889
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.