[DevTools] Make UI.GlassPane position contentElement for different overlay controls.

[DevTools] Make UI.GlassPane position contentElement for different overlay controls.

Currently used in Dialog and SuggestBox, with SoftContextMenu, Tooltip, Toolbar and Popover to follow.

BUG=none

Review-Url: https://codereview.chromium.org/2658383002
Cr-Commit-Position: refs/heads/master@{#447669}
Committed: https://chromium.googlesource.com/chromium/src/+/ed1f2fac6209f8d2e0db3f7dcff38d1e69f5db26

[dgozman, 2017-01-30 20:36:00]

Description was changed from

==========
[DevTools] Introduce UI.ModalOverlay which handles positioning for different UI
controls.

BUG=none
==========

to

==========
[DevTools] Introduce UI.ModalOverlay which handles positioning for different UI
controls.

Currently used in Dialog and SuggestBox, with SoftContextMenu, Tooltip and
Popover to follow.

BUG=none
==========

[dgozman, 2017-01-30 20:36:05]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 20:36:34]

Dry run: 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, 2017-01-30 20:38:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-30 20:38:43]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dgozman, 2017-01-30 21:44:24]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[dgozman, 2017-01-30 21:44:49]

dgozman@chromium.org changed reviewers:
+ caseq@chromium.org, einbinder@chromium.org, luoe@chromium.org

[dgozman, 2017-01-30 21:44:50]

Could you please take a look?

[commit-bot: I haz the power, 2017-01-30 21:45:23]

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

[einbinder, 2017-01-30 23:59:47]

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js (right):

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/UIUtils.js:323:
UI._glassPaneZIndex -= 1000;
Should we do a check if this.element has a parent here? I'm thinking some weird
bug later where dispose gets called more than once on a GlassPane and the
z-index moves too low.

[commit-bot: I haz the power, 2017-01-31 00:03:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 00:03:06]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2017-01-31 00:39:17]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2017-01-31 00:39:17]

Why isn't it a glass pane?

[dgozman, 2017-01-31 01:35:49]

On 2017/01/31 00:39:17, pfeldman wrote:
> Why isn't it a glass pane?

It has one :-)

[pfeldman, 2017-01-31 18:11:14]

Judging by the name, I was thinking it would be one thing. Glass pane _is_ modal
overlay.

[caseq, 2017-01-31 21:27:20]

