1. open the attached test.html
2. click the link
Expected: the page is scrolled, "Test passed" is displayed
Observed: the page is NOT scrolled

Bisect info: 478608 (good) - 478619 (bad)
https://chromium.googlesource.com/chromium/src/+log/937beecf..54315298?pretty=fuller
Suspecting r478611 "Update HR implementation to match the spec"
Landed in 61.0.3129.0

The spec doesn't forbid "id" attribute on HR [1] nor the ability to use HR as a target for anchor navigation [2] so the observed behavior is a bug. 
Firefox doesn't have the bug.

  [1]: https://html.spec.whatwg.org/multipage/dom.html#global-attributes
       >The class, id, and slot attributes may be specified on all HTML elements.
  [2]: https://html.spec.whatwg.org/multipage/browsing-the-web.html#the-indicated-part-of-the-document
       >If there is an element in the document tree that has an ID equal to fragment, then return the first such element in tree order.

Deleted: test.html 136 bytes

test.html 136 bytes View Download

Please watch this video.
I've confirmed the bug, and recorded:

Sample1:  If the <hr> tag has a style of positive height, it works.
Sample2: If the <hr> tag has no style, it fails.
Sample3: If the <hr> tag has a style of '0px' height, it also fails.

Deleted: chrome_bug3.mp4 5.2 MB

	chrome_bug3.mp4 5.2 MB View Download

>Comment 1
Thank you for showing me a simpler example.


In addition, I've confirmed the same bug in chrome 63 of Chrome canary, too.
Internet Explorer, Edge and Safari don't have the bug, either.

I apologize that the mp4 file I attached to Comment 2 may not be played properly on the browser.
You can play it on Edge. But in Chrome, I guess you can play it if you 'Download' it instead of 'View'.

Able to reproduce the issue on Windows 10, Ubuntu 14.04 and Mac 10.12.6 using chrome stable version #61.0.3163.100 and latest canary #63.0.3225.0.

Bisect Information:
=====================
Good build: 61.0.3128.0	 Revision(478527)
Bad Build : 61.0.3129.0	 Revision(478840)

Change Log URL: 
https://chromium.googlesource.com/chromium/src/+log/63f99bb69d83795394a801343c3827de017ecbbb..a2c5540dd92ca78780a8836b8325bd34440c318f

From the above change log suspecting below change
Change-Id: I1734242d240cb236269b218283bcb16b4ca7c0e4
Reviewed-on: https://chromium-review.googlesource.com/521044 

mstensho@ - Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Note: Adding label ReleaseBlock-Stable as it seems to be a recent regression. Please feel free to remove the same if not appropriate.

Thanks...!!

Related comment in chrome release blog post - https://chromereleases.googleblog.com/2017/09/stable-channel-update-for-desktop_21.html

Since I was updated to Chrome 61.0.3163.100, all my websites that provide scrolling to anchors are affected and not working normally. Ironically, it's working fine on Chrome ... iOS. And all other browsers, MS Edge, FireFox.

I'm using: jQuery.LocalScroll - Animated scrolling navigation, using anchors by http://flesler.blogspot.com

This was all working perfectly on Chrome previous versions - and an older one on another PC.

So any user updated to 61.0.3163.100 can no longer anchor scroll on my various website... aaaaaaah. Please provide a patch or update! Must be alot affected, as anchor scrolling via jQuery is kind of popular.

******************************************************************
+ pbommana@, pdr@, xidachen@, could this be related to  bug 767908 ?

#9, this bug is not related to the old jQuery problem with scrollTopLeftInterop feature because the test html is pure HTML, no scripts.
The CL found by bisecting sets "overflow: hidden" on HR which surprisingly breaks anchor navigation. It shouldn't, of course.
Overriding via "hr { overflow: visible }" or explicitly setting "height: 10px" CSS property hides the bug.

Adding M-62 as well. For sure we should merge a fix to 62 at least.

This is probably not serious enough to justify, by itself, a respin of M61; but it is likely serious enough that the fix should be included in any future M61 push.

I am verifying that reverting mstensho's CL will fix the problem.  If so, then we'll likely need to land a patch that reverts the changes to html.cc and LayoutBlockFlow.cpp, and adds all of the tests to LayoutTests/TestExpectations (rather than re-rebaselining all of the tests).

