Bug 204178 – Web Inspector: Sources: move the "Local Override..." creation context menu item from the Breakpoints section options menu to the Create Resource menu

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 383512 [details]
Patch

[Joseph Pecoraro]

Screenshot?

[Devin Rousso]

Created attachment 383544 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Devin Rousso]

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.

[Joseph Pecoraro]

> 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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Devin Rousso]

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?

[Joseph Pecoraro]

(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.

[Devin Rousso]

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`.

[WebKit Commit Bot]

Comment on attachment 383512 [details]
Patch

Clearing flags on attachment: 383512

Committed r252517: <https://trac.webkit.org/changeset/252517>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57247092>