DevTools: Create 2D slider for shadow-editor offset

DevTools: Create 2D slider for shadow-editor offset

This replaces the 1D sliders for X offset and Y offset.

BUG=639095
Committed: https://crrev.com/3fdf53278760d3691bd7cb04203907f45475018d
Cr-Commit-Position: refs/heads/master@{#415728}

[flandy, 2016-08-29 17:28:51]

flandy@google.com changed reviewers:
+ dgozman@chromium.org, lushnikov@chromium.org

[flandy, 2016-08-29 17:28:52]

Please take a look. See the bug for new screenshots.

[lushnikov, 2016-08-30 19:22:19]

I played with the new control and liked it! I only miss "shift-drag" experience
- would be nice to have it

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js (right):

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:304:
this._xySlider.hasFocus = true;
let's not add fields to the DOM wrappers - i think in this case we can just rely
on activeElement

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:319: var
point = new WebInspector.Geometry.Point(event.x - this._canvasOrigin.x, event.y
-  this._canvasOrigin.y);
nit: excessive spaces

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:327:
this._xInput.classList.toggle("invalid", false);
nit: this._xInput.classList.remove("invalid")

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:344: var
upDown = (event.key === "ArrowUp" || event.key === "ArrowDown");
can we use WI.KeyboardShortcuts for key detection?

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:346: if
(!upDown && !leftRight || !this._xySlider.hasFocus)
how could it be non-focused? doesn't even triggering mean it has focus?

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:347: return
; missing

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:353:
newAmount = event.key === "ArrowUp" ? offsetY.amount - 1 : offsetY.amount + 1;
Let's compare for event.key only only once, in the beginning of the function


_onCanvasUpDown: function(e)
{
   var shiftX = 0;
   var shiftY = 0;
   if (isUp)
       shiftY = 1;
   else if (isDown)
       shiftY = -1;
   else if (isLeft)
       shiftX = 1;
   else if (isRight)
       shiftX = -1;
 
   if (!shiftX && !shiftY)
      return;

   if (shiftX) {
       newAmount = oldAmount + shiftX;
       ...
   }
   if (shiftY) {
       ...
   }

}

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:378: if
((Math.abs(point.x) <= max && Math.abs(point.y) <= max) || (point.x === 0 &&
point.y === 0))
why special case for zeroes?

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:380: if
(Math.abs(point.x) > Math.abs(point.y)) {
the function could be written simpler:

   if (Math.abs(point.x) < max &&  Math.abs(point.y) < max)
       return new WebInspector.Geometry.Point(point.x, point.y);
   var scale = max / Math.max(Math.abs(point.x), Math.abs(point.y));
   return new WebInspector.Geometry.Point(point.x * scale, point.y * scale);

Btw, it is usually convenient to treat points as radius-vectors and thus have
"scale" operation on the Point class! We should add one!

[flandy, 2016-08-30 22:50:42]

Please take another look.

> I played with the new control and liked it! I only miss "shift-drag"
experience
> - would be nice to have it

Good to hear. Chris mentioned that if we want an experience like that with
different drag functionality, we shouldn't be using sliders. In another patch,
I've added a function where shift snaps to different 45 degree angles.

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js (right):

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:304:
this._xySlider.hasFocus = true;
On 2016/08/30 19:22:19, lushnikov wrote:
> let's not add fields to the DOM wrappers - i think in this case we can just
rely
> on activeElement

Done. I've added Document.prototype.deepActiveElement to deal with shadow DOM,
but we could also just pass another boolean parameter, drawFocus, to
_updateCanvas.

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:319: var
point = new WebInspector.Geometry.Point(event.x - this._canvasOrigin.x, event.y
-  this._canvasOrigin.y);
On 2016/08/30 19:22:19, lushnikov wrote:
> nit: excessive spaces

Done.

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:327:
this._xInput.classList.toggle("invalid", false);
On 2016/08/30 19:22:19, lushnikov wrote:
> nit: this._xInput.classList.remove("invalid")

Done.

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:344: var
upDown = (event.key === "ArrowUp" || event.key === "ArrowDown");
On 2016/08/30 19:22:19, lushnikov wrote:
> can we use WI.KeyboardShortcuts for key detection?
> 

This seems unnecessary here. Also, keyCode is deprecated
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyCode.

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:346: if
(!upDown && !leftRight || !this._xySlider.hasFocus)
On 2016/08/30 19:22:19, lushnikov wrote:
> how could it be non-focused? doesn't even triggering mean it has focus?

Yes good point. Removed.

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:347: return
On 2016/08/30 19:22:19, lushnikov wrote:
> ; missing

Done.

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:353:
newAmount = event.key === "ArrowUp" ? offsetY.amount - 1 : offsetY.amount + 1;
On 2016/08/30 19:22:19, lushnikov wrote:
> Let's compare for event.key only only once, in the beginning of the function
> 
> 
> _onCanvasUpDown: function(e)
> {
>    var shiftX = 0;
>    var shiftY = 0;
>    if (isUp)
>        shiftY = 1;
>    else if (isDown)
>        shiftY = -1;
>    else if (isLeft)
>        shiftX = 1;
>    else if (isRight)
>        shiftX = -1;
>  
>    if (!shiftX && !shiftY)
>       return;
> 
>    if (shiftX) {
>        newAmount = oldAmount + shiftX;
>        ...
>    }
>    if (shiftY) {
>        ...
>    }
> 
> }

Done.

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:378: if
((Math.abs(point.x) <= max && Math.abs(point.y) <= max) || (point.x === 0 &&
point.y === 0))
On 2016/08/30 19:22:19, lushnikov wrote:
> why special case for zeroes?

To avoid divide by zero if max < 0. I never use max < 0, so I'll remove.

https://codereview.chromium.org/2292583002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:380: if
(Math.abs(point.x) > Math.abs(point.y)) {
On 2016/08/30 19:22:19, lushnikov wrote:
> the function could be written simpler:
> 
>    if (Math.abs(point.x) < max &&  Math.abs(point.y) < max)
>        return new WebInspector.Geometry.Point(point.x, point.y);
>    var scale = max / Math.max(Math.abs(point.x), Math.abs(point.y));
>    return new WebInspector.Geometry.Point(point.x * scale, point.y * scale);
> 
> Btw, it is usually convenient to treat points as radius-vectors and thus have
> "scale" operation on the Point class! We should add one!

Thanks! Done.

[lushnikov, 2016-08-31 00:48:03]

https://codereview.chromium.org/2292583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Geometry.js (right):

https://codereview.chromium.org/2292583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Geometry.js:97: this.x *=
scalar;
let's return a new object instead of mutating

https://codereview.chromium.org/2292583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js (right):

https://codereview.chromium.org/2292583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:160: if
(this._xySlider.ownerDocument.deepActiveElement() === this._xySlider) {
I actually like your "drawFocus" parameter even more then this
deepActiveElement!

https://codereview.chromium.org/2292583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:352: return;
let's do a conventional bail-out

if (!shiftX && !shiftY)
    return;

https://codereview.chromium.org/2292583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:355: var
newAmount;
no need to forward-declare

[flandy, 2016-08-31 01:35:06]

Please take another look.

https://codereview.chromium.org/2292583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/Geometry.js (right):

https://codereview.chromium.org/2292583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/Geometry.js:97: this.x *=
scalar;
On 2016/08/31 00:48:03, lushnikov wrote:
> let's return a new object instead of mutating

Done.

https://codereview.chromium.org/2292583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js (right):

https://codereview.chromium.org/2292583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:160: if
(this._xySlider.ownerDocument.deepActiveElement() === this._xySlider) {
On 2016/08/31 00:48:03, lushnikov wrote:
> I actually like your "drawFocus" parameter even more then this
> deepActiveElement!

Done.

https://codereview.chromium.org/2292583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:352: return;
On 2016/08/31 00:48:03, lushnikov wrote:
> let's do a conventional bail-out
> 
> if (!shiftX && !shiftY)
>     return;

Done.

https://codereview.chromium.org/2292583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:355: var
newAmount;
On 2016/08/31 00:48:03, lushnikov wrote:
> no need to forward-declare

Done.

[lushnikov, 2016-08-31 15:46:33]

https://codereview.chromium.org/2292583002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js (right):

https://codereview.chromium.org/2292583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:88: var
slider = createSliderLabel(0, WebInspector.CSSShadowEditor.maxRange, -1);
probably you wanted maxRange - 1? There's no third argument for
createSliderLabel

https://codereview.chromium.org/2292583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:162:
context.beginPath();
shouldn't this beginPath go under if statement below?

[flandy, 2016-08-31 16:31:27]

https://codereview.chromium.org/2292583002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js (right):

https://codereview.chromium.org/2292583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:88: var
slider = createSliderLabel(0, WebInspector.CSSShadowEditor.maxRange, -1);
On 2016/08/31 15:46:33, lushnikov wrote:
> probably you wanted maxRange - 1? There's no third argument for
> createSliderLabel

No, I added a third argument for tabIndex.

https://codereview.chromium.org/2292583002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js:162:
context.beginPath();
On 2016/08/31 15:46:33, lushnikov wrote:
> shouldn't this beginPath go under if statement below?

Done.

[lushnikov, 2016-08-31 17:47:16]

lgtm

[flandy, 2016-08-31 18:08:53]

The CQ bit was checked by flandy@google.com

[commit-bot: I haz the power, 2016-08-31 18:11:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 19:56:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-31 19:58:24]

Description was changed from

==========
DevTools: Create 2D slider for shadow-editor offset

This replaces the 1D sliders for X offset and Y offset.

BUG=639095
==========

to

==========
DevTools: Create 2D slider for shadow-editor offset

This replaces the 1D sliders for X offset and Y offset.

BUG=639095

Committed: https://crrev.com/3fdf53278760d3691bd7cb04203907f45475018d
Cr-Commit-Position: refs/heads/master@{#415728}
==========

[commit-bot: I haz the power, 2016-08-31 19:58:25]

Patchset 4 (id:??) landed as
https://crrev.com/3fdf53278760d3691bd7cb04203907f45475018d
Cr-Commit-Position: refs/heads/master@{#415728}