Bug 42593 – element.scrollIntoView() sometimes doesn't scroll

• Component: DOM
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Created attachment 61991 [details]
Reduction

This reduction works well in IE 8 and Firefox 4 beta.

[Nikita Vasilyev]

"Visual user agents should further scroll horizontally as necessary to bring the element to the attention of the user."

http://www.w3.org/TR/html5/editing.html#dom-scrollintoview

[Emil A Eklund]

The current behavior is intentional. Whether it's correct or not I'll leave unsaid for now.

RenderLayer.cpp:

    #define MIN_INTERSECT_FOR_REVEAL 32

    if (intersectWidth == exposeRect.width() || intersectWidth >= MIN_INTERSECT_FOR_REVEAL)
        // If the rectangle is fully visible, use the specified visible behavior.
        // If the rectangle is partially visible, but over a certain threshold,
        // then treat it as fully visible to avoid unnecessary horizontal scrolling

[Martin Robinson]

*** Bug 203497 has been marked as a duplicate of this bug. ***

[Martin Robinson]

*** Bug 225387 has been marked as a duplicate of this bug. ***

[Martin Robinson]

*** Bug 192862 has been marked as a duplicate of this bug. ***

[Martin Robinson]

This code was added in https://trac.webkit.org/changeset/9747/webkit (2005) and the comment from the ChangeLog at the time was:

```
Horizontal scrolling while tabbing through forms can be surprising, and distracting.  This
change will minimize that scrolling if we're not explicitly trying to center a rectangle in
_KWQ_scrollRectToVisible.
```

So perhaps a way forward here is to only maintain the threshold while tabbing through forms and then decide whether or not to eliminate it later.

[Martin Robinson]

It seems like this MIN_INTERSECT_FOR_REVEAL is not used when tabbing through fields any longer:

1. Tabbing with the keyboard through forms eventually calls HTMLTextFormControlElement::setSelectionRange 
2. HTMLTextFormControlElement::setSelectionRange calls FrameSelection::moveWithoutValidationTo
3. FrameSelection::moveWithoutValidationTo calls FrameSelection::setSelection
4. FrameSelection::setSelection calls FrameSelection::updateAndRevealSelection with RevealExtentOption::RevealExtent because options **does not** contain RevealSelectionBounds
5. FrameSelection::updateAndRevealSelection calls FrameSelection::revealSelection
6. FrameSelection::revealSelection assigns the caret boundaries to the reveal rectangle since revealExtentOption == RevealExtent (passed from step 4) and uses the rect in the call to RenderLayer::scrollRectToVisible
7. scrollRectToVisible calls getExtentRect which compares the intersection of the rectangle to reveal to the exposure rectangle. Since the caret is usually one pixel wide, this rectangle is never larger than MIN_INTERSECT_FOR_REVEAL (which is 32) and the condition is never fulfilled.

My conclusion is that MIN_INTERSECT_FOR_REVEAL is interfering with scrolling to elements in a lot of cases, but never for its original purpose. It seems like it is safe just to remove this legacy workaround.

Note: The meanings of RevealExtent and RevealSelectionBounds seems to be opposite from each other, which is very confusing. The only time that RevealSelectionBounds is used is when restoring highlights from AppHighlightStorage via temporary selection changes.

[Martin Robinson]

Created attachment 449484 [details]
Patch

[Martin Robinson]

Update: This constant was removed from blink in https://bugs.chromium.org/p/chromium/issues/detail?id=916631&q=MIN_INTERSECT_FOR_REVEAL&can=1. They found a couple issues when doing so:

1. This changed the behavior of scrolling to an anchor, which are the failures that the current patch sees. 
2. This also changed the behavior of "focus()" aligning some elements to the left edge when this did not happen in the past.

There's an issue open about focus() on the W3C bug tracker [1]. Firefox has another behavior where it decided whether or not to align using a calculation based on the line height.

I think there are two options available to WebKit:

1. Keep the current threshold behavior, but add a new option to override it for script calls. This will allow scrollIntoView() to be spec-compliant without changing the behavior of other parts of the code.
2. Modify the other parts of the code to use ScrollAlignment::alignToEdgeIfNotVisible.

It's quite likely that something will need to change here, if the W3C ever standardizes the behavior that should happen for "focus()"


1. https://github.com/w3c/csswg-drafts/issues/4778

[Martin Robinson]

Created attachment 449562 [details]
Patch

[Martin Robinson]

Created attachment 449573 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 449573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449573&action=review

> Source/WebCore/rendering/ScrollAlignment.h:68
> +    void disableLegacyHorizontalVisibilityTreshold() { m_enableLegacyHorizontalVisibilityThreshold = false; }
> +    bool legacyHorizontalVisibilityTresholdEnabled() const { return m_enableLegacyHorizontalVisibilityThreshold; }

"Treshold"

[Martin Robinson]

Created attachment 449645 [details]
Patch

[EWS]

Committed r288358 (246264@main): <https://commits.webkit.org/246264@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449645 [details].

[Radar WebKit Bug Importer]

<rdar://problem/87875972>

[Charlie Croom]

Hey all, is this fixed in STP? I tested the codepen attached to the original ticket and it didn't seem to be resolved: https://codepen.io/comp615/pen/bGqbrEx, just not sure how the branching / builds work but wanted to help verify this.

[Martin Robinson]

(In reply to Charlie Croom from comment #16)
> Hey all, is this fixed in STP? I tested the codepen attached to the original
> ticket and it didn't seem to be resolved:
> https://codepen.io/comp615/pen/bGqbrEx, just not sure how the branching /
> builds work but wanted to help verify this.

This fix does not appear to be in the latest STP. It landed only a few days before the release.