Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scroll bubbling monkeypatches #71

Merged
merged 15 commits into from
Jul 31, 2023
Merged

Add scroll bubbling monkeypatches #71

merged 15 commits into from
Jul 31, 2023

Conversation

blu25
Copy link
Collaborator

@blu25 blu25 commented Apr 11, 2023

@blu25 blu25 requested a review from domfarolino April 11, 2023 16:13
spec.bs Outdated

Modify the [=run the scroll steps=] algorithm to add a check after step 1.1 that reads:

2. If <var ignore=''>target</var> is a {{Document}} whose [=node navigable=]'s
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches the impl behavior that doesn't propagate scroll events into a fenced frame. Is this the expected behavior? I wrote this just to match what we currently have in impl, but if this isn't what the behavior is supposed to be, we should file a crbug to fix.

@domfarolino
Copy link
Collaborator

Could you resolve the conflicts on this PR?

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@domfarolino
Copy link
Collaborator

The wrapping on this PR seems pretty off. Could you fix it?

@domfarolino
Copy link
Collaborator

Formatting is still off :( please check your work

@blu25
Copy link
Collaborator Author

blu25 commented Jun 5, 2023

I got Rewrap working with the spec file, so hopefully the formatting issues shouldn't happen again.

Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please look at the fixes I've made in the last commit so that you do not make them again.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
container/fenced navigable=], then let this be the last instance of this algorithm that stops
any further recursive instances that would otherwise follow.

Note: This allows scrolling to "bubble up" to a fenced frame boundary, but not cross it.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh one more thing, do we have any tests that we can point to here in a <wpt> block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment, no. But we have an item in the fenced frame tracker to migrate the browsertests over to WPTs.

@blu25 blu25 merged commit eb6b50e into master Jul 31, 2023
1 check passed
@blu25 blu25 deleted the liam-scroll-bubbling branch July 31, 2023 20:58
github-actions bot added a commit that referenced this pull request Jul 31, 2023
SHA: eb6b50e
Reason: push, by blu25

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants