Bug 237747

Summary:

Web Inspector: REGRESSION(r290770): Styles: creating a new property scrolls the input to the top of the panel

Product:

WebKit

Reporter:

Devin Rousso <hi>

Component:

Web Inspector

Assignee:

Nikita Vasilyev <nvasilyev>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, hi, inspector-bugzilla-changes, nvasilyev, pangle, rcaliman, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

238985

Bug Blocks:

Attachments:

Description	Flags
Recording of bug	none
Patch	none
Patch	none

Devin Rousso

Reported 2022-03-10 16:38:11 PST

# 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

Attachments
Recording of bug (1.58 MB, video/quicktime) 2022-03-16 12:42 PDT, Razvan Caliman	no flags	Details
Patch (1.76 KB, patch) 2022-04-04 15:30 PDT, Nikita Vasilyev	no flags	Details Formatted Diff Diff
Patch (2.65 KB, patch) 2022-04-05 11:15 PDT, Nikita Vasilyev	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2022-03-10 16:38:25 PST

<rdar://problem/90130669>

Razvan Caliman

Comment 2 2022-03-16 12:42:33 PDT

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()`

Nikita Vasilyev

Comment 3 2022-04-04 15:30:49 PDT

Created attachment 456640 [details] Patch

Nikita Vasilyev

Comment 4 2022-04-04 15:39:56 PDT

Separately, I'll create a test case for r290770. It is possible that r290770 introduced a bug.

Patrick Angle

Comment 5 2022-04-05 08:37:51 PDT

Comment on attachment 456640 [details] Patch Do we need to remove this from SpreadsheetSelectorField.js:62 as well?

Nikita Vasilyev

Comment 6 2022-04-05 11:12:21 PDT

(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.

Nikita Vasilyev

Comment 7 2022-04-05 11:15:50 PDT

Created attachment 456727 [details] Patch

Devin Rousso

Comment 8 2022-04-05 11:40:43 PDT

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)?

Nikita Vasilyev

Comment 9 2022-04-05 12:12:01 PDT

(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.

Patrick Angle

Comment 10 2022-04-05 15:44:36 PDT

(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...

Nikita Vasilyev

Comment 11 2022-04-11 15:37:18 PDT

(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.

Patrick Angle

Comment 12 2022-04-11 15:42:30 PDT

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.

Patrick Angle

Comment 13 2022-04-11 15:42:59 PDT

Comment on attachment 456727 [details] Patch r=me

EWS

Comment 14 2022-04-11 17:24:18 PDT

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].

Note You need to log in before you can comment on or make changes to this bug.