1488953 - Make scrollport events use something other than WillPaintObserver, and remove WillPaintObservers.

Status: REOPENED

(Core :: Layout, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

There's no good reason for them now that scroll events don't use them anymore. See bug 1488871.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1488953 - Use script runner for scrollport events.

This is consistent with the scroll area events too, and allows us to
remove the WillPaintObserver stuff.

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 9007241 [details]
Bug 1488953 - Use script runner for scrollport events.

Mats Palmgren (:mats) has approved the revision.

Comment 3

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

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:emilio, could you have a look please?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yes, we're all aware of this.

Comment 5

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

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:emilio, could you have a look please?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

FFS

Comment 7

[Hiroyuki Ikezoe (:hiro)]

I will take over for landing the patch. The patch makes browser_tabCloseSpacer.js fail on Window.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a544c5f90b775c9c659ac102d51c7e36dca815f&selectedJob=249128288

Comment 8

[Hiroyuki Ikezoe (:hiro)]

There are two failure causes in browser_tabCloseSpacer.js. I can reproduce the failure in the try in comment 7 on local linux box with debug build, and also I found another failure case that is actually a timeout (see this TV for example). So the causes are;

1. we proceed the test during the last tab is still scrolled into view
   this makes the test frequently fail on debug builds on TV runs, I think this is a pre-existing issue in the test itself
2. sometimes scrolling the last tab into view stops on race conditions so that the close button on the last tab isn't visible at all
   this makes the test intermittently fail on opt builds on TV runs, to be honest I haven't dug in detail what the condition causes the situation, but you can see this issue on the current nightly to open a number of tabs by keeping pressing Ctrl+t, so I am pretty sure this is a pre-existing issue (or it might not be an issue at all)

To avoid 1), we can wait for a scrollend event there, to avoid 2) we can disable smooth scrolling for the scrollbox, browser_overflowScroll.js does the same thing.

A try with the above two changes looks good.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1488953 - Wait for a scrollend event to make sure the scrolling has finished before proceeding the test. r?bgrins

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1488953 - Disable smooth scroll on tabbar scrollbox to avoid stopping scrolling the last tab on race conditions. r?bgrins

Depends on D33614

Comment 11

[Hiroyuki Ikezoe (:hiro)]

browser_windowopen.js failed on MacOS opt builds on a try that I pushed the last week, and I tried to reproduce the failure locally on a macbook but can't reproduce so far. But now the test in question didn't fail on a try based on the latest m-c.

So I am going to land patches in this bug as it is. Hope these patches to stick to m-c.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a5cbbb9bc48
Wait for a scrollend event to make sure the scrolling has finished before proceeding the test. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/519acd8e145f
Disable smooth scroll on tabbar scrollbox to avoid stopping scrolling the last tab on race conditions. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/8646ea969443
Use a script runner for scrollport events. r=mats

Comment 13

[Andreea Pavel [:apavel]]

Backed out for failing bc at browser_windowopen.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&group_state=expanded&revision=8646ea96944350a9e1b88881087011c582b94326&selectedJob=250872027

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=250872027&repo=autoland&lineNumber=1950

Backout: https://hg.mozilla.org/integration/autoland/rev/f9a79922a9ad115ff5f39485f85b2ae40de44c08

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Sigh. I did actually miss the failure happened in my try in comment 11. The result told us that the failure happens only on MacOS 10.10 opt builds, doesn't happen on MacOS 10.14 opt builds. That's the reason why I can't reproduce the failure locally.

Anyways, I have to dive into the difference between them. :/

Comment 15

[Hiroyuki Ikezoe (:hiro)]

I was totally missing the case where the titlebar flicker which is actually the failure in browser_windowopen.js in question happens along with the urlbar flickers that I added as an exception in bug 1488871. So we need to handle the case both flickers happen at the same time.

Hey :mconley, I am sorry for bothering about browser_windowopen.js again. I am going to request you to review a patch handling the case.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Here is a try with the patch I mentioned in comment 15;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9652c3f01c1a8fe705eba4ae6257199869ebd207

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1488953 - Allows toolbar background paint flash along with the urlbar flickers on MacOSX in browser_windowopen.js. r?mconley

On MacOSX, especially on MacOS 10.10, the toolbar paint flush happens along
with the urlbar flickers.

Depends on D33615

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a638724408b
Wait for a scrollend event to make sure the scrolling has finished before proceeding the test. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/5555e12078d3
Disable smooth scroll on tabbar scrollbox to avoid stopping scrolling the last tab on race conditions. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/6759badeec0e
Allows toolbar background paint flash along with the urlbar flickers on MacOSX in browser_windowopen.js. r=mconley
https://hg.mozilla.org/integration/autoland/rev/6b9cfebe8076
Use a script runner for scrollport events. r=mats

Comment 19

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/2a638724408b
https://hg.mozilla.org/mozilla-central/rev/5555e12078d3
https://hg.mozilla.org/mozilla-central/rev/6759badeec0e
https://hg.mozilla.org/mozilla-central/rev/6b9cfebe8076

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/91c42888cf7f
Backed out 4 changesets for causing Bug 1559690. a=backout

Comment 21

[Emilio Cobos Álvarez (:emilio)]

The patches from bug 1559690 were pretty close, we need to figure those out before landing this, but I don't think I have the cycles to do that right now.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

I am going to land D33614 and D33615 to mitigate browser_tabCloseSpacer.js failure (bug 1549985).

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6e51e948711
Wait for a scrollend event to make sure the scrolling has finished before proceeding the test. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/381031318227
Disable smooth scroll on tabbar scrollbox to avoid stopping scrolling the last tab on race conditions. r=bgrins

Comment 24

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/f6e51e948711
https://hg.mozilla.org/mozilla-central/rev/381031318227

Comment 25

[Mats Palmgren (inactive)]

Given that these events are non-standard and not on track to become a standard I think we should unship them.
Chrome used to support a similar but incompatible overflowchanged event, but they removed it in ~2014 according to:
https://groups.google.com/a/chromium.org/g/blink-dev/c/1RCzsyEyNU8/m/5vNpxrhRbUIJ
(I checked a recent Chrome and it's still gone.)
That indicates to me that this functionality likely isn't needed by web developers.

Thoughts?

Comment 26

[Emilio Cobos Álvarez (:emilio)]

So, yeah, agreed in general, but we have also internal consumers like the url bar that we'd have to rewrite (in fact that's what made these patches so hard to land). It'd be good to add some use counters before just removing the code though.

Comment 27

[Mats Palmgren (inactive)]

All they seem to use it for is basically a poly-fill for text-overflow:fade though:
https://searchfox.org/mozilla-central/search?q=%5Btextoverflow&path=
so we should probably prioritize implementing that...

I filed bug 1688022...

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/756680c06b2b
Use script runner for scrollport events. r=mats

Comment 29

[Cristian Tuns]

Backed out for causing bc failures

• Backout link

• Push with failures

• Failure Log

• Failure line: TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_tabstrip_overflow_underflow.js | unexpected reflow at MozArrowScrollbox/overflowObserver<@chrome://global/content/elements/arrowscrollbox.js hit 1 times

• Failure Log

• Failure line: TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_tabdetach.js | unexpected reflow at MozArrowScrollbox/overflowObserver<@chrome://global/content/elements/arrowscrollbox.js hit 1 times