1888756 - Text-Fragments: Implement User Activation

Status: RESOLVED FIXED

(Core :: DOM: Navigation, task)

Description

[Jan Jaeschke [:jjaschke]]

This bug tracks the implementation of the User Activation behavior of Text Fragments.

Comment 1

[Jan Jaeschke [:jjaschke]]

Attachment: WIP: Bug 1888756: Temp commit for user activation and restricting scroll.

Comment 2

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1888756, part 1 - Text Fragments: Introduce `textDirectiveUserActivation` flag. r=edgar!,#dom-core

This commit is a prerequisite for part 5 of this patch set
and introduces the text directive user activation flag
as well as the necessary boilerplate to transport the value
through process boundaries in nsDocShellLoadState and LoadInfo.

There is no changed behavior in this commit.

Comment 3

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1888756, part 2 - Text Fragments: Implement setting the `textDirectiveUserActivation` flag in the document loading algorithm. r=edgar!,#dom-core

This commit is a prerequisite for part 5 of this patch set.

This flag is set in the docshell, based on user gesture activation.
This is not necessarily sufficient,
as the text fragments spec bases the value of
this flag also on User Involvement, which is not
implemented in Gecko yet.

The flag is then plumbed through the parent process
(DocumentLoadListener::CreateDocumentLoadInfo())
to the document, where it can be consumed.
This includes client-side redirects as described in [0].

There is no changed behavior in this commit.

[0] https://wicg.github.io/scroll-to-text-fragment/#restricting-the-text-fragment

Comment 4

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1888756, part 3 - Text Fragments: Preserve information if a load is a same-document load in `nsILoadInfo`. r=edgar!,#dom-core

This commit is a prerequisite for part 5 of this patch set.

The idea is to keep the information whether the last
load done by the doc shell was a same-document navigation.
The value is reset for every navigation, therefore it
should always be correct.

There is no changed behavior in this commit.

Comment 5

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1888756, part 4 - Text Fragments: Add more logging to parsing/stripping the fragment directive. r=avandolder,#dom-core

This commit is a prerequisite for part 5 of this patch set
and does not introduce any changes in behavior.

Comment 6

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1888756, part 5 - Text Fragments: Implemented user activation. r=edgar!,#dom-core

This commit implements the rules given in the spec [0]
to determine if a text directive is allowed to be scrolled to.

This includes checking and consuming the text directive user activation
which was introduced in the previous parts of this patch set.

Additionally, a number of checks given in [1] are implemented.
The newly added method is injected into our scroll-to-fragment
algorithms in Document::ScrollToRef() and nsDocShell::ScrollToAnchor().

[0] https://wicg.github.io/scroll-to-text-fragment/#restricting-the-text-fragment
[1] https://wicg.github.io/scroll-to-text-fragment/#check-if-a-text-directive-can-be-scrolled

Comment 7

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1888756, part 6 - Text Fragments: Changed order of tests in non-html-documents.html. r=#dom-core

## non-html-documents.html

Gecko does not fire an onload event for JSON files,
which causes the test to time out.
Therefore, the test for JSON has been moved
to the end of the file, so that the other tests
run before the test times out.

Note: The rules implemented in part 5 of this
patch set prevent JSON files from being scrolled to;
Failing this test does not imply incorrect behavior.

## iframe-scroll.sub.html

This test opens a same/cross origin iframe with a text fragment.
According to the test file, the iframe should
scroll to the text fragment in both cases.
Additionally, the same-origin case should update
the parent document's scroll position for the same-origin case.

In the same-origin case, the current Gecko implementation
scrolls inside of the iframe, but does not bubble
the scroll position to the parent document.
In the cross-origin case, the iframe does not scroll,
because it is a cross-origin non-top-level navigation.

Comment 8

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1888756, part 7 - Text Fragments: Tentatively allow tests to fail when fission is not enabled. r=edgar!,#dom-core

Currently, user activation is not transported correctly
if fission is disabled.
This makes a lot of tests fail now, as no user activation
prevents scrolling and most tests rely on scrolling.

