Note that https://chromium-review.googlesource.com/1089042 will fix the final activeElement. However we still miss FOCUS/BLUR for anchorLink.

Sigh...few patches have caused me as much pain as that one.

Thank you tkent@ for filing and distilling the bug. I'll take a look this week.

Unfortunately there's been at least 2 regressions, mine was just the first. The behavior I observe in a build prior to my patch:

Click -> FrameLoader::Load
  ->ProcessFragment() with kUrlFragmentScroll
anchorLink.focus()
rAF -> Document::FinishedParsing
  ->ProcessFragment() with kUrlFragmentDontScroll
    -> My patch introduced a bug where we don't change
       focus with kUrlFragmentDontScroll

However, since then there have been some navigation changes so it seems we finish the same document load synchronously during the click. This means we won't try to restore the fragment after focus. I fixed the bug at my patch but the same fix doesn't work in ToT so I need some guidance on how to fix the loading issue.

+dgozman@ and clamy@, The Load call was changed to StartNavigation in https://crrev.com/df3ea35f363255ed60698f5e951522beac51c7c0 but I'm not sure if that would be responsible. Thoughts?

Oops, actually +dgozman@

Ping, clamy@ - can you comment on whether StartNavigation should finish the same document navigation synchronously here and whether this used to be async?

My change was mostly renaming and should not affect sync vs async behavior. Maybe https://crrev.com/af4bf2d9fdaa6ca56ac275c2a53b85f9cf51c394 touched that?

@bokan: yes, same-document navigations are expected to be synchronous. There were not asynchronous before.

+arthursonzogni: who had issues with scrolls during navigations before.

Thanks all. I think I've understood the old behavior a bit better - no asynch involved after all. I've tracked down the second change to:

https://crrev.com/c/1057507 

Moving the ProcessFragment to happen before the load directly contradicts the test case posted by tkent@. tkent, where is this behavior specified?

arthursonzogni@, I'll make my fix and confirm it un-breaks this case when I revert you patch locally but ptal if this change is intended.

> no asynch involved after all.

Yeah, same-document navigation in Blink is synchronous though it should be asynchronous according to the HTML specification. However scroll-to-fragment in Blink was kind of asynchronous until your CL, and now it is triggered on unexpected timing by Document::UpdateStyleAndLayout*(), which is used at many places in Blink.

> Moving the ProcessFragment to happen before the load directly contradicts the test case posted by tkent@

I think they don't contradicts each other.
As for https://crrev.com/c/1057507, the trigger of the navigation is not observable in the loaded Document. So we may handle scroll-to-fragment anytime.
As for my test case, 'load' event is unrelated to the behavior though the test is triggered by it. The trigger of the navigation is anchorLink.click(), and (the navigation and) scroll-to-fragment should be handled after finishing the event handler function.

The behavior is specified in step 4-6 of https://html.spec.whatwg.org/multipage/browsing-the-web.html#update-the-session-history-with-the-new-page .

Sorry I didn't have time to take a look for the moment, but I will take a look next Monday.

> scroll-to-fragment in Blink was kind of asynchronous until your CL, and now it is triggered on unexpected timing by 
> Document::UpdateStyleAndLayout*(), which is used at many places in Blink.

Unless I'm mistaken, my patch doesn't change the fact we call from UpdateStyleAndLayout - it just moves around the calls.

After my patch:

    FrameLoader::ProcessFragment -----------------------------------> LFV::ProcessUrlFragment
                                                                                   |
                                                                                   V
    Document::UpdateLayout---->Document::LayoutUpdated --------> LFV::ScrollAndFocusFragmentAnchor ----> SCROLL 

Before my patch:

    FrameLoader::ProcessFragment ---+
                                    |
    Document::UpdateLayout ---------+----> LFV::ProcessUrlFragment ---> LFV::SetFragmentAnchor --> SCROLL or LAYOUT
                                    |
    Document::ImplicitClose --------+


So we've always tried to do fragment scroll+focus from UpdateStyleAndLayout.

