Bug 157768 - REGRESSION (r194717): Web Inspector: Elements tab: an element loses focus when selected by Up/Down key
Summary: REGRESSION (r194717): Web Inspector: Elements tab: an element loses focus whe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-16 16:25 PDT by Nikita Vasilyev
Modified: 2016-06-09 15:48 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 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.
Comment 1 Radar WebKit Bug Importer 2016-05-16 16:26:02 PDT
<rdar://problem/26311155>
Comment 2 Devin Rousso 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...
Comment 3 Nikita Vasilyev 2016-06-08 14:10:35 PDT
This regressed in http://trac.webkit.org/changeset/194717
Comment 4 Brian Burg 2016-06-08 14:22:04 PDT
Thanks for regressing!
Comment 5 Nikita Vasilyev 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.
Comment 6 Nikita Vasilyev 2016-06-08 17:55:06 PDT
Created attachment 280862 [details]
WIP

This code is gnarly but it appears to be working.
Comment 7 Brian Burg 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.
Comment 8 Devin Rousso 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`).
Comment 9 Nikita Vasilyev 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.
Comment 10 Nikita Vasilyev 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.
Comment 11 Nikita Vasilyev 2016-06-09 15:15:33 PDT
Created attachment 280956 [details]
Patch
Comment 12 WebKit Commit Bot 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>
Comment 13 WebKit Commit Bot 2016-06-09 15:48:01 PDT
All reviewed patches have been landed.  Closing bug.