1334944 - Make all layout/reftests/bugs/315920-* tests use MozReftestInvalidate instead of onload

Status: NEW

(Core :: Layout, defect, P5)

Description

[Mats Palmgren (inactive)]

layout/reftests/bugs/315920-17.html failed when it became the first test to run
in a reftest chunk, so I fixed it in bug 1330962 like so:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b222ec9a5d90805a8bb0e8bcdfbc3a34d42bbbc0

I see that the other 315920-* tests are similar, so this is an accident
waiting to happen once one of the other tests becomes the first test.
We should fix all of them in a similar way.

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: Bug 1334944 - Make all layout/reftests/bugs/315920-* tests use MozReftestInvalidate instead of onload

Review commit: https://reviewboard.mozilla.org/r/115896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115896/

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8841795 [details]
Bug 1334944 - Make all layout/reftests/bugs/315920-* tests use MozReftestInvalidate instead of onload

https://reviewboard.mozilla.org/r/115896/#review117284

::: layout/reftests/bugs/315920-22.html:23
(Diff revision 1)
>      </div>
> +    <script>
> +    function doTest() {
> +      var elem = document.getElementById("one");
> +      elem.setAttribute("disabled", "disabled");
> +      document.getElementById("two").removeAttribute("disabled")

I was unsure if I should leave the trailing semicolon missing when moving this around.

::: layout/reftests/bugs/315920-23.html:24
(Diff revision 1)
>    </body>
> +    <script>
> +    function doTest() {
> +      var elem = document.getElementById("one");
> +      elem.setAttribute("disabled", "disabled");
> +      document.getElementById("two").removeAttribute("disabled")

Same for this missing semicolon.

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Hmm, tests are failing on Linux with this...

Comment 4

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

Do all of these tests show at least one checked checkbox/radio when the load (so that the image load happens)?  Otherwise waiting for the MozReftestInvalidate doesn't seem like it would wait for an image load triggered *by* the dynamic changes.  (But also see bug 1342240 comment 20.)

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

:kwierso, thanks for writing a patch for this- is there a chance you could try comment 20 from bug 1342240 out?  Do you need any help?

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I actually don't have a whole lot of time to work on this, if someone else can take it from here, that'd be great.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

as a note, I just added fuzzy-if for the common failures in bug 1342240, we can remove those along with a fix here :)

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 8841795 [details]
Bug 1334944 - Make all layout/reftests/bugs/315920-* tests use MozReftestInvalidate instead of onload

I think this is unnecessary now that we've backed out the code that made
the checkmark a background image from all branches (bug 1357169 and bug 1352406).

Comment 9

[Devika Sugathan]

Hello,
I'm an absolute beginner and would like to work on this bug.

Comment 10

[Paarmita Bhargava]

Hey,
If this issue is not assigned to anybody, I would like to work on it.

Comment 11

[Manvi]

Hi,
I would like to work on it. Please, assign it to me if nobody is working on it.

Comment 12

[cassyblaise]

Pease can someone assign this project for me?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

To be clear, people can submit patches without being marked as assignees. Generally we mark the assignee when the first patch is submitted :)

You're very welcome to work on this!

Comment 14

[Ayus Das]

Can anyone please give me the github link of the project so that I can start contributing.

Comment 15

[Helen]

Hello, I'm new to Mozilla and I'd like to help with this task.

Comment 16

[zuoqiwchris]

Hi, I have continued the work from :kwierso and added some missing html class "reftest-wait" document. Now out of 57 related tests, 55 passes. However, 315920-29a and 315920-29b appear to be "UNEXPECTED-FAIL". I tried to convert the given base64 string to png image however generated html looks identical to the ref html. Could someone provide more insights on this?

Comment 17

[Mats Palmgren (inactive)]

That sounds promising, thanks for working on this! If you don't see any difference it's probably some minor anti-aliasing difference which we usually solve by adding a fuzzy annotation in the manifest file (reftest.list). Search for "fuzzy" in https://searchfox.org/mozilla-central/source/layout/tools/reftest/README.txt

When you submit the tests to the Try server it will tell you the parameters you need. If you attach the patch I can submit it for you in case you don't have access yet. https://wiki.mozilla.org/ReleaseEngineering/TryServer

Comment 18

[zuoqiwchris]

Thanks Mats, these two cases finally passed with fuzzy(0-8,0-2). I wonder if the parameters of the fuzzy are appropriate? They are the smallest pairs to work on my machine but I wonder if I need to leave a bit more space for potential fluctuation? And also I will look into the Try server; it may take me some time since I'm pretty new to these tools.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Also, you can paste the log in https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml, and it would show you the minor differences.

Comment 20

[Saumya]

hello!
I am new to contributing to mozilla, can I do some help?

Comment 21

[Kanav]

Hello!
Is this issue still open to work on? If yes, please provide a github link for me to start working on.

Comment 22

[sushant.co19]

I want to solve this bug

Comment 23

[Mark Banner (:standard8)]

Mats, there's a few people interested in this bug, but I think it isn't entirely clear what needs doing. Comment 8 implies it isn't needed, but comment 16 and 17 imply it is wanted.

Please could do a brief summary for anyone that wants to pick it up? (or close it if appropriate)

Comment 24

[Mats Palmgren (inactive)]

I don't know to what extent our form controls make use of images (in the UA sheet) these days. If they do, or plan to do, then we should make these changes, otherwise it's unnecessary.

The changes in the attached patch looks fine to me. The remaining part is to make sure it pass on Try, with a fuzz annotation for anti-aliasing differences if needed, as noted in comment 18 / 19.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

I tend to think this is not so much of a good first bug, and it's not totally clear whether this needs doing at all. Even with onload the reftest harness should make sure all the painting is done properly...