lgtm

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/Dialog.js (right):

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:74:
this._overlay.document().body.addEventListener('keydown', this._keyDownBound,
false);
I wonder if we can make it simpler by focusing to the dialog and listening
locally instead?

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:109: if
(this._overlay.visible())
remove?

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:119: if
(this._overlay.visible())
ditto

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:142: if
(this._overlay.visible())
ditto.

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:157: if
(this._overlay.visible())
ditto.

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/Geometry.js (right):

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Geometry.js:353: sizeToMin(size)
{
clip() or clipTo()?

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/ModalOverlay.js (right):

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/ModalOverlay.js:79: if
(!this._visible) {
nit: if (this._visible) return;

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/ModalOverlay.js:109:
addGlassPaneEventListener(eventType, listener, capture) {
should we make the interface simpler by converting this to a callback?

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/ModalOverlay.js:128: var gutter
= 5;
nit: gutterSize or gutterPixels

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/ModalOverlay.js:148: var
rightWidth = containerWidth - anchorBox.x - anchorBox.width - gutter;
nit: Height/Width looks a bit artificial above. Perhaps replace both with
Margin?

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js (right):

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:115: return 300;
please extract to a named constant.

[pfeldman, 2017-01-31 21:30:18]

not lgtm

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/ModalOverlay.js (right):

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/ModalOverlay.js:5:
UI.ModalOverlay = class {
Could you clarify when this is used and how? The mixture of setMaxSize,
setPosition, setAnchorBox methods make it unclear how I am supposed to use this.

[dgozman, 2017-01-31 23:39:29]

Description was changed from

==========
[DevTools] Introduce UI.ModalOverlay which handles positioning for different UI
controls.

Currently used in Dialog and SuggestBox, with SoftContextMenu, Tooltip and
Popover to follow.

BUG=none
==========

to

==========
[DevTools] Make UI.GlassPane position contentElement for different overlay
controls.

Currently used in Dialog and SuggestBox, with SoftContextMenu, Tooltip, Toolbar
and Popover to follow.

BUG=none
==========

[dgozman, 2017-01-31 23:39:49]

PTAL

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/Dialog.js (right):

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:74:
this._overlay.document().body.addEventListener('keydown', this._keyDownBound,
false);
On 2017/01/31 21:27:19, caseq wrote:
> I wonder if we can make it simpler by focusing to the dialog and listening
> locally instead?

Done.

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:109: if
(this._overlay.visible())
On 2017/01/31 21:27:20, caseq wrote:
> remove?

Done.

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/Geometry.js (right):

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Geometry.js:353: sizeToMin(size)
{
On 2017/01/31 21:27:20, caseq wrote:
> clip() or clipTo()?

Done.

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/ModalOverlay.js (right):

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/ModalOverlay.js:5:
UI.ModalOverlay = class {
On 2017/01/31 21:30:18, pfeldman wrote:
> Could you clarify when this is used and how? The mixture of setMaxSize,
> setPosition, setAnchorBox methods make it unclear how I am supposed to use
this.

Updated.

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/ModalOverlay.js:79: if
(!this._visible) {
On 2017/01/31 21:27:20, caseq wrote:
> nit: if (this._visible) return;

Done.

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/ModalOverlay.js:109:
addGlassPaneEventListener(eventType, listener, capture) {
On 2017/01/31 21:27:20, caseq wrote:
> should we make the interface simpler by converting this to a callback?

Simplified.

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/ModalOverlay.js:128: var gutter
= 5;
On 2017/01/31 21:27:20, caseq wrote:
> nit: gutterSize or gutterPixels

Done.

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/ModalOverlay.js:148: var
rightWidth = containerWidth - anchorBox.x - anchorBox.width - gutter;
On 2017/01/31 21:27:20, caseq wrote:
> nit: Height/Width looks a bit artificial above. Perhaps replace both with
> Margin?

Reworked.

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js (right):

https://codereview.chromium.org/2658383002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:115: return 300;
On 2017/01/31 21:27:20, caseq wrote:
> please extract to a named constant.

Done.

[pfeldman, 2017-02-01 00:07:19]

now i like it very much. lgtm

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/Dialog.js (right):

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:82:
this._glassPane.setContentPosition(this._positionX, this._positionY);
setContentPosition/setContentAnchorBox would be mutually exclusive calls that
are positioning content.

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:85:
this._glassPane.element.addEventListener('click', this._onClickBound, false);
this._glassPane.on('exit', () => {})

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js (right):

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:83:
this._glassPane.setAnchorBehavior(UI.GlassPane.AnchorBehavior.PreferTop);
PreferBottom

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:153:
this._glassPane.element.ownerDocument.body.addEventListener('mousedown',
this._maybeHideBound, true);
You don't need it anymore.

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:154:
this._glassPane.element.window().addEventListener('resize', this._hideBound,
false);
You don't need it anymore

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:163:
this._glassPane.element.ownerDocument.body.removeEventListener('mousedown',
this._maybeHideBound, true);
You should not need it anymore.

[dgozman, 2017-02-01 16:19:45]

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/Dialog.js (right):

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:82:
this._glassPane.setContentPosition(this._positionX, this._positionY);
On 2017/02/01 00:07:18, pfeldman wrote:
> setContentPosition/setContentAnchorBox would be mutually exclusive calls that
> are positioning content.

Done.

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:85:
this._glassPane.element.addEventListener('click', this._onClickBound, false);
On 2017/02/01 00:07:18, pfeldman wrote:
> this._glassPane.on('exit', () => {})

Reworked.

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js (right):

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:83:
this._glassPane.setAnchorBehavior(UI.GlassPane.AnchorBehavior.PreferTop);
On 2017/02/01 00:07:19, pfeldman wrote:
> PreferBottom

Done.

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:153:
this._glassPane.element.ownerDocument.body.addEventListener('mousedown',
this._maybeHideBound, true);
On 2017/02/01 00:07:18, pfeldman wrote:
> You don't need it anymore.

Done.

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:154:
this._glassPane.element.window().addEventListener('resize', this._hideBound,
false);
On 2017/02/01 00:07:19, pfeldman wrote:
> You don't need it anymore

Done.

https://codereview.chromium.org/2658383002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/ui/SuggestBox.js:163:
this._glassPane.element.ownerDocument.body.removeEventListener('mousedown',
this._maybeHideBound, true);
On 2017/02/01 00:07:18, pfeldman wrote:
> You should not need it anymore.

Done.

[dgozman, 2017-02-01 16:20:38]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2017-02-01 16:20:39]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2658383002/#ps140001
(title: "review comments addressed")

[commit-bot: I haz the power, 2017-02-01 16:20:50]

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, 2017-02-01 16:22:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 16:22:47]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[dgozman, 2017-02-01 23:21:32]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2017-02-01 23:21:32]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2658383002/#ps160001
(title: "rebased")

[commit-bot: I haz the power, 2017-02-01 23:21:56]

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, 2017-02-02 01:15:03]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485991292091930,
"parent_rev": "09a6a441c91c5a52650c315cc915fe461e58cb33", "commit_rev":
"ed1f2fac6209f8d2e0db3f7dcff38d1e69f5db26"}

[commit-bot: I haz the power, 2017-02-02 01:19:48]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485991292091930,
"parent_rev": "09a6a441c91c5a52650c315cc915fe461e58cb33", "commit_rev":
"ed1f2fac6209f8d2e0db3f7dcff38d1e69f5db26"}

[commit-bot: I haz the power, 2017-02-02 01:19:56]

Description was changed from

==========
[DevTools] Make UI.GlassPane position contentElement for different overlay
controls.

Currently used in Dialog and SuggestBox, with SoftContextMenu, Tooltip, Toolbar
and Popover to follow.

BUG=none
==========

to

==========
[DevTools] Make UI.GlassPane position contentElement for different overlay
controls.

Currently used in Dialog and SuggestBox, with SoftContextMenu, Tooltip, Toolbar
and Popover to follow.

BUG=none

Review-Url: https://codereview.chromium.org/2658383002
Cr-Commit-Position: refs/heads/master@{#447669}
Committed:
https://chromium.googlesource.com/chromium/src/+/ed1f2fac6209f8d2e0db3f7dcff3...
==========

[commit-bot: I haz the power, 2017-02-02 01:19:58]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/ed1f2fac6209f8d2e0db3f7dcff3...