RESOLVED FIXED 42593
element.scrollIntoView() sometimes doesn't scroll
https://bugs.webkit.org/show_bug.cgi?id=42593
Summary element.scrollIntoView() sometimes doesn't scroll
Nikita Vasilyev
Reported 2010-07-19 14:26:06 PDT
Created attachment 61991 [details] Reduction This reduction works well in IE 8 and Firefox 4 beta.
Attachments
Reduction (671 bytes, text/html)
2010-07-19 14:26 PDT, Nikita Vasilyev
no flags
Patch (11.07 KB, patch)
2022-01-19 08:31 PST, Martin Robinson
no flags
Patch (12.64 KB, patch)
2022-01-20 03:26 PST, Martin Robinson
no flags
Patch (17.28 KB, patch)
2022-01-20 07:39 PST, Martin Robinson
no flags
Patch (17.26 KB, patch)
2022-01-21 01:28 PST, Martin Robinson
ews-feeder: commit-queue-
Nikita Vasilyev
Comment 1 2010-07-19 14:34:32 PDT
"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
Comment 2 2011-03-15 13:24:30 PDT
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
Comment 3 2021-05-31 03:56:09 PDT
*** Bug 203497 has been marked as a duplicate of this bug. ***
Martin Robinson
Comment 4 2021-05-31 03:56:50 PDT
*** Bug 225387 has been marked as a duplicate of this bug. ***
Martin Robinson
Comment 5 2021-07-08 01:33:03 PDT
*** Bug 192862 has been marked as a duplicate of this bug. ***
Martin Robinson
Comment 6 2022-01-19 06:08:51 PST
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
Comment 7 2022-01-19 08:08:22 PST
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
Comment 8 2022-01-19 08:31:23 PST
Martin Robinson
Comment 9 2022-01-20 02:28:58 PST
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
Comment 10 2022-01-20 03:26:40 PST
Martin Robinson
Comment 11 2022-01-20 07:39:31 PST
Simon Fraser (smfr)
Comment 12 2022-01-20 10:44:33 PST
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
Comment 13 2022-01-21 01:28:42 PST
EWS
Comment 14 2022-01-21 02:37:39 PST
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
Comment 15 2022-01-21 02:38:18 PST
Charlie Croom
Comment 16 2022-02-04 09:40:20 PST
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
Comment 17 2022-02-04 09:53:24 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.