Morten, if you can then figure out a proper fix, then you can just remove the TestExpectations lines.

Interestingly, it's the overflow:hidden line in html.css that breaks this.

I'd like to give Morten a chance to fix this without reverting his previous patch, so I'm going to wait a day before doing anything.

A bit more to chew on -- I ran this test code:

<!DOCTYPE html>
<hr id="abc">
<div style="height:2000px"></div>
<a href="#abc">abc</a>

... and here's the layout tree dump:

layer at (0,0) size 800x600 clip at (0,0) size 785x600 scrollHeight 2046
  LayoutView at (0,0) size 800x600
layer at (0,0) size 785x2046 backgroundClip at (0,0) size 785x600 clip at (0,0) size 785x600
  LayoutBlockFlow {HTML} at (0,0) size 785x2046
    LayoutBlockFlow {BODY} at (8,8) size 769x2030
      LayoutBlockFlow {DIV} at (0,10) size 769x2000
      LayoutBlockFlow (anonymous) at (0,2010) size 769x20
        LayoutInline {A} at (0,0) size 22x19 [color=#0000EE]
          LayoutText {#text} at (0,0) size 22x19
            text run at (0,0) width 22: "abc"
        LayoutText {#text} at (0,0) size 0x0
layer at (8,8) size 769x2 clip at (0,0) size 0x0
  LayoutBlockFlow {HR} at (0,0) size 769x2 [border: (1px inset #EEEEEE)]
#EOF
#EOF
#EOF


It's weird that the clip on the hr's layer has size 0x0.  Makes me think there's something wrong with the way the overflow clip rect is calculated for hr.

The incorrect clip rect could be messing with hit test behavior used by Element.scrollIntoView

Morten is on vacation until 10/2, so I'd suggest reverting now.  Should be easy to reland later.

Sorry for the slow reply. Kind of on vacation, but trying to keep an eye on things.

Although my change for sure made this more easily reproducible with HR elements, the bug itself has little to do with HR elements. Attaching a test that uses an empty DIV with overflow:hidden instead. This fails in Chrome (with our without my change), but passes in Firefox.

By all means, revert if it's urgent (updating TestExpectations rather than rebaselining sounds like a really good idea), but the ideal step forwards would be to fix the underlying (and possibly very old) problem.

Deleted: tc.html 214 bytes

tc.html 214 bytes View Download

Using #19 tc.html, per-snapshot bisect info: 415055 (good) - 415098 (bad)
https://chromium.googlesource.com/chromium/src/+log/b6cc4b6e..a5d40669?pretty=fuller
Might be r415081 "Make document.rootScroller work properly across iframes."
Might be r415082 "Clip PaintLayerScrollableArea::scrollIntoView return to layer bounds"
Landed in 55.0.2845.0

Thanks! Then it's definitely r415082, because I just debugged and ended up in LayoutBox::ScrollRectToVisibleRecursive(), where we bail if the rectangle is empty. That was introduced by that commit.

The test attached to that CL doesn't even trigger the early return in LayoutBox::ScrollRectToVisibleRecursive():

    if (new_rect.IsEmpty())
      return;

So I'll just try to remove it and see if anything starts failing.

Reassigning to sunyunjia@ and removing release block label.  This is has been broken for all zero-size block elements for a year now; mstensho's change merely caused the breakage to extend to <hr>.

I'll keep the bug for now. :) Working on a fix: https://chromium-review.googlesource.com/c/chromium/src/+/693354

Just removing the code will cause LayoutTests/http/tests/devtools/console/console-focus.html to regress. This is a test that was added after r415082 landed, and it seems to me that this test somehow depends on the new behavior introduced by r415082. I don't know how to deal with this. Feel free to steal the test from my (now abandoned) CL, though. :)

Thanks for looking into it. There's a bit of code above that's meant to handle the case of empty layout rects (we treat it as a 1x1 rect) but I'm wondering why that's not helping.

sunyunjia@ is out this week. I have plenty of context here though so I'll take a look.

So it does seem like DevTools is relying on any change that would fix this. Our original motivation here was to match Firefox on another ScrollIntoView bug but it seems like there's something subtle in how they handle these "empty size parent" cases. 

+luoe for devtools magic :)

Magic, incoming.  In DevTools, we have a textarea inside a parent with height 0, the console prompt, and we expect that textarea.focus() will not scroll.

DevTools depends on this to avoid  https://crbug.com/706128  .  I'm currently trying a workaround.  If it works safely, I think we should try to land a workaround along with mstensho's abandoned CL.

Could we treat focus scrolling differently from anchor (hash) scrolling?

Long term,  issue 734166  will help.

 Issue 769652  has been merged into this issue.

DevTools has a simple workaround of explicitly restoring scrollTop to prevent scrolling.  Here's what it looks like:
https://chromium-review.googlesource.com/c/chromium/src/+/702685

If we could separate focus scrolling from hash scrolling, that would work, too.  I'm not sure which fix is preferred / whether sites in the wild depend on this.

Thanks luoe@, I think that's a better fix as we already have enough exceptions and complications in the ScrollIntoView logic. 

I'd like to double check that landing a DevTools workaround is enough.  The CL DevTools depended on was introduced in late 2016, so the web has been affected for ~1 year.
https://chromium.googlesource.com/chromium/src/+/29cea456ef3562dd7364e8e6e80aaddc0365a516

In my CL, it was brought up that we could do a blink-dev PSA, but I don't think there's much time before M62's promotion.  I don't have much knowledge about Blink scrolling, but it seems less risky to revert the mstensho@'s original CL for now.  What do others think?

I don't think any PSA is needed.  We don't normally send PSA's or intents for minor bug fixes, even though they technically change observable behavior.

Failing to scroll to zero-height elements is obviously a bug, recently introduced and only in Chrome.  I think it's unlikely any significant number of webpages are relying on it, even though devtools is.

I also don't think there's much value in reverting mstensho's patch, since that would only fix <hr> elements, and not other zero-height elements.  I think it's safe enough to just land and merge http://crrev.com/c/702685.

Thanks for weighing in, skobes@.  I'm proceeding with landing the patch.

Since I'm more familiar with DevTools than scrolling/focus, I think someone else with more familiarity would be able to own this, ensure merge conflicts are resolved, etc.  bokan@, would you mind taking this back? :)

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

commit b8ac8ae0a029de8ca616c5a046030c50385a4b54
Author: Erik Luo <luoe@chromium.org>
Date: Tue Oct 10 01:10:15 2017

Allow scrolling anchors established by empty layers.

HR elements recently got visiblity:hidden added to the UA style sheet, which
means that they establish a layer with zero content size by default. We need
to be able to jump (scroll) to such HR elements if the user clicks on an <a
href="#id">, where #id is the ID of said HR.

Remove an empty rect check that seems unnecessary.

DevTools Console prompt was depending on this check to prevent scrolling when
focused. Now, it will prevent scrolling explicitly.

Bug:  769174 
Change-Id: I10d8055da7e878e4a06bc672c593166e6f434c46
Reviewed-on: https://chromium-review.googlesource.com/702685
Commit-Queue: Erik Luo <luoe@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507561}
[add] https://crrev.com/b8ac8ae0a029de8ca616c5a046030c50385a4b54/third_party/WebKit/LayoutTests/fast/scrolling/empty-layer-anchor.html
[modify] https://crrev.com/b8ac8ae0a029de8ca616c5a046030c50385a4b54/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/b8ac8ae0a029de8ca616c5a046030c50385a4b54/third_party/WebKit/Source/devtools/front_end/console/ConsoleView.js

This bug hit so many people because <hr><h3 id="abc"> will ignore the top margin when scrolling to <a href="#abc">links</a>.

It seems the new approach is to better recognize <hr> as a layout divider... so maybe for <a href="#abc">links</a> to an element with a top margin that butts against a layout divider like <hr>, the scroll will recognize any top-margin on the anchored element.

Before, the simple fix was <hr id="abc"><h3>, which gave the expected results.  I'm guessing this is a widespread problem.

 Issue 774764  has been merged into this issue.

Re 39: The bug here is that we simply wouldn't scroll at all, not that the scroll goes to the wrong place. Re: margins, I'd like to avoid special casing elements. In general, we should be trying to match how other browsers behave.

Oh, and if it's not clear, the patch in #38 fixes the regression. It should ship in M63 :)