763461 - Mozmill tests for cut-off elements should not report failure for scrollable direction

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Our Mozmill l10n tests have been shown that there are some elements which are cut-off due to the localization. The results of the test you can find here:

http://mozmill-ci.blargon7.com/#/l10n/report/ee284dfa9d664754a65676abbc060a2d

Details: 	

    Node is cut off by 10 px at the bottom: listitem#en-us. Parent node: listboxbody[rows=6][xbl:inherits=rows,size,minheight]

Comment 1

[Dwayne Bailey]

Hi Henrik,

How would I find where I should correct this?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

CC'ing Axel who was helping with this feature. I agree that this information is not that helpful when looking at the plain text results. The screenshots should be more helpful but those are not available yet. I'm working at least at the next step here: https://github.com/mozilla/mozmill-ci/issues/108

I could try to find those screenshots for this locale.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: screenshot

That's also the same listbox for the ve locale.

Comment 4

[Dwayne Bailey]

The screenshots do really help.

Not important for this locale as it was a cleanup on my side that triggered a build.  But thought I'd anticipate this for the other teams that are live and active.

Some OCR on that could make for some searchable strings to get us to the right page.  I'm dreaming ;)

Comment 5

[Axel Hecht [:Pike]]

Increasing <!ENTITY window.width "30em"> in tn/browser/chrome/browser/preferences/languages.dtd will help.

Sadly, I don't know where the height of that dialog is coming from.

Comment 6

[Dwayne Bailey]

I actually think this one is a false positive.  The list is just longer for tn then normal I think.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks like that we should exclude list/tree items in that case?

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So I agree. I can reproduce it with an en-US locale by just adding more languages to the accepted languages list.

So it looks like that we should check first if the direction is scrollable, if not we can perform those tests. Otherwise we should return early. Axel, does that sound ok to you?

Comment 9

[Andrei Eftimie]

Attachment: Patch v1

I've removed richlistbox items with a vertical orient, which targets only the failed test.
We get a green testrun on OSX and Windows:
http://mozmill-crowd.blargon7.com/#/l10n/report/c8094a822ef568b588c5eddaca81bdf4
http://mozmill-crowd.blargon7.com/#/l10n/report/c8094a822ef568b588c5eddaca85e9a4

I've separately attached an unskip patch because we still fail on Linux (seems to be some other cause):
http://mozmill-crowd.blargon7.com/#/l10n/report/c8094a822ef568b588c5eddaca815833

Apply both patches to be able to run the test.
The unskip patch should not land yet, as we should probably resolve the Linux problems first.

Comment 10

[Andrei Eftimie]

Attachment: unskip

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 740220 [details] [diff] [review]
Patch v1

Review of attachment 740220 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/localization.js
@@ +99,5 @@
>    var parent = childBox.parentBox;
>  
> +  // skip for vertical richlistbox items
> +  if (childBox.element.nodeName === "richlistbox" &&
> +      childBox.element.orient === "vertical") {

There are various kinds of elements which have scrollbars. So we cannot exclude richlistbox only. Also elements can have horizontal scrollbars. So this approach will not work.

scrollHeight or something similar might help:
https://developer.mozilla.org/en-US/docs/DOM/element.scrollHeight

Comment 12

[Andrei Eftimie]

Attachment: patch v3

I've fixed the issue by using offsetWidth and offsetHeight instead of width and height.

The l10n crop test passes on all platforms:
OSX: http://mozmill-crowd.blargon7.com/#/l10n/report/5aa1ca5e7015e3740d269dc9472931a6
Linux: http://mozmill-crowd.blargon7.com/#/l10n/report/5aa1ca5e7015e3740d269dc947298fcd
Windows: http://mozmill-crowd.blargon7.com/#/l10n/report/5aa1ca5e7015e3740d269dc94729c965

Comment 13

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 774533 [details] [diff] [review]
patch v3

Review of attachment 774533 [details] [diff] [review]:
-----------------------------------------------------------------

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/14d962b72438 (default)

We will need backport on this in order to have the l10n tests fixed.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I wouldn't backport this for all branches but aurora only. Reason is that we only run l10n tests on that branch. But if it turns out that the patch applies cleanly, I'm open to even backport it further. Lets get this into aurora, so that we can re-enable the test.

Btw. this patch should not have been re-enabled the test. I'm fairly sure we have a bug open which disabled the test. Also the manifest file hasn't been taken care of.

Comment 15

[Andrei Eftimie]

Attachment: reskip followup

reskip followup patch

As discussed we'll unskip the test in bug 741301

Comment 16

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 775666 [details] [diff] [review]
reskip followup

Review of attachment 775666 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, my mistake here. Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/e20b71e03c74 (default)

Comment 17

[Andrei Eftimie]

Attachment: patch v3.1

Applies cleanly an all branches. 
(contains fix, test still skipped)

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so lets get this backported to aurora immediately, and the test enabled. Andrei, I assume you have tested the patch with various locales also with a RTL one?

Comment 19

[Andrei Eftimie]

Yep, works fine on .ar which is a RTL language

Comment 20

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 775672 [details] [diff] [review]
patch v3.1

Review of attachment 775672 [details] [diff] [review]:
-----------------------------------------------------------------

Great. Andrei tested all branches and it applied cleanly so here we have:

http://hg.mozilla.org/qa/mozmill-tests/rev/da282053b9bf (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/81927d0b853b (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/e482db7a858a (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/6d27262378b1 (esr17)

Lets get the test enabled now.