Bug 30752

Summary: Web Inspector: Multiple Selection on Scope Bars by default Conflicts with other behavior on OSX
Product: WebKit Reporter: Brian Weinstein <bweinstein>
Component: Web Inspector (Deprecated)Assignee: Brian Weinstein <bweinstein>
Severity: Normal CC: aroben, joepeck, rik, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
bweinstein: commit-queue-
Fix w/ platform specific modifiers
bweinstein: commit-queue-
Fix + Joe's Comments
timothy: review+, bweinstein: commit-queue-
Fix + Refactoring timothy: review+, bweinstein: commit-queue-

Description Brian Weinstein 2009-10-24 17:57:41 PDT
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.
Comment 1 Brian Weinstein 2009-10-24 18:01:49 PDT
Created attachment 41801 [details]

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.
Comment 2 Brian Weinstein 2009-10-24 18:11:25 PDT
Created attachment 41802 [details]
Fix w/ platform specific modifiers
Comment 3 Brian Weinstein 2009-10-24 18:21:00 PDT
Created attachment 41803 [details]
Fix + Joe's Comments
Comment 4 Timothy Hatcher 2009-10-24 20:23:15 PDT
Comment on attachment 41803 [details]
Fix + Joe's Comments

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


> +            // 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.
Comment 5 Brian Weinstein 2009-10-24 21:09:08 PDT
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.
Comment 6 Timothy Hatcher 2009-10-24 21:43:06 PDT
Brian landed this in r50038.