Bug 157768

Summary:

REGRESSION (r194717): Web Inspector: Elements tab: an element loses focus when selected by Up/Down key

Product:

WebKit

Reporter:

Nikita Vasilyev <nvasilyev>

Component:

Web Inspector

Assignee:

Nikita Vasilyev <nvasilyev>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, commit-queue, hi, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
[Animated GIF] Bug	none
WIP	nvasilyev: review-, nvasilyev: commit-queue-
Patch	none

Nikita Vasilyev

Reported 2016-05-16 16:25:21 PDT

Created attachment 279064 [details] [Animated GIF] Bug Steps: 1. Open https://webkit.org 2. Open Inspector, select Elements tab 3. In the DOM outline, click on <body> 4. Click on any CSS property in the styles sidebar 5. Press Down, Up, Up Expected: <head> element is selected Actual: <body> element is selected because styles sidebar stole the focus when <body> was selected Notes: This works as expected in Safari in El Capitan, but it's broken in Safari Technology Preview.

Attachments
[Animated GIF] Bug (81.65 KB, image/gif) 2016-05-16 16:25 PDT, Nikita Vasilyev	no flags	Details
WIP (3.60 KB, patch) 2016-06-08 17:55 PDT, Nikita Vasilyev	nvasilyev: review- nvasilyev: commit-queue-	Details Formatted Diff Diff
Patch (9.90 KB, patch) 2016-06-09 15: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 2016-05-16 16:26:02 PDT

<rdar://problem/26311155>

Devin Rousso

Comment 2 2016-05-26 22:13:03 PDT

This may be happening because of the logic with "this._previousFocusedSection" on UserInterface/Views/RulesStyleDetailsPanel.js:471. I remember changing it so that it will try to reselect the previously selected rule in the case that the panel was refreshed. Now that I look back on it, it doesn't really cover all of the cases, seeing as it only remembers the last focused section...

Nikita Vasilyev

Comment 3 2016-06-08 14:10:35 PDT

This regressed in http://trac.webkit.org/changeset/194717

Blaze Burg

Comment 4 2016-06-08 14:22:04 PDT

Thanks for regressing!

Nikita Vasilyev

Comment 5 2016-06-08 15:40:48 PDT

This isn't as easy to fix as I anticipated. I don't know any straightforward way to distinguish: 1. WebInspector.CSSStyleDeclarationSection created by pressing "+" 2. WebInspector.CSSStyleDeclarationSection created by selecting a node in DOM tree outline. In both cases WebInspector.RulesStyleDetailsPanel#refresh gets called, whipping out all previous DOM nodes and creating new ones from scratch.

Nikita Vasilyev

Comment 6 2016-06-08 17:55:06 PDT

Created attachment 280862 [details] WIP This code is gnarly but it appears to be working.

Blaze Burg

Comment 7 2016-06-08 19:13:38 PDT

Comment on attachment 280862 [details] WIP I'm not sure what your patch is doing, but I guess you will add a changelog. Based on where I got debugging this issue, the RulesStyleDetailsPanel never clears this._previousFocusedSection when the text editor loses focus by clicking on the DOM tree. The code attempts to pipe codemirror's "blur" event from the CSSStyleDeclarationTextEditor to the CSSStyleDeclaration to the RulesStyleDetailsPanel, but the link between the last two didn't seem to be working. The panel tries to add and remove a listener for this event, but it apparently isn't installed at the time the editor is blurred.

Devin Rousso

Comment 8 2016-06-08 20:46:26 PDT

(In reply to comment #5) > This isn't as easy to fix as I anticipated. > > I don't know any straightforward way to distinguish: > > 1. WebInspector.CSSStyleDeclarationSection created by pressing "+" > 2. WebInspector.CSSStyleDeclarationSection created by selecting a node in > DOM tree outline. > > In both cases WebInspector.RulesStyleDetailsPanel#refresh gets called, > whipping out all previous DOM nodes and creating new ones from scratch. I don't think anyone really needs/uses the feature where reselecting a node will focus the previously selected style section. I would probably just change the logic to remove that feature entirely and make it so that only newly created rules are automatically focused. In order to do that, you could probably just remove the entirety of the logic around `_previouslyFocusedSection` or rework it where it only gets set when a new rule is added (via `_newInspectorRuleSelector` or `newRuleButtonClicked`).

Nikita Vasilyev

Comment 9 2016-06-09 14:03:10 PDT

(In reply to comment #8) > (In reply to comment #5) > > This isn't as easy to fix as I anticipated. > > > > I don't know any straightforward way to distinguish: > > > > 1. WebInspector.CSSStyleDeclarationSection created by pressing "+" > > 2. WebInspector.CSSStyleDeclarationSection created by selecting a node in > > DOM tree outline. > > > > In both cases WebInspector.RulesStyleDetailsPanel#refresh gets called, > > whipping out all previous DOM nodes and creating new ones from scratch. > > I don't think anyone really needs/uses the feature where reselecting a node > will focus the previously selected style section. I would probably just > change the logic to remove that feature entirely and make it so that only > newly created rules are automatically focused. In order to do that, you > could probably just remove the entirety of the logic around > `_previouslyFocusedSection` or rework it where it only gets set when a new > rule is added (via `_newInspectorRuleSelector` or `newRuleButtonClicked`). This is exactly what I'm doing in my patch! 1. Removed _previouslyFocusedSection. 2. Added _inspectorSection and _isInspectorSectionPendingFocus, which serve the same purpose as _newInspectorRuleSelector or newRuleButtonClicked.

Nikita Vasilyev

Comment 10 2016-06-09 14:10:21 PDT

(In reply to comment #7) > Comment on attachment 280862 [details] > WIP > > I'm not sure what your patch is doing, but I guess you will add a changelog. > > Based on where I got debugging this issue, the RulesStyleDetailsPanel never > clears this._previousFocusedSection when the text editor loses focus by > clicking on the DOM tree. > > The code attempts to pipe codemirror's "blur" event from the > CSSStyleDeclarationTextEditor to the CSSStyleDeclaration to the > RulesStyleDetailsPanel, but the link between the last two didn't seem to be > working. The panel tries to add and remove a listener for this event, but it > apparently isn't installed at the time the editor is blurred. I removed _previousFocusedSection entirely since it isn't needed. It was added in bug 117600 Web Inspector: New Rule button doesn't reveal the newly added rule if not visible upon creation http://trac.webkit.org/changeset/185709/trunk/Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js I made sure my patch didn't regress it.

Nikita Vasilyev

Comment 11 2016-06-09 15:15:33 PDT

Created attachment 280956 [details] Patch

WebKit Commit Bot

Comment 12 2016-06-09 15:47:57 PDT

Comment on attachment 280956 [details] Patch Clearing flags on attachment: 280956 Committed r201890: <http://trac.webkit.org/changeset/201890>

WebKit Commit Bot

Comment 13 2016-06-09 15:48:01 PDT

All reviewed patches have been landed. Closing bug.

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