It's a bit odd to have the "Create Local Override..." be part of the breakpoints list, especially since it's not a breakpoint at all.
Created attachment 383512 [details] Patch
Screenshot?
Created attachment 383544 [details] [Image] After Patch is applied
I don't understand the screenshot. What did you click that caused this to happen? ---- I'm not sure I like this direction. The Sources Navigation Sidebar has 2 sections, split in the middle by a navigation bar. - Top Section - Pause Reason - Call Stack - Breakpoints - Local Overrides - Bottom Section - Resources Perhaps a better solution would be moving the (+) Bottom in the bottom corner to the navigation bar in the middle. That way there would be a (+) in the same place (top right) of each navigation bar for each of the 2 sections in the sidebar.
Another way to phrase my position is that it would be an error to think of the top section in the Sources sidebar as only for Breakpoints. I think of this top section as "non-page" state. - The user maintains breakpoints outside the page's control. - The user maintains local overrides outside the page. - The debugger pauses the page, outside of the page's control. The resources in the bottom section are the page's resources, which (for the most part) the page populates. We can inject an extra resource into the page but it goes away with that page.
I understand your point, but I think it's far more antagonistic for user to see multiple + buttons. Frankly, I think we should consider moving all the items in the Add Breakpoint + next to the breakpoints list to be part of the Create Resource + at the bottom as well. With multiple +, it makes it more confusing as now the user has to remember "which one do I need to click in order to get what I want", whereas if there is only one +, there's no "thinking" involved. The idea of these "sections" is really something that we've constructed as the developers of Web Inspector, not something that I think is necessarily obvious or intuitive, or even necessary. I can see the visual distinction between the sections, but it's a somewhat arbitrary concept and I think is too subtle for it to be something we "rely" on. As it is, the Inspector Bootstrap Script is inside the Create Resource + adds an item to the Local Overrides section, so there is already precedent for this. Furthermore, it makes _no_ sense for Local Override... to be inside the Add Breakpoint + because local overrides are _not_ breakpoints. Sure, overriding a JavaScript resource can be thought of as a breakpoint, but it is fundamentally _not_ a breakpoint (the very fact that we call them different things means that they are not the same). I don't think anyone who's trying to create a local override would think to look inside the Add Breakpoint + for the item to do so.
> I understand your point, but I think it's far more antagonistic for user to > see multiple + buttons. Frankly, I think we should consider moving all the > items in the Add Breakpoint + next to the breakpoints list to be part of the > Create Resource + at the bottom as well. With multiple +, it makes it more > confusing as now the user has to remember "which one do I need to click in > order to get what I want", whereas if there is only one +, there's no > "thinking" involved. I don't find it confusing, but I'd be fine with a single (+). I've found that when we have a (+) button at the bottom that I often miss it in a relatively empty sidebar. Case in point the Styles sidebar: It always takes me a few seconds to remember the (+) to add a new rule is at the bottom. So if we went with 1, I'd probably prefer it at the top. > The idea of these "sections" is really something that we've constructed as > the developers of Web Inspector, not something that I think is necessarily > obvious or intuitive, or even necessary. I can see the visual distinction > between the sections, but it's a somewhat arbitrary concept and I think is > too subtle for it to be something we "rely" on. I disagree strongly. We use navigation bars and sections because of their UI/UX implications. If you're suggesting a different separation / sectioning that would be fine, but we have a separation today that makes sense to me as I described above. > Furthermore, it makes _no_ sense for Local Override... to be inside the Add > Breakpoint + because local overrides are _not_ breakpoints. Sure, > overriding a JavaScript resource can be thought of as a breakpoint, but it > is fundamentally _not_ a breakpoint (the very fact that we call them > different things means that they are not the same). I don't think anyone > who's trying to create a local override would think to look inside the Add > Breakpoint + for the item to do so. Again, I do not think of the (+) as an "Add Breakpoint" (+), I think of it as Add something to the top section. So most of your reasoning doesn't make sense to me. It sounds like just renaming the tooltip from "Create Breakpoint" to "Create Breakpoint or Local Override" or something appropriate would be fine.
Talked about this in person. I'm fine with it moving to the bottom left (+) menu because: 1. This is a "Power User" feature of Local Overrides, most Local Overrides will be created through other means (resource content view button or context menus). This creation path is very rare. 2. The existing (+) is tied to the breakpoints section at the top and can therefore be pushed around if paused in the debugger.
Comment on attachment 383512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383512&action=review rs=me > Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:664 > + if (!this._localOverridesTreeOutline.children.length) > + this._localOverridesContainer.hidden = true; Is there another beep and return after this which needs the same thing? Or did that go away.
Comment on attachment 383512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383512&action=review >> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:664 >> + this._localOverridesContainer.hidden = true; > > Is there another beep and return after this which needs the same thing? Or did that go away. What do you mean? There's already a beep and return right below this?
(In reply to Devin Rousso from comment #10) > Comment on attachment 383512 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383512&action=review > > >> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:664 > >> + this._localOverridesContainer.hidden = true; > > > > Is there another beep and return after this which needs the same thing? Or did that go away. > > What do you mean? There's already a beep and return right below this? Each of the early returns should do this. The review tool wasn't letting me expand to see surrounding code but trunk showed 2 early returns.
Comment on attachment 383512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383512&action=review >>>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:664 >>>> + this._localOverridesContainer.hidden = true; >>> >>> Is there another beep and return after this which needs the same thing? Or did that go away. >> >> What do you mean? There's already a beep and return right below this? > > Each of the early returns should do this. The review tool wasn't letting me expand to see surrounding code but trunk showed 2 early returns. Ah. We don't need to do this for the second return as it's only possible to reach that early return if there is an existing local override that matches the popover's configuration. As such, we should have something in the `_localOverridesTreeOutline`.
Comment on attachment 383512 [details] Patch Clearing flags on attachment: 383512 Committed r252517: <https://trac.webkit.org/changeset/252517>
All reviewed patches have been landed. Closing bug.
<rdar://problem/57247092>