Comment 9

[Jan Jaeschke [:jjaschke]]

Here is a current try run.

There are two issues currently:

1. User Activation is not moved across process boundaries correctly if Fission is disabled. This would currently prevent text directives from being scrolled to on Android. I've filed Bug 1901064. Once this is fixed, the test expectations added in part 7 of this patchset can be reverted.
2. There are issues regarding consumption of the user gesture activation in combination with the popup blocker. If the popup blocker is enabled, it consumes the user activation, before the text directive user activation is created based off of its value. When running WPTs, the popup blocker is disabled, hence the tests pass. However, in real-life scenarios, the text directive will typically not be scrolled to if the page is navigated to using window.open(). Edgar is filing a follow-up bug for and looking into this.

Both issues do not leak sensitive information -- text directives that are actually allowed to be scrolled to are not being scrolled to, not vice versa. Therefore these issues should not block landing this feature.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0eb8b13b0717
part 1 - Text Fragments: Introduce `textDirectiveUserActivation` flag. r=edgar,dom-core
https://hg.mozilla.org/integration/autoland/rev/04180e2fb99d
part 2 - Text Fragments: Implement setting the `textDirectiveUserActivation` flag in the document loading algorithm. r=edgar,dom-core
https://hg.mozilla.org/integration/autoland/rev/400a8ff57ae4
part 3 - Text Fragments: Add a flag in the `DocShell` to keep the information if a load was same-document. r=farre,dom-core,edgar
https://hg.mozilla.org/integration/autoland/rev/efff3e5e95c2
part 4 - Text Fragments: Add more logging to parsing/stripping the fragment directive. r=dom-core,edgar
https://hg.mozilla.org/integration/autoland/rev/a84d886d8753
part 5 - Text Fragments: Implemented user activation. r=edgar,dom-core
https://hg.mozilla.org/integration/autoland/rev/1458ad53d229
part 6 - Text Fragments: Changed order of tests in non-html-documents.html. r=dom-core,edgar
https://hg.mozilla.org/integration/autoland/rev/85022f2e4129
part 7 - Text Fragments: Tentatively allow tests to fail when fission is not enabled. r=edgar,dom-core

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46838 for changes under testing/web-platform/tests

Comment 12

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/0eb8b13b0717
https://hg.mozilla.org/mozilla-central/rev/04180e2fb99d
https://hg.mozilla.org/mozilla-central/rev/400a8ff57ae4
https://hg.mozilla.org/mozilla-central/rev/efff3e5e95c2
https://hg.mozilla.org/mozilla-central/rev/a84d886d8753
https://hg.mozilla.org/mozilla-central/rev/1458ad53d229
https://hg.mozilla.org/mozilla-central/rev/85022f2e4129

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 14

[Cristian Tuns]

Backed out for potentially causing Session History crashes.
Backout link: https://hg.mozilla.org/mozilla-central/rev/deea4a904a0d3d546ab359aeca91a4bb6dc02a77

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46859 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c706ebb5cbf8
part 1 - Text Fragments: Introduce `textDirectiveUserActivation` flag. r=edgar,dom-core
https://hg.mozilla.org/integration/autoland/rev/3b60fbb80f67
part 2 - Text Fragments: Implement setting the `textDirectiveUserActivation` flag in the document loading algorithm. r=edgar,dom-core
https://hg.mozilla.org/integration/autoland/rev/142591a5a921
part 3 - Text Fragments: Preserve information if a load is a same-document load in `nsILoadInfo`. r=farre,dom-core,necko-reviewers,kershaw,peterv
https://hg.mozilla.org/integration/autoland/rev/e1915755c9ca
part 4 - Text Fragments: Add more logging to parsing/stripping the fragment directive. r=dom-core,edgar
https://hg.mozilla.org/integration/autoland/rev/339436073065
part 5 - Text Fragments: Implemented user activation. r=edgar,dom-core
https://hg.mozilla.org/integration/autoland/rev/11957961b0fa
part 6 - Text Fragments: Changed order of tests in non-html-documents.html. r=dom-core,edgar
https://hg.mozilla.org/integration/autoland/rev/662204fa8621
part 7 - Text Fragments: Tentatively allow tests to fail when fission is not enabled. r=edgar,dom-core

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46909 for changes under testing/web-platform/tests