> I think they don't contradicts each other. As for https://crrev.com/c/1057507, the trigger of the navigation is not
> observable in the loaded Document. So we may handle scroll-to-fragment anytime.
> As for my test case, 'load' event is unrelated to the behavior though the test is triggered by it. The trigger of
> the navigation is anchorLink.click(), and (the navigation and) scroll-to-fragment should be handled after finishing
> the event handler function.

scroll-to-fragment used to happen after the anchorLink.focus() because it would be called after the load event finished. The load event handler is dispatched from FrameLoader::FinishedParsing --> CheckCompleted(). It used to be:

FinishedParsing() {
  CheckCompleted();  // Load event fires (anchorLink.click() + anchorLink.focus())
  ProcessFragment(); // scroll-to-fragment + unfocus
}

After 1057505:

FinishedParsing() {
  ProcessFragment(); // scroll-to-fragment + unfocus
  CheckCompleted();  // Load event fires (anchorLink.click() + anchorLink.focus())
}

So we no longer try to scroll-to-fragment after the load event, hence the change in behavior. It looks like the old behavior was maybe accidental accidental since it doesn't look anything like the spec you linked. I suspect we use layout to emulate the "fragment loop" rather than explicitly "spinning the event loop" but in this case there is no layout.

The change in my patch that changes behavior made it so that we no longer set focus if we don't want to scroll-to-fragment - e.g. if we restored scroll from history (that actually seems to match the spec). The change from my patch seems related to the history.back() call, removing it and loading using Chrome at my patch's revision produces the expected output. Adding history.back() in breaks it. The cause is FrameLoader::ProcessFragment calls LFV::ProcessUrlFragment with kUrlFragmentDontScroll (but the fragment still in the URL bar, not sure that's expected) because we've popped scroll state from the history item.

tkent@, can you confirm whether the history.back() call is what you're testing? I'm not familiar enough with history navigations to know what should be happening. If I undo just my focus change at my patch's rev your example works. Undoing it at ToT is broken until I also undo 105705 at which point it works too.

I have one question about the HTML document in #1

I am not sure to understand the point with:
~~~
   requestAnimationFrame(function() { 
     log('activeElement: ' + document.activeElement.tagName);
     history.back();
   });
~~~

I guess it can be replaced by: 

~~~
   setTimeout(function(){
      log('activeElement: ' + document.activeElement.tagName);
   }, 0)
~~~
Am I correct?

__________________________


If I understand correctly, the issue is that we would like the same-document
navigation to be async and happens after 'anchorLink.focus', even if it
was triggered before.

To make the same-document navigation async, maybe we should use the
NavigationScheduler? There is a comment in the code asking why we don't.

| void HTMLAnchorElement::HandleClick(Event* event) {
|   [...]
| 
|   // Why doesn't this go through NavigationScheduler?
|   frame->Loader().StartNavigation(frame_request, WebFrameLoadType::kStandard,
|                                   NavigationPolicyFromEvent(event));
| }

__________________________

What I think happens in this test:

Before my patch:

| FinishedParsing():
|   1) 'load' event
|     1.1) New same-document navigation
|        1.1.1) ProcessFragment() (for the second navigation)
|     1.2) Focus on anchorLink.
|   2) ProcessFragment() (for the first navigation)

After my patch:

| FinishedParsing():
|   1) ProcessFragment() (for the first navigation)
|   2) 'load' event
|     1.1) New same-document navigation
|        1.1.1) ProcessFragment() (for the second navigation())
|     1.2) Focus on anchorLink.

The behavior after my patch looks saner.
ProcessFragment() for the first navigation being no more called
at the end probably explain why we didn't see the problem so far.

#c12, oh, right. Load event and scroll-to-fragment order affects the test. Sorry for the confusing test.

I looked at the code more closely, and my current understanding is that:
- We had async-like behavior if not Document::IsRenderingReady()
 UpdateStyleAndLayout*() didn't do scroll-to-fragment if !IsRenderingReady().
- After bokan's change, UpdateStyleAndLayout*() does scroll-to-fragment unconditionally.

