1637776 - Write mochitests for Fenix hit testing regressions (bug 1637113, bug 1638655, bug 1638458, bug 1638441, bug 1637135, bug 1643212, bug 1638594)

Status: RESOLVED FIXED

(Core :: Panning and Zooming, task)

Description

[Botond Ballo [:botond]]

Timothy suggested in a review comment that it would be useful to add a test covering the scenario that bug 1637113 regressed to avoid regresseing it in the future.

Will add a test in this bug.

Comment 1

[Timothy Nikkel (:tnikkel)]

Testing the specific fix of bug 1637113 is good, having a general test in the tree that checks that "scroll down, pinch zoom, tap" actually hits the expected element under the tap would be great.

Comment 2

[Botond Ballo [:botond]]

I have a mochitest for bug 1637113 that's working locally. It required making some improvements to coordinatesRelativeToScreen() to handle iframes better, and these changes are causing some other mochitest failures which I need to investigate.

Comment 3

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #2)

I have a mochitest for bug 1637113 that's working locally. It required making some improvements to coordinatesRelativeToScreen() to handle iframes better, and these changes are causing some other mochitest failures which I need to investigate.

Finally have a green Try push. The patches need a bit of cleanup as my understanding of the issue has evolved over the course of investigating the failures.

Comment 4

[Botond Ballo [:botond]]

Attachment: Bug 1637776 - Fix coordinatesRelativeToScreen() to work better for targets inside an iframe. r=tnikkel

Comment 5

[Botond Ballo [:botond]]

Attachment: Bug 1637776 - Add a mochitest for bug 1637113. r=tnikkel

Depends on D83018

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/461dea66f9f1
Fix coordinatesRelativeToScreen() to work better for targets inside an iframe. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/92b59d89a40e
Add a mochitest for bug 1637113. r=tnikkel

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 1637776 - Fix trailing whitespace in test

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/58000ee18336
Fix trailing whitespace in test r=rmaries

Comment 9

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/461dea66f9f1
https://hg.mozilla.org/mozilla-central/rev/92b59d89a40e
https://hg.mozilla.org/mozilla-central/rev/58000ee18336

Comment 10

[Botond Ballo [:botond]]

The test that was landed for bug 1637113 provides test coverage for bug 1638655 as well: this Try push shows the test failing on Android if the patch for bug 1638655 is backed out.

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1637776 - Fix non-unified bustage in nsDisplayList.cpp. r=tnikkel

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1637776 - Add a mochitest for bug 1638458. r=tnikkel

Depends on D84882

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1637776 - Rewrite the test for bug 1637113 in the async/await style. r=tnikkel

The test is also revised to fail immediately rather than via a timeout
if the expected event is fired but targets the wrong element.

Comment 14

[Botond Ballo [:botond]]

Attachment: Bug 1637776 - Add a mochitest for bug 1638441. r=tnikkel

Depends on D84891

Comment 15

[Botond Ballo [:botond]]

Attachment: Bug 1637776 - Add a mochitest for bug 1637135. r=tnikkel

Depends on D84892

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2705663333f
Fix non-unified bustage in nsDisplayList.cpp. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/75650b9112fa
Add a mochitest for bug 1638458. r=tnikkel

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e2705663333f
https://hg.mozilla.org/mozilla-central/rev/75650b9112fa

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/729ccac1e832
Rewrite the test for bug 1637113 in the async/await style. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/0b41d1d9ed6c
Add a mochitest for bug 1638441. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/2d3d30eb308b
Add a mochitest for bug 1637135. r=tnikkel

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/729ccac1e832
https://hg.mozilla.org/mozilla-central/rev/0b41d1d9ed6c
https://hg.mozilla.org/mozilla-central/rev/2d3d30eb308b

Comment 20

[Botond Ballo [:botond]]

We now have test coverage for 5 of the 7 bugs mentioned in the bug title.

The remaining two are:

• Bug 1643212, which is a dynamic toolbar issue. As discussed here, we don't really have the test infrastructure in place to test issues like this.
• Bug 1638594, which only affects content that's loaded in the parent-process, which again I'm not quite sure how to exercise in a mochitest environment.

I'm inclined to say that having test coverage for 5 of these 7 issues is good enough, and move on to other things.

Comment 21

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #20)

• Bug 1638594, which only affects content that's loaded in the parent-process, which again I'm not quite sure how to exercise in a mochitest environment.

mochitest-chrome or browser-chrome test?

I'm inclined to say that having test coverage for 5 of these 7 issues is good enough, and move on to other things.

Sounds reasonable. Thanks for working on this!

Comment 22

[Kartikaya Gupta (email:[e-mail])]

This doesn't really block desktop zooming on nightly, so let's disconnect it.

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:botond, maybe it's time to close this bug?

Comment 24

[Botond Ballo [:botond]]

I was going to write a browser chrome test for bug 1638594 as suggested in comment 21 but realistically I don't think I'm going to get to it.