560888 - accessible/tests/mochitest/attributes/test_text.html is failing for Fedora unit tests

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Armen [:armenzg]]

From bug 554934#c13:
(In reply to comment #13)
> (In reply to comment #5)
> > Created an attachment (id=434879) [details] [details]
> > mochitest-a11y -
> > chrome://mochikit/content/a11y/accessible/attributes/test_text.html
> 
> Ok, here's a clue as to what's happening with reftest.  If you go look at this
> test:
> http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/attributes/test_text.html?force=1
> you will see that this thing really doesn't do a good job of setting a default
> known state before tickling a bunch of font properties that all will be
> affected by the system's default settings.  
> 
> Digging some ancient (and possibly incorrect) information out of my head, I
> recall there is a default CSS used for each OS in the browser and in lieu of
> any user CSS direction those values are used.  They are used to set things like
> default font, font size, font family etc.  So, if those default settings aren't
> what they are on centos then this test could do something unexpected.  This one
> is really a fault of the test because it doesn't appear (in my quick scan) to
> set all the defaults to the known states that it is expecting.  If we were to
> change those default CSS values then we'd also break this test, I think.
> 
> And that might be related to what's going on with those reftest font-face
> tests.

1595 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/attributes/test_text.html | Attribute 'font-size' has wrong value. Getting default text attributes for area8 - got "8pt", expected "10pt"
1604 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/attributes/test_text.html | Attribute 'font-size' has wrong value for area8 at offset 0 - got "8pt", expected "10pt"
1614 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/attributes/test_text.html | Attribute 'font-size' has wrong value for area8 at offset 11 - got "8pt", expected "10pt"
1624 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/attributes/test_text.html | Attribute 'font-size' has wrong value for area8 at offset 17 - got "8pt", expected "10pt"
1634 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/attributes/test_text.html | Attribute 'font-size' has wrong value for area8 at offset 18 - got "8pt", expected "10pt"

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1271864813.1271866635.10140.gz

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

OK sounds like our tests need to check the desktop settings somehow. Right now they are hard coded, and obviously fragile. Do we have precedent for checking such things from mochitests?

Comment 2

[(no longer active)]

I think mostly we use systemfont as default.  CCing some people who might have an idea what needs to be done here.

Comment 3

[Karl Tomlinson (:karlt)]

Would it be testing what is intended if the font-size were set on the input?
Or perhaps getComputedStyle could be used on the input?

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #3)
> Would it be testing what is intended if the font-size were set on the input?

You would think. Doesn't seem to be work for input. I'm not sure why.

> Or perhaps getComputedStyle could be used on the input?

We have to expose the font size in pt units so in C++ we do some unpleasant contortions. getComputedStyle seems to return px and we're not sure how to do the same contortions in the test (in js). For this reason I think our testing of font size in test_text.html is inherently fragile.

I can make the test more forgiving for inputs.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch - skip font size check for input

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 2 (sniff fedora)

Armen can you give this a whirl?

Comment 7

[Armen [:armenzg]]

Comment on attachment 441230 [details] [diff] [review]
patch 2 (sniff fedora)

The sniffing doesn't seem to be working and we still get the same test failures.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Ah right, that sniffer probably only works on Fedora packaged builds. Hmm.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #8)
> Ah right, that sniffer probably only works on Fedora packaged builds. Hmm.

Nope it should work.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Heh, nope again. Just confirmed my comment 8. I'll have to go with something like the earlier approach.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 441223 [details] [diff] [review]
patch - skip font size check for input

Marco, this allows us to bypass testing font size for input elements. We can't sniff for linux distro in our builds.

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 441223 [details] [diff] [review]
patch - skip font size check for input

I'd still like a better way to test this in the long run, esp when it comes to rich-editing in Google Docs where the font size is important. So we should add another bug to rework this for textboxes or the like. But r=me for this for now so we can get Fedora going.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #12)
> (From update of attachment 441223 [details] [diff] [review])
> I'd still like a better way to test this in the long run, esp when it comes to
> rich-editing in Google Docs where the font size is important. So we should add
> another bug to rework this for textboxes or the like. But r=me for this for now
> so we can get Fedora going.

That's a good point. I'll rework it slightly.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: bypass linux only.

Marco, might as well test on win and mac eh?
Armen, can you give this a whirl?

Comment 15

[Marco Zehe (:MarcoZ)]

Comment on attachment 441814 [details] [diff] [review]
bypass linux only.

r=me for this approach.

Comment 16

[Armen [:armenzg]]

Comment on attachment 441814 [details] [diff] [review]
bypass linux only.

Gentleman, this works for me :)

14245 INFO Passed: 13995
14246 INFO Failed: 0
14247 INFO Todo:   7
14248 INFO SimpleTest FINISHED

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

Landed on central: http://hg.mozilla.org/mozilla-central/rev/68d7be8a8375

Thanks all - I think we ended up with the right fix here.