# STEPS TO REPRODUCE 1. inspect <https://webkit.org> 2. select the `<body>` in the Elements Tab 3. click on the first CSS variable in the Styles panel 4. scroll the Styles panel all the way to the top 5. press Shift-Tab -> the `body` CSS selector is selected 6. press Shift-Tab -> a new property is added to the "Style Attribute" inline style section ## EXPECTED the scroll position should not change ## ACTUAL the Styles panel is scrolled such that the new property is at the top of the Styles panel, hiding the "Style Attribute" # NOTES this is _much_ worse if the rule containing the new property is positioned in the middle of the Styles panel, as it hides everything above the new property
<rdar://problem/90130669>
Created attachment 454881 [details] Recording of bug There’s an ever easier way to test: - Inspect the body element on webkit.org - In the Styles panel, click to add a new CSS declaration to Style Attribute section. The bug manifests by scrolling the Styles panel down to the CSS property name input field. See attached video recording. —- Autospade says: Works: Internal ddfb413d76c9 - OpenSource r290769 Fails: Internal ddfb413d76c9 - OpenSource r290770 https://trac.webkit.org/changeset/290770 Bug 237189 - Treat empty intersection correctly in RenderLayer::getRectToExpose https://bugs.webkit.org/show_bug.cgi?id=237189 But my hunch is that it’s actually caused by another patch: Bug 42593 - element.scrollIntoView() sometimes doesn’t scroll https://bugs.webkit.org/show_bug.cgi?id=42593 That patch also relies on changes to RenderLayer.cpp to determine what counts as visible. So I think our regression point compounded with the changes to how `Element.scrollIntoView()` works. The Changelog is kind of telling: > For some operations which scroll to a rectangle, if an object is more than > 32 pixels onscreen, it’s not considered onscreen. This was originally used > to prevent unnecessary scrolling while tabbing through form fields, but is > no longer used for that in the majority of cases. Instead, the behavior affects > the calls to Element.focus(), Element.scrollIntoView(), and navigations to > anchor elements. I’m not yet sure how to approach this. Ideas are very welcome. cc @Nikita Vasilyev who filed the original bug about `Element.scrollIntoView()`
Created attachment 456640 [details] Patch
Separately, I'll create a test case for r290770. It is possible that r290770 introduced a bug.
Comment on attachment 456640 [details] Patch Do we need to remove this from SpreadsheetSelectorField.js:62 as well?
(In reply to Patrick Angle from comment #5) > Comment on attachment 456640 [details] > Patch > > Do we need to remove this from SpreadsheetSelectorField.js:62 as well? Good catch. Interestingly, this one doesn't seem to cause an unwanted scroll for me.
Created attachment 456727 [details] Patch
Comment on attachment 456727 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456727&action=review > Source/WebInspectorUI/ChangeLog:9 > + Remove unnecessary scrollIntoViewIfNeeded call. I added the scrollIntoViewIfNeeded call in 2017 (r222959) because, There are a lot of these throughout Web Inspector. Are they now all unnecessary? Or are others still useful (and if so, why)?
(In reply to Devin Rousso from comment #8) > Comment on attachment 456727 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=456727&action=review > > > Source/WebInspectorUI/ChangeLog:9 > > + Remove unnecessary scrollIntoViewIfNeeded call. I added the scrollIntoViewIfNeeded call in 2017 (r222959) because, > > There are a lot of these throughout Web Inspector. Are they now all > unnecessary? Or are others still useful (and if so, why)? They are unnecessary when the element is focused by calling `focus()` method, which is only some of those cases.
(In reply to Nikita Vasilyev from comment #6) > (In reply to Patrick Angle from comment #5) > > Comment on attachment 456640 [details] > > Patch > > > > Do we need to remove this from SpreadsheetSelectorField.js:62 as well? > > Good catch. Interestingly, this one doesn't seem to cause an unwanted scroll > for me. It seems odd to me that this reproduces with one but not the other... The patch seems reasonable given that inputs that take focus no longer need to be scrolled into view by us, but I am concerned that there may be a different issue hiding here causing the engine to scroll one element, but not the other, to the top...
(In reply to Patrick Angle from comment #10) > (In reply to Nikita Vasilyev from comment #6) > > (In reply to Patrick Angle from comment #5) > > > Comment on attachment 456640 [details] > > > Patch > > > > > > Do we need to remove this from SpreadsheetSelectorField.js:62 as well? > > > > Good catch. Interestingly, this one doesn't seem to cause an unwanted scroll > > for me. > > It seems odd to me that this reproduces with one but not the other... The > patch seems reasonable given that inputs that take focus no longer need to > be scrolled into view by us, but I am concerned that there may be a > different issue hiding here causing the engine to scroll one element, but > not the other, to the top... I investigated the regression (bug 238985). The unnecessary scrolling to the top only happens with empty containers. That's why it happens when creating a new CSS property, but not anywhere else in Web Inspector AFAIK. For instance, when we create a new CSS rule, we always generate a selector for it. I think we should land my fix now because I don't know when bug 238985 is going to land.
Comment on attachment 456727 [details] Patch Thank you for investigating and finding the underlying cause (and the ping today). I agree, let's take this fix since focusing the element gets us the same behavior in this case anyways.
Comment on attachment 456727 [details] Patch r=me
Committed r292746 (249530@main): <https://commits.webkit.org/249530@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 456727 [details].