Open Bug 1334944 Opened 5 years ago Updated 1 year ago

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

Categories

(Core :: Layout, defect, P5)

defect

Tracking

()

People

(Reporter: MatsPalmgren_bugz, Unassigned)

References

Details

(Whiteboard: [lang=html] )

Attachments

(1 file)

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 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.
Hmm, tests are failing on Linux with this...
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.)
: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?
Flags: needinfo?(wkocher)
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.
Flags: needinfo?(wkocher)
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 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).
Attachment #8841795 - Flags: review?(mats)
Priority: -- → P3
Hello,
I'm an absolute beginner and would like to work on this bug.

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

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

Pease can someone assign this project for me?

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!

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

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

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?

Flags: needinfo?(mats)

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

Flags: needinfo?(mats)

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.

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.

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

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

I want to solve this bug

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)

Flags: needinfo?(mats)

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.

Severity: normal → S4
Flags: needinfo?(mats)
Priority: P3 → P5

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...

Keywords: good-first-bug
You need to log in before you can comment on or make changes to this bug.