Bug 171902 – REGRESSION (r?): Web Inspector: Shift-click on color square in Styles sidebar should not select text

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Steps:
1. Inspect any element.
2. Add "color: brown" rule.
3. Click twice on the color square before "brown" while holding Shift key.

Expected:
Text selection shouldn't change.

[Devin Rousso]

Created attachment 310468 [details]
Patch

[Matt Baker]

Comment on attachment 310468 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310468&action=review

r=me, with some notes.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:-455
> -        if (this._mouseDownCursorPosition.line === cursor.line && this._mouseDownCursorPosition.ch === cursor.ch) {

Hmm, it would be nice if we could keep this check and return early if it fails. This would improve the readability of the function and avoid doing the work to determine `clickedBookmark` when not needed. To do this would require that `this._mouseDownCursorPosition = null` happen first, instead of at the end of the function. A local `mouseDownCursorPosition` would be needed since the rest of the function uses the value.

I'll leave it up to you. If you think the function is cleanest as-is, I'm fine with it.

[Devin Rousso]

Comment on attachment 310468 [details]
Patch

(In reply to Matt Baker from comment #2)
> Comment on attachment 310468 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310468&action=review
> 
> r=me, with some notes.
> 
> > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:-455
> > -        if (this._mouseDownCursorPosition.line === cursor.line && this._mouseDownCursorPosition.ch === cursor.ch) {
> 
> Hmm, it would be nice if we could keep this check and return early if it
> fails. This would improve the readability of the function and avoid doing
> the work to determine `clickedBookmark` when not needed. To do this would
> require that `this._mouseDownCursorPosition = null` happen first, instead of
> at the end of the function. A local `mouseDownCursorPosition` would be
> needed since the rest of the function uses the value.
> 
> I'll leave it up to you. If you think the function is cleanest as-is, I'm
> fine with it.

I personally would prefer not to do that, as it seems a bit awkward to me to null a value that will be used later while also creating a local variable with that value.  If it gets any more complex, however, it might be worth revisiting.

[WebKit Commit Bot]

Comment on attachment 310468 [details]
Patch

Clearing flags on attachment: 310468

Committed r217072: <http://trac.webkit.org/changeset/217072>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.