Comment 19

[Jan Jaeschke [:jjaschke]]

The new version fixes the session history crashes and adjusts the test results mentioned in bug 1904773.

Comment 20

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/c706ebb5cbf8
https://hg.mozilla.org/mozilla-central/rev/3b60fbb80f67
https://hg.mozilla.org/mozilla-central/rev/142591a5a921
https://hg.mozilla.org/mozilla-central/rev/e1915755c9ca
https://hg.mozilla.org/mozilla-central/rev/339436073065
https://hg.mozilla.org/mozilla-central/rev/11957961b0fa
https://hg.mozilla.org/mozilla-central/rev/662204fa8621

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 22

[Florin Bilt]

Hello !
The following alert was generated by the backout . Do you think this bug or one of the following Bug 1888756 , Bug 1901064, Bug 1888756 is responsible for generating the alert ?

Perfherder has detected a browsertime performance change from push 5f6f41a83f0ea8d64264cf04f49a5940f9c8705b.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
22%	wikipedia fcp	android-hw-a51-11-0-aarch64-shippable-qr	warm webrender	69.60 -> 85.21	Before/After
11%	speedometer3 TodoMVC-Preact-Complex-DOM/Adding100Items/Sync	macosx1400-64-shippable-qr	fission webrender	0.70 -> 0.78	Before/After
8%	speedometer3 TodoMVC-Svelte-Complex-DOM/Adding100Items/Sync	macosx1400-64-shippable-qr	fission webrender	1.51 -> 1.62	Before/After
7%	speedometer3 TodoMVC-Svelte-Complex-DOM/Adding100Items/Sync	windows10-64-shippable-qr	fission webrender	3.21 -> 3.43	Before/After
6%	speedometer3 TodoMVC-Svelte-Complex-DOM/Adding100Items/Sync	windows10-64-nightlyasrelease-qr	fission webrender	3.17 -> 3.37	Before/After
5%	speedometer3 TodoMVC-Preact-Complex-DOM/Adding100Items/total	macosx1400-64-shippable-qr	fission webrender	5.19 -> 5.44	Before/After
4%	speedometer3 TodoMVC-Svelte-Complex-DOM/CompletingAllItems/total	windows10-64-shippable-qr	fission webrender	7.37 -> 7.69	Before/After
4%	speedometer3 Charts-chartjs/Show tooltip/Sync	macosx1400-64-shippable-qr	fission webrender	0.08 -> 0.08	Before/After
4%	speedometer3 TodoMVC-Svelte-Complex-DOM/CompletingAllItems/Async	windows10-64-shippable-qr	fission webrender	5.97 -> 6.18	Before/After
3%	speedometer3 TodoMVC-Preact-Complex-DOM/total	macosx1400-64-shippable-qr	fission webrender	11.59 -> 11.93	Before/After
2%	speedometer3 TodoMVC-Preact-Complex-DOM/DeletingAllItems/Async	macosx1400-64-shippable-qr	fission webrender	1.74 -> 1.78	Before/After

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
17%	fandom largestContentfulPaint	linux1804-64-shippable-qr	fission warm webrender	47.46 -> 39.44	Before/After
9%	speedometer3 TodoMVC-Svelte-Complex-DOM/Adding100Items/Async	windows10-64-nightlyasrelease-qr	fission webrender	9.35 -> 8.52	Before/After
8%	wikia FirstVisualChange	linux1804-64-shippable-qr	cold fission webrender	330.15 -> 305.37	Before/After
7%	speedometer3 TodoMVC-Svelte-Complex-DOM/Adding100Items/Async	windows10-64-shippable-qr	fission webrender	9.36 -> 8.67	Before/After
7%	wikia largestContentfulPaint	linux1804-64-shippable-qr	fission warm webrender	193.05 -> 180.32	Before/After
...	...	...	...	...	...
2%	speedometer3 Charts-observable-plot/Stacked by 6/Async	macosx1400-64-shippable-qr	fission webrender	2.38 -> 2.33	Before/After

