When selector is modified and no longer matches the selected element, it should stay on screen until the new element is selected. This is a new enhancement; we don't do it in the old styles sidebar.
<rdar://problem/35062434>
Created attachment 329468 [details] Patch
Comment on attachment 329468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329468&action=review > Source/WebInspectorUI/ChangeLog:14 > + Drive-by fix: fix tabbing so that it will wrap to the first/last rule. I recommend splitting this into its own change. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:217 > - _renderSelector() > + _renderSelector(selectors) Should this be renamed "_renderSelectors" plural? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:237 > - break; > + index = this._sections.length - 1; What is up here? Seems like we could get into an infinite loop if all sections are not editable. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:284 > + _focusedSectionDoesNotMatch() This reads poorly. Does not match what?
Comment on attachment 329468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329468&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:237 >> + index = this._sections.length - 1; > > What is up here? Seems like we could get into an infinite loop if all sections are not editable. This could get into an infinite loop, but I don't think it's possible since if every section is not editable then none of the section switching code would ever be reachable. We only call these functions when the section's selector or property editor is selected, meaning that if it's not editable then we don't have any keydown listeners. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:284 >> + _focusedSectionDoesNotMatch() > > This reads poorly. Does not match what? The idea is to return false if the section would not re-appear if we were to regenerate the list of sections based on the DOMNodeStyles. I had a hard time naming this :P
Devin, could you rebaseline the patch? It no longer applies to WebKit ToT. I'd like to get this landed soon.
Created attachment 335759 [details] Patch
Comment on attachment 335759 [details] Patch Attachment 335759 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/6947382 New failing tests: http/tests/preload/download_resources.html
Created attachment 335764 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 335759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335759&action=review Overall this looks good. r- only because of the following issue encountered when testing: Changes to the selector are lost when committing with Tab or Enter. Steps to Reproduce: 1. Inspect: <style> p { color: blue; } </style> <p>Some text.</p> 2. Elements tab > select the <p> DOM node 3. Styles sidebar > click on the "p" selector, change to "p.foo". 4. Hit enter or tab. Expected: => Selector shows "p.foo", first property name becomes selected. Actual: => Selector reverts back to "p". Note: Repeating the steps a second time works. The selector shows "p.foo" and is grayed out. > Source/WebInspectorUI/ChangeLog:10 > + (SpreadsheetRulesStyleDetailsPanel) who keeps track of the focused section. When a refresh who -> which > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:105 > + this.element.addEventListener("click", this._handleClick.bind(this)); This line doesn't need to move, it just causes unnecessary code churn. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:269 > + var usingCustomSelectors = selectors !== this._style.ownerRule.selectors; This should move into the `if (selectors.length) { ... }` block below, this that is the only place it's used. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:427 > + Remove extra newline. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:71 > + let uniqueOrderedStyles = (styles) => { Is there a good reason for renaming this? Seems like unnecessary churn for no benefit.
Comment on attachment 329468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329468&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:217 >> + _renderSelector(selectors) > > Should this be renamed "_renderSelectors" plural? We're just rendering a single selector, but it is a little confusing because of the argument's name.
Created attachment 340634 [details] Patch This doesn\'t quite work yet, but it's close. There seems to be an issue with how `significantChange` is calculated, since `WI.DOMNodeStyles` seems to think that the changing of the selector warrants that, even though it probably shouldn\'t.
Comment on attachment 340634 [details] Patch Attachment 340634 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7718582 New failing tests: http/tests/security/canvas-remote-read-remote-video-redirect.html
Created attachment 340679 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 356749 [details] Patch Trying a different approach. The patch could probably be cleaned up a bit, but I wanted to get everyone's opinion on this approach before really committing to it.
Comment on attachment 356749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356749&action=review Seems to work, but It's hard to tell which changes are essential to fixing the problem. Could you drop the unnecessary refactoring and add a change log comment or two? Doesn't have to be long, just a sentence or two explaining the approach you took. > Source/WebInspectorUI/UserInterface/Models/CSSRule.js:59 > + update(sourceCodeLocation, selectorText, selectors, matchedSelectorIndices, style, mediaList) Why was the flag dropped?
Comment on attachment 356749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356749&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:59 >> + update(sourceCodeLocation, selectorText, selectors, matchedSelectorIndices, style, mediaList) > > Why was the flag dropped? It's no longer needed, as the only use of it was to prevent `WI.CSSRule.Event.Changed` from being fired. There were no listeners for that event, so it felt fine to remove.
Created attachment 357995 [details] Patch
(In reply to Devin Rousso from comment #16) > Comment on attachment 356749 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=356749&action=review > > >> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:59 > >> + update(sourceCodeLocation, selectorText, selectors, matchedSelectorIndices, style, mediaList) > > > > Why was the flag dropped? > > It's no longer needed, as the only use of it was to prevent > `WI.CSSRule.Event.Changed` from being fired. There were no listeners for > that event, so it felt fine to remove. Okay! That kind of thing is pretty hard to determine in a code review.
Comment on attachment 357995 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357995&action=review Looks good! I'd feel better if Nikita looked over this too since he works on this code a lot. > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:54 > + static parseSelectorListPayload(selectorList) Why was this made static? It is only used within DOMNodeStyles. Is it going to be reused, or is it purely because it wasn't referencing `this`? > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:73 > + static createSourceCodeLocation(sourceURL, {line, column, documentNode} = {}) Why was this made static? Is it going to be used outside this class? Unlike the other method, making this static required changing the signature. Both these changes feel like unnecessary churn for no benefit. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:213 > + Style: I'd drop this empty line, and add one... > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:222 > + }); ...here! IMO it makes this section easier to read.
Comment on attachment 357995 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357995&action=review >> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:54 >> + static parseSelectorListPayload(selectorList) > > Why was this made static? It is only used within DOMNodeStyles. Is it going to be reused, or is it purely because it wasn't referencing `this`? This is also used by `WI.CSSRule.prototype._selectorResolved`. >> Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:73 >> + static createSourceCodeLocation(sourceURL, {line, column, documentNode} = {}) > > Why was this made static? Is it going to be used outside this class? Unlike the other method, making this static required changing the signature. Both these changes feel like unnecessary churn for no benefit. It's used by `WI.CSSRule.prototype._selectorResolved`.
Created attachment 358002 [details] [Video] Bug (attachment 357995 [details]) r- because it creates section duplicates.
Created attachment 358003 [details] Patch When a selector is changed back to something that applies to the current node, `WI.DOMNodeStyles` will create a new `WI.CSSStyleDeclaration` for that style, but the id will remain the same as the old style. Don\'t add any sections if there already exists a section that has the same id.
(In reply to Devin Rousso from comment #23) > Created attachment 358003 [details] > Patch > > When a selector is changed back to something that applies to the current > node, `WI.DOMNodeStyles` will create a new `WI.CSSStyleDeclaration` for that > style, but the id will remain the same as the old style. Don\'t add any > sections if there already exists a section that has the same id. It makes sense that the selector applies to the current node, and so a declaration is created. I don't follow why there is another section though.
(In reply to Matt Baker from comment #24) > It makes sense that the selector applies to the current node, and so a declaration is created. I don't follow why there is another section though. Two sections appear because we’ve saved them inside `preservedSections`. One of them comes from the newly created section for the newly created store after the refresh, and the other comes from the section we’ve kept around in the preserved list.
(In reply to Devin Rousso from comment #25) > (In reply to Matt Baker from comment #24) > > It makes sense that the selector applies to the current node, and so a declaration is created. I don't follow why there is another section though. > Two sections appear because we’ve saved them inside `preservedSections`. > One of them comes from the newly created section for the newly created store > after the refresh, and the other comes from the section we’ve kept around in > the preserved list. I understand there is a technical rationale for the behavior, but it is unexpected and feels like a bug.
Created attachment 360915 [details] Patch
Comment on attachment 360915 [details] Patch Does this latest patch fix the duplicate section issue?
Created attachment 365415 [details] Patch The previous patch fixed the "double section" issue. This update fixes an issue with pseudo-element styles using the same key in the maps on each `WI.DOMNodeStyles`.
Comment on attachment 365415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365415&action=review > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:80 > + // Try to use the node to find the frame which has the correct resource first. Bad indentation. > Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js:89 > + // If that didn't find the resource, then search all frames. Ditto. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:366 > + section.__showingForNode = this.nodeStyles.node; > + section.__index = this._sections.indexOf(section); Should change these to symbols so they are only visible in this file.
Created attachment 365434 [details] Patch
Comment on attachment 365434 [details] Patch Rejecting attachment 365434 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 365434, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/11587525
Created attachment 365436 [details] Patch
Comment on attachment 365436 [details] Patch Clearing flags on attachment: 365436 Committed r243264: <https://trac.webkit.org/changeset/243264>
All reviewed patches have been landed. Closing bug.