| 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: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Nikita Vasilyev
2016-05-16 16:25:21 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... This regressed in http://trac.webkit.org/changeset/194717 Thanks for regressing! 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. Created attachment 280862 [details]
WIP
This code is gnarly but it appears to be working.
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.
(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`). (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. (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. Created attachment 280956 [details]
Patch
Comment on attachment 280956 [details] Patch Clearing flags on attachment: 280956 Committed r201890: <http://trac.webkit.org/changeset/201890> All reviewed patches have been landed. Closing bug. |