RESOLVED FIXED 157768
REGRESSION (r194717): Web Inspector: Elements tab: an element loses focus when selected by Up/Down key 
https://bugs.webkit.org/show_bug.cgi?id=157768
Summary REGRESSION (r194717): Web Inspector: Elements tab: an element loses focus whe...
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-
DetailsFormatted DiffDiff
Patch (9.90 KB, patch)
2016-06-09 15:15 PDT, Nikita Vasilyev 		no flags
DetailsFormatted DiffDiff
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.