As author of one of the patches included in that push, we need your help to address this regression.
Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the patch(es) may be backed out in accordance with our regression policy.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 899

For more information on performance sheriffing please see our FAQ.

Comment 23

[Jan Jaeschke [:jjaschke]]

Hi Florin,

I can take a look, but I highly doubt that the backout of this bug, or the bug itself, can cause a perf regression. The underlying feature is prefed off, so the changes in these patches should be (almost) a noop.

Given that the patches have landed again today, I would suggest re-running the tests to see if this still persists. WDYT?

Comment 24

[Florin Bilt]

(In reply to Jan Jaeschke [:jjaschke] from comment #23)

Hi Florin,

I can take a look, but I highly doubt that the backout of this bug, or the bug itself, can cause a perf regression. The underlying feature is prefed off, so the changes in these patches should be (almost) a noop.

Given that the patches have landed again today, I would suggest re-running the tests to see if this still persists. WDYT?

The regression was recorded on this revision : https://treeherder.mozilla.org/jobs?repo=autoland&revision=5f6f41a83f0ea8d64264cf04f49a5940f9c8705b&group_state=expanded&selectedTaskRun=LHnMakbESkuPnRwRmX-4eg.0

Also you can see the graph:
https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&replicates=0&selected=5042290,43548387&series=autoland,5042290,1,13&timerange=1209600&zoom=1718863319466,1719053273250,1.3119688284156954,1.7668973643782215

In this revision multiple bugs were backed out, including your bugs. (Bug 1901799, Bug 1897271, Bug 1900863, Bug 1902364, Bug 1902671, Bug 1902086, Bug 1901771, Bug 1902003, Bug 1902359, Bug 1888756, Bug 1901064 Bug 1860915 Bug 1888756)
I left a 'need info' request for each of the bug owners, hoping to find out which of the bug caused the alert.

Comment 25

[Fatih Kilic [:fkilic]]

could it be that making text-fragment parsing preffed off actually improved performance and the backout ended up having worse performance? In Bug 1901799#c13 :fbilt mentioned it is the backout that had the worse performance. I see that backout performance profiles introduced extra mozilla::dom::FragmentDirective::FindTextFragmentsInDocument calls, so backing out the preffing off actually made performance worse? Can you confirm if this is what might have happened?

Comment 26

[Jan Jaeschke [:jjaschke]]

Ah, sorry for the confusion. The value of the pref has not changed. FragmentDirective::FindTextFragmentsInDocument() should always be a noop as long as the pref is turned off. Could you show me the exact profile you've been looking into?

Comment 27

[Fatih Kilic [:fkilic]]

Ah I see. This one is the one with our patches (before the backout) and this one is after the backout. I see calls to nsHtml5ExecutorReflusher increased due to mozilla::dom::Document::ScrollToRef in the backout. Everything else looks somewhat similar. This is my first time dealing with perfherder, so if my finding is totally wrong, I'm sorry hahaha.

Comment 28

[Jan Jaeschke [:jjaschke]]

Oof. Nice find. I'll take back everything I said :)

Okay, here's the explanation:
FindTextFragmentsInDocument() did flush frames unconditionally before this bug was applied. Applying part 4 of the patches added an early return in that function before flushing. Therefore, applying the patch improved performance, and backing it out restored the slowness. Now it all makes sense, sorry. I was not aware that I introduced the early return here, I thought it was there all along.

But, at the end of the day, I think no further action is necessary because the change has now landed (again). Sounds good?

Comment 29

[Fatih Kilic [:fkilic]]

Yep, sounds good to me. Thank you!

Comment 30

[Florin Bilt]

Yes, sounds good. Thank you :)