I had the same bisect result with the following test. After bokan's change, anchorLink.offsetWidth changes activeElement.

<!DOCTYPE html>
<style>
.NONE { display: none; }
</style>
<script>
function log(mes) {
  document.querySelector('pre').textContent += mes + '\n';
}
function logEvent(e) {
  if (e.target.nodeType == Node.ELEMENT_NODE)
    log(e.type + ': ' + e.target.nodeName);
}

document.addEventListener('focus', logEvent, true);
document.addEventListener('blur', logEvent, true);
document.addEventListener('DOMContentLoaded', () => {
  var anchorLink = document.getElementById('anchorlink');
  log('Same-document navigation starts ...');
  anchorLink.click();
  log('Force layout ...');
  anchorLink.style.display = 'block';
  anchorLink.offsetWidth;
  log('Did force layout');
  log('activeElement after the force layout: ' + document.activeElement.nodeName);

  window.onload = () => {
    // setTimeout to ignore https://crrev.com/c/1057507
    setTimeout(() => {
      log('activeElement after LOAD: ' + document.activeElement.tagName);

      // Show logs
      document.querySelector('pre').classList.toggle('NONE');
      document.querySelector('div').classList.toggle('NONE');
      // Cleanup
      document.removeEventListener('focus', logEvent, true);
      document.removeEventListener('blur', logEvent, true);
      // For ease of repeating the test.
      // history.back();
    }, 0);
  };
});
</script>
<link rel="stylesheet" type="text/css" href="http://www.haun.org/kent/stall.pl">
</head>
<body><div>
  <div>
    <a id="anchorlink" href="#bottom">Link to bottom of the page</a>
  </div>
  <div style="height: 1000px;"></div>
  <input id="bottom">Bottom of the page</a>
</div>
<pre class="NONE"></pre>
</body>


#c14,
I think the difference between rAF and setTimeout doesn't matter.
My biggest concern is that now scroll-to-fragment happens in random APIs which call Document::UpdateStyleAndLayout*() internally.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9ca40f890a61dfb675f51f45a3b0546d2088455b

commit 9ca40f890a61dfb675f51f45a3b0546d2088455b
Author: David Bokan <bokan@chromium.org>
Date: Wed Aug 15 03:25:38 2018

Avoid fragment activation while rendering blocked

This bug was introduced by:
  https://crrev.com/dbd4eed1920a2aafbc87236db7efb98f108ba063

The previous behavior was to only ProcessUrlFragment if rendering was
ready. If it wasn't, we would mark it as needing processing and try
again when it becomes ready. My patch above separated ProcessUrlFragment
from ScrollAndFocusFragmentAnchor but omitted the deferral of the latter
in cases where rendering wasn't yet ready.

This patch simply early-outs in ScrollAndFocusFragmentAnchor when
rendering isn't yet ready to match the old behavior. It also adds back a
hook in ImplicitClose since a layout may not be needed at the time the
document was closed because it was done at a time when rendering was
still blocked.

Bug:  851338 
Change-Id: I70d4ab233580f7c005fd56e805d5acd79cd4eee6
Reviewed-on: https://chromium-review.googlesource.com/1130958
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583146}
[modify] https://crrev.com/9ca40f890a61dfb675f51f45a3b0546d2088455b/third_party/WebKit/LayoutTests/http/tests/html/click-anchor-link-crash.html
[modify] https://crrev.com/9ca40f890a61dfb675f51f45a3b0546d2088455b/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/9ca40f890a61dfb675f51f45a3b0546d2088455b/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/9ca40f890a61dfb675f51f45a3b0546d2088455b/third_party/blink/renderer/core/frame/local_frame_view_test.cc

I'm marking this as fixed because the regression from my patch is fixed.

> My biggest concern is that now scroll-to-fragment happens in random APIs which call Document::UpdateStyleAndLayout*() internally.

I believe this is still the case in general - we now avoid scroll-to-fragment while rendering is blocked; however, if rendering is unblocked this still happens. That's not a regression though and has been the case since before my patch. We've always called ScrollAndFocusFragmentAnchor at the end of layout.