Web Inspector: Fix issues with go to property arrow in Computed CSS pane when details sidebar is unified

[mirnovov]

b8288eb

Web Inspector: Fix issues with go to property arrow in Computed CSS pane when details sidebar is unified
https://bugs.webkit.org/show_bug.cgi?id=272876

Reviewed by Devin Rousso.

When using the Elements tab of the Inspector, the Computed pane has a button to jump to
the respective CSS property. This works well with the default two-column view, but when
"show independent Styles sidebar" is disabled, it can fail to scroll the respective CSS
into view or replace it with white space.

This commit fixes this problem:
- When opening details panes, do not reset the scroll position.
- Fix a bug that meant that focused/highlighted properties failed to update the layout when
  being initialised (this also can happen for other reasons,
  but is outside the scope of this commit).
- Ensure that all previous properties are always deselected, as occurs already in the
  two-column view.

Combined changes:
* Source/WebInspectorUI/UserInterface/Views/GeneralStyleDetailsSidebarPanel.js:
(WI.GeneralStyleDetailsSidebarPanel.prototype.layout):
* Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:
(WI.SpreadsheetCSSStyleDeclarationEditor):
(WI.SpreadsheetCSSStyleDeclarationEditor.prototype.initialLayout):
(WI.SpreadsheetCSSStyleDeclarationEditor.prototype.selectProperties):
* Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:
(WI.SpreadsheetCSSStyleDeclarationSection.prototype.deselectProperties):
* Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:
(WI.SpreadsheetRulesStyleDetailsPanel.prototype.scrollToSectionAndHighlightProperty):
* Source/WebInspectorUI/UserInterface/Views/StyleDetailsPanel.js:

Canonical link: https://commits.webkit.org/278686@main

c4c8329

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	❌ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 0497d2a)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 556c629)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	🧪 api-mac	🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	⏳ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	🧪 gtk-wk2
	✅ 🛠 tv-sim	🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 390ff95)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

[dcrousso]

rather than call deselectProperties here i wonder if instead we should have WI.SpreadsheetCSSStyleDeclarationEditor.prototype.highlightProperty call this if nothing matches (i.e. right before the final return false;) as that would solve this bug and any like that also exist today or are accidentally added in the future

[mirnovov]

This breaks when the old selection and new selection are under the same rule. I'd have to add it for both branches of WI.SpreadsheetCSSStyleDeclarationEditor.prototype.highlightProperty to work.

Also in the future there may be other features where the property is highlighted but the selection isn't invalidated.

[dcrousso]

what exactly do you mean by "under the same rule"? can you give an example?

[mirnovov]

I.e. within the same CSS rule set or section

.foo {
    color: blue;
    background: red;
}

.bar {
    border: 1px solid green;
}

If I select color and then go to the computed tab and click on the goto arrow for border, it will work, but if I click on the goto arrow next to background it will still break

[webkit-early-warning-system]

EWS run on previous version of this PR (hash e19f0a1)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on previous version of this PR (hash d770b63)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

[dcrousso]

can you explain more why this is needed? is it that this.editing is now true? perhaps the issue is with this._style.locked?

[mirnovov]

The layout() function of this class terminates when editing is true. Until this commit, _focused was always set to true, as usually focusing meanings that we are editing the CSS and we shouldn't layout. However focusing is also done when an item is highlighted (which AFAIK is either when it is arrived at from a go to arrow or other UI element, or when it is selected manually by the user by clicking in the row but outside the text field).

So when the go to arrow was clicked, it would prevent the initial layout and therefore all of the properties could appear blank. This didn't occur with two-column view since the styles pane was already layouted.

this._style.locked doesn't seem to be checked as part of this initial condition in the layout() function, so I don't think that's the cause.

[dcrousso]

hmm that makes me think the real issue is that we consider highlighting as a form of focus, but i can see how it might be difficult/annoying to untangle those two concepts 🤔

maybe we could have the "focus" event handler only do something if it's user initiated (i.e. isTrusted)?

is the issue only on the first layout or in any layout? if the former, perhaps we instead just adjust the this.focus check inside layout to also check for this.didInitialLayout?

[mirnovov]

Yeah, I did think the same thing - that ideally focus wouldn't do double-duty for two disparate things, but it's probably not something to do rn. I also think there may be AX concerns with unbundling it, given that both of them are "focusing" in an accessibility sense.

I have already tried this.didInitialLayout but it's not false when necessary (since it's changed after initialLayout but before the first layout in View). Similarly isTrusted is always true.

[webkit-early-warning-system]

EWS run on previous version of this PR (hash 0326b45)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	🧪 api-mac	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

[webkit-early-warning-system]

EWS run on current version of this PR (hash c4c8329)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	❌ 🛠 watch
	✅ 🛠 watch-sim

[rcaliman-apple]

@mirnovov Is this patch ready to land?

[mirnovov]

I believe so, unless anyone has any issues.

[webkit-commit-queue]

Committed 278686@main (b8288eb): https://commits.webkit.org/278686@main

Reviewed commits have been landed. Closing PR #27503 and removing active labels.