Web Inspector: Multiple Selection on Scope Bars by default Conflicts with other behavior on OSX

[Brian Weinstein]

In my testing, none of the other scope bars on OSX allow for multiple selections, making the Inspector be an outlier, however, there are use cases where multiple selection is useful, it is just confusing when it is the default. It should be that a click without modifier keys selects just the clicked scope, and a click with the meta key selects multiple options.

I have a patch done, but the one thing I am not sure about is how to handle the case where the multiple selection key should be the Command key on Mac (e.metaKey), but the Control key on Windows (e.ctrlKey). There is a FIXME there for now, but if anyone knows how to handle that, that would be great.

[Brian Weinstein]

Created attachment 41801 [details]
Fix

At some point, it would be good if these two filter functions were factored into a command function, as they are very similar. I'll file a bug for that and try it in a future patch.

[Brian Weinstein]

Created attachment 41802 [details]
Fix w/ platform specific modifiers

[Brian Weinstein]

Created attachment 41803 [details]
Fix + Joe's Comments

[Timothy Hatcher]

Comment on attachment 41803 [details]
Fix + Joe's Comments

> +        Need a short description and bug URL (OOPS!)
> +
> +        No new tests. (OOPS!)

Remove.


> +            // If multiple selection is off, we want to unselect everything else
> +            // and just select ourselves.
> +            for (var i = 0; i < this.filterBarElement.childNodes.length; ++i) {
> +                var child = this.filterBarElement.childNodes[i];
> +                if (!child.category)
> +                    continue;
> +                
> +                child.removeStyleClass("selected");
> +
> +                var filterClass = "filter-" + child.category.toLowerCase();
> +                this.resourcesGraphsElement.removeStyleClass(filterClass);
> +                this.resourcesTreeElement.childrenListElement.removeStyleClass(filterClass);
> +            }

You could factor this into a helper nested function that you share with the other part of this function that also deselects all.


>          if (target.hasStyleClass("selected")) {
> +            // If selectMultiple is turned on, and we were selected, we just
> +            // want to unselect ourselves.
>              target.removeStyleClass("selected");
> -
>              var filterClass = "filter-" + target.category.toLowerCase();

I prefer to keep the empty line that you deleted, it helps readability. You should also factor out the var filterCalss line so you don't need it twice. Like you did for the ConsoleView.


>              target.addStyleClass("selected");
> -
>              var filterClass = "filter-" + target.category.toLowerCase();

Keep that empty line.

[Brian Weinstein]

Created attachment 41811 [details]
Fix + Refactoring

Just wanted one more review on the refactoring for a second set of eyes to make sure I didn't make a silly mistake.

[Timothy Hatcher]

Brian landed this in r50038.