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>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, joepeck, rik, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Fix
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-

Brian Weinstein
Reported 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.
Attachments
Fix (7.09 KB, patch)
2009-10-24 18:01 PDT, Brian Weinstein
bweinstein: commit-queue-
Fix w/ platform specific modifiers (7.71 KB, patch)
2009-10-24 18:11 PDT, Brian Weinstein
bweinstein: commit-queue-
Fix + Joe's Comments (8.83 KB, patch)
2009-10-24 18:21 PDT, Brian Weinstein
timothy: review+
bweinstein: commit-queue-
Fix + Refactoring (9.98 KB, patch)
2009-10-24 21:09 PDT, Brian Weinstein
timothy: review+
bweinstein: commit-queue-
Brian Weinstein
Comment 1 2009-10-24 18:01:49 PDT
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
Comment 2 2009-10-24 18:11:25 PDT
Created attachment 41802 [details] Fix w/ platform specific modifiers
Brian Weinstein
Comment 3 2009-10-24 18:21:00 PDT
Created attachment 41803 [details] Fix + Joe's Comments
Timothy Hatcher
Comment 4 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!) 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
Comment 5 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.
Timothy Hatcher
Comment 6 2009-10-24 21:43:06 PDT
Brian landed this in r50038.
Note You need to log in before you can comment on or make changes to this bug.