Addressed review comments

lint fix

rebased and fixed a complie error with C++20?

rebased and fixed a complie error with C++20?

rebased and fixed a complie error with C++20?

rebased and fixed a complie error with C++20?

rebased and fixed a complie error with C++20?

rebased and fixed a complie error with C++20?

rebased and fixed a complie error with C++20?

@ohall Botond has been a member of gecko-viewers so that we don't need to explicitly set one of the person in the mobile team.

In D227882#9472136, @dlrobertson wrote:

I started writing the web-platform tests, and then I realized this would require something like bug 1848958 for pan gestures in order for a wpt to work for us. I think our choices for the test that verifies that the order of events and the behavior for iframes is:

1. write a mochitest
2. write a wpt test that we will fail, but chrome passes

1 would be a regression test for us, while 2 would be helpful for spec purposes... I suppose we could also do both.

In D273330#9458275, @hiro wrote:

In D273330#9458261, @tnikkel wrote:

One last thing you could try for a test, try a wpt crashtest. I've had luck before where a wpt crashtest worked to reproduce the bug when a reftest or mochitest did not.

Thanks for the info! I will try it.

In D273330#9458261, @tnikkel wrote:

One last thing you could try for a test, try a wpt crashtest. I've had luck before where a wpt crashtest worked to reproduce the bug when a reftest or mochitest did not.

testing-exception-other: Any automated test didn't work for the assertion in question. See https://bugzilla.mozilla.org/show_bug.cgi?id=1995803#c10 and https://bugzilla.mozilla.org/show_bug.cgi?id=1995803#c14 .

Add a comment about bug 2000473

Thanks for the commit message. It totally makes sense.

Fixed non-unified build error

Align the comment for NotifyLayersUpdated close to the function declaration

clang-format

Thanks for the tests! The second case is actually what I wanted to see.

Dan, I'd expect the test is written in a way that pan gesture scroll distance is predictable on all platforms, e.g. EventUtils.synthesizeWheel.

I am sorry, I was somehow misunderstanding this change. This change makes more permissive to create an active scroll container.

There are a bunch of use cases of layout.css.scroll-behavior.spring-constant. https://searchfox.org/firefox-main/search?q=layout.css.scroll-behavior.spring-constant&path=&case=false&regexp=false

testing-exception-elsewhere: I'd suppose that this change doesn't have any impacts on user visible behavior since last time I checked there's no usage of the GeckoView functions on github.com repos. This change may have some impacts on tests using nsIDOMWindowUtils.scrollToVisual, so I would say our existing tests have covered up this change code path.

(I'd hope the incoming test cases have some chained anchor cases.)

What about overscroll-behavior? Given that there's a visibility:hidden scroll container, then there's a child visibility:visible scroll container and then, without setting the parent id to the visibility:hidden container, it properly allows handing off to the parent? Vise versa, if the visibility:hidden container has overscroll-behavior: none, the scrolling in the child container isn't handed off to the grand parent?

With this patch I finally can understand some of the stuffs in bug 1988030. As I understand it for anchor positioning stuff we will never get null aLimitAncestor. And after circled back to D271804, I realized the commit message clearly mentions it. I thought I read though the commit message first before reading the code change, probably I couldn't get it at that momenent. ;)

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!

Overall looks good to me other than the call of AllocateActiveScrolledRoot in ActivateDisplayportOnAsyncScrollableAncestors.

In D271804#9405713, @tnikkel wrote:

In D271804#9405697, @hiro wrote:

This change itself looks reasonable to me. Though I don't quite understand how we use this function, specifically why we return nullptr when we hit aLimitAncestor.

If you want to look at where we use this it's in https://phabricator.services.mozilla.com/D271806 A summary: let X be the anchored contant, let CB be the containing block of X, let A be the anchor. We know that the anchor A will be in CB (the containing block of X) (because the acceptable anchor algorithm requires that so that anchor is laid out first). X would normally get assigned the ASR of CB, but instead we are now assigning it the ASR of A gets. Since A is in CB we know that the ASR of CB is an ancestor of the ASR of A. So we need to walk the ASR ancestor chain from the ASR of A to the ASR of CB, and stop there.

aLimitAncestor will never be a scroll frame?

Is it a problem if it is? I think it might never be a scroll frame, since it's the containing block it would be the scrolled frame rather than the scroll frame.

So now GetNearestScrollContainerFrame should not accept both of ASYNC_SCROLLABLE and ONLY_ACTIVATED, right? We should add an assertion in the function if either of the flag is set, the other one is not set.

This change itself looks reasonable to me. Though I don't quite understand how we use this function, specifically why we return nullptr when we hit aLimitAncestor. aLimitAncestor will never be a scroll frame?

I have the same question of what I did in https://phabricator.services.mozilla.com/D271800#9405681 . If D271800 got r+ I think this one will be able to automatically stamp r+.

I think this change should be reviewed by layout-anchor-positioning-reviewers. One big thing I don't quite understand is how anchor positioning stuff avoids infinite loop with this change.

(I had to read the commit message of the patch for bug 1995752 and had to understand to stamp r+ on this)

testing-exception-other: There may be automated tests hitting this condition such as a menu list longer than the browser window height, but I am not sure.

On my local environment the test has never failed.

In D271384#9404735, @ajakobi wrote:

The output of the test page has one flaw left: The distance between the bottom of the green box and the bottom of the page (both of which are on screen by the way) is 25px according to the reftest analyzer. My expectation would be 50px due to the dynamic toolbar max height pref being set to 50. I see the same outcome when removing the reftest-resolution option from the html tag.

Fixed the commit message, inserted '!'=0
'#'=0
'$'=4006
'*'=( )
-=5679Xhis
0=/usr/bin/zsh
'?'=141
@=( )
ARGC=0
CDPATH=''
COLORTERM=truecolor
COLUMNS=126
CPUTYPE=x86_64
DBUS_SESSION_BUS_ADDRESS='unix:path=/run/user/1000/bus'
DEFAULT=$'%{\C-[[1;m%}'
DESKTOP_AUTOSTART_ID=10d1b2e844ef14820a176263736947713000000031700016
DESKTOP_SESSION=ubuntu
DISPLAY=:0
EDITOR=nvim
EGID=1000
EUID=1000
FIGNORE=''
FPATH=/usr/local/share/zsh/site-functions:/usr/share/zsh/vendor-functions:/usr/share/zsh/vendor-completions:/usr/share/zsh/functions/Calendar:/usr/share/zsh/functions/Chpwd:/usr/share/zsh/functions/Completion:/usr/share/zsh/functions/Completion/AIX:/usr/share/zsh/functions/Completion/BSD:/usr/share/zsh/functions/Completion/Base:/usr/share/zsh/functions/Completion/Cygwin:/usr/share/zsh/functions/Completion/Darwin:/usr/share/zsh/functions/Completion/Debian:/usr/share/zsh/functions/Completion/Linux:/usr/share/zsh/functions/Completion/Mandriva:/usr/share/zsh/functions/Completion/Redhat:/usr/share/zsh/functions/Completion/Solaris:/usr/share/zsh/functions/Completion/Unix:/usr/share/zsh/functions/Completion/X:/usr/share/zsh/functions/Completion/Zsh:/usr/share/zsh/functions/Completion/openSUSE:/usr/share/zsh/functions/Exceptions:/usr/share/zsh/functions/MIME:/usr/share/zsh/functions/Math:/usr/share/zsh/functions/Misc:/usr/share/zsh/functions/Newuser:/usr/share/zsh/functions/Prompts:/usr/share/zsh/functions/TCP:/usr/share/zsh/functions/VCS_Info:/usr/share/zsh/functions/VCS_Info/Backends:/usr/share/zsh/functions/Zftp:/usr/share/zsh/functions/Zle
FUNCNEST=500
GDMSESSION=ubuntu
GID=1000
GNOME_DESKTOP_SESSION_ID=this-is-deprecated
GNOME_SETUP_DISPLAY=:1
GNOME_SHELL_SESSION_MODE=ubuntu
GNOME_TERMINAL_SCREEN=/org/gnome/Terminal/screen/ce84af73_0331_4971_86a0_d72820e43b32
GNOME_TERMINAL_SERVICE=:1.89
GREEN=$'%{\C-[[1;32m%}'
GSM_SKIP_SSH_AGENT_WORKAROUND=true
GTK_MODULES=gail:atk-bridge
HISTCHARS='!^#'
HISTCMD=104848
HISTFILE=/home/hiro/.zsh-history
HISTSIZE=100000
HOME=/home/hiro
HOST=hiro-B650I-AORUS-ULTRA
IFS=$' \t\n\C-@'
KEYBOARD_HACK=''
KEYTIMEOUT=40
LANG=ja_JP.UTF-8
LINENO=9
LINES=50
LISTMAX=100
LOGNAME=hiro
MACHTYPE=x86_64
MAILCHECK=60
MAILPATH=''
MANPATH=''
MEMORY_PRESSURE_WATCH='/sys/fs/cgroup/user.slice/user-1000.slice/[email protected]/app.slice/app-gnome\x2dsession\x2dmanager.slice/[email protected]/memory.pressure'
MEMORY_PRESSURE_WRITE='c29tZSAyMDAwMDAgMjAwMDAwMAA='
MODULE_PATH=/usr/lib/x86_64-linux-gnu/zsh/5.9
MOZ_QUIET=1
NULLCMD=cat
NVM_DIR=/home/hiro/.nvm
OLDPWD=/home/hiro/central
OPTARG=args
OPTIND=1
OSTYPE=linux-gnu
PATH=/home/hiro/.pyenv/bin:/home/hiro/.pyenv/shims:/home/hiro/.mozbuild/arcanist/bin:/home/hiro/.cargo/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/snap/bin:/home/hiro/.mozbuild/fzf/bin:/home/hiro/bin:/home/hiro/.mozbuild/android-sdk-linux/platform-tools:/home/hiro/.local/bin:/home/hiro/depot_tools:/home/hiro/venv/bin:/home/hiro/.mozbuild/git-cinnabar
PPID=3759
PROMPT='%(!.#.%%) '
PROMPT2='%_> '
PROMPT3='?# '
PROMPT4='+%N:%i> '
PS1='%(!.#.%%) '
PS2='%_> '
PS3='?# '
PS4='+%N:%i> '
PSVAR=''
PWD=/home/hiro/central
PYENV_ROOT=/home/hiro/.pyenv
QT_ACCESSIBILITY=1
QT_IM_MODULE=ibus
RANDOM=18746
READNULLCMD=/usr/bin/pager
RPROMPT=$'%{\C-[[1;32m%}[%~]%{\C-[[1;m%}'
SAVEHIST=100000
SECONDS=647
SESSION_MANAGER=local/hiro-B650I-AORUS-ULTRA:@/tmp/.ICE-unix/3170,unix/hiro-B650I-AORUS-ULTRA:/tmp/.ICE-unix/3170
SHELL=/usr/bin/zsh
SHLVL=1
SPROMPT='zsh: correct '\''%R'\'' to '\''%r'\'' [nyae]? '
SSH_AUTH_SOCK=/run/user/1000/keyring/ssh
SYSTEMD_EXEC_PID=3170
TERM=xterm-256color
TIMEFMT='%J %U user %S system %P cpu %*E total'
TMPPREFIX=/tmp/zsh
TRY_BLOCK_ERROR=-1
TRY_BLOCK_INTERRUPT=-1
TTY=/dev/pts/3
TTYIDLE=2
UID=1000
USER=hiro
USERNAME=hiro
VALGRIND_OPTIONS='--smc-check=all-non-file --vex-iropt-register-updates=allregs-at-mem-access --show-mismatched-frees=no --read-inline-info=yes'
VENDOR=ubuntu
VTE_VERSION=7600
WATCH
WAYLAND_DISPLAY=wayland-0
WORDCHARS='*?_-.[]~=/&;!#$%^(){}<>'
WORKON_HOME=/home/hiro/.virtualenvs
XAUTHORITY=/run/user/1000/.mutter-Xwaylandauth.M2J9E3
XDG_CURRENT_DESKTOP=ubuntu:GNOME
XDG_DATA_DIRS=/usr/local/share/:/usr/share/:/var/lib/snapd/desktop
XDG_MENU_PREFIX=gnome-
XDG_RUNTIME_DIR=/run/user/1000
XDG_SESSION_CLASS=user
XDG_SESSION_DESKTOP=ubuntu
XDG_SESSION_TYPE=wayland
XMODIFIERS='@IM=ibus'
ZLE_LINE_ABORTED=''
ZSH_ARGZERO=/usr/bin/zsh
ZSH_EVAL_CONTEXT=toplevel:cmdsubst
ZSH_NAME=zsh
ZSH_PATCHLEVEL=ubuntu/5.9-6ubuntu2
ZSH_SUBSHELL=1
ZSH_VERSION=5.9
_=set
_comp_assocs=( '' )
_comp_dumpfile=/home/hiro/.zcompdump
_comp_options
_comp_setup
_compautos
_comps
_lastcomp
_patcomps
_postpatcomps
_services
aliases
argv=( )
builtins
cdpath=( )
commands
comppostfuncs=( )
compprefuncs=( )
debian_missing_features=( )
dirstack
dis_aliases
dis_builtins
dis_functions
dis_functions_source
dis_galiases
dis_patchars
dis_reswords
dis_saliases
fignore=( )
fpath=( /usr/local/share/zsh/site-functions /usr/share/zsh/vendor-functions /usr/share/zsh/vendor-completions /usr/share/zsh/functions/Calendar /usr/share/zsh/functions/Chpwd /usr/share/zsh/functions/Completion /usr/share/zsh/functions/Completion/AIX /usr/share/zsh/functions/Completion/BSD /usr/share/zsh/functions/Completion/Base /usr/share/zsh/functions/Completion/Cygwin /usr/share/zsh/functions/Completion/Darwin /usr/share/zsh/functions/Completion/Debian /usr/share/zsh/functions/Completion/Linux /usr/share/zsh/functions/Completion/Mandriva /usr/share/zsh/functions/Completion/Redhat /usr/share/zsh/functions/Completion/Solaris /usr/share/zsh/functions/Completion/Unix /usr/share/zsh/functions/Completion/X /usr/share/zsh/functions/Completion/Zsh /usr/share/zsh/functions/Completion/openSUSE /usr/share/zsh/functions/Exceptions /usr/share/zsh/functions/MIME /usr/share/zsh/functions/Math /usr/share/zsh/functions/Misc /usr/share/zsh/functions/Newuser /usr/share/zsh/functions/Prompts /usr/share/zsh/functions/TCP /usr/share/zsh/functions/VCS_Info /usr/share/zsh/functions/VCS_Info/Backends /usr/share/zsh/functions/Zftp /usr/share/zsh/functions/Zle )
funcfiletrace
funcsourcetrace
funcstack
functions
functions_source
functrace
fzf_default_completion=expand-or-complete
galiases
histchars='!^#'
history
historywords
jobdirs
jobstates
jobtexts
key=( [BackSpace]=$'\C-?' [Delete]=$'\C-[[3~' [Down]=$'\C-[OB' [End]=$'\C-[OF' [Home]=$'\C-[OH' [Insert]=$'\C-[[2~' [Left]=$'\C-[OD' [PageDown]=$'\C-[[6~' [PageUp]=$'\C-[[5~' [Right]=$'\C-[OC' [Up]=$'\C-[OA' )
keymaps
mailpath=( )
manpath=( )
module_path=( /usr/lib/x86_64-linux-gnu/zsh/5.9 )
modules
nameddirs
options
parameters
patchars
path=( /home/hiro/.pyenv/bin /home/hiro/.pyenv/shims /home/hiro/.mozbuild/arcanist/bin /home/hiro/.cargo/bin /usr/local/sbin /usr/local/bin /usr/sbin /usr/bin /sbin /bin /usr/games /usr/local/games /snap/bin /snap/bin /home/hiro/.mozbuild/fzf/bin /home/hiro/bin /home/hiro/.mozbuild/android-sdk-linux/platform-tools /home/hiro/.local/bin /home/hiro/depot_tools /home/hiro/venv/bin /home/hiro/.mozbuild/git-cinnabar )
pipestatus=( 141 )
prompt='%(!.#.%%) '
psvar=( )
reswords
saliases
signals=( EXIT HUP INT QUIT ILL TRAP IOT BUS FPE KILL USR1 SEGV USR2 PIPE ALRM TERM STKFLT CHLD CONT STOP TSTP TTIN TTOU URG XCPU XFSZ VTALRM PROF WINCH POLL PWR SYS ZERR DEBUG )
status=141
termcap
terminfo
userdirs
usergroups
watch
widgets
zle_bracketed_paste=( $'\C-[[?2004h' $'\C-[[?2004l' )
zsh_eval_context=( toplevel cmdsubst )
zsh_scheduled_events