136268 2014-08-26 16:19:58 -0700 Web Inspector: Layout issues for popover on not legacy OS 2014-08-27 13:19:49 -0700 1 1 1 Unclassified WebKit Web Inspector 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 1 saam saam commit-queue graouts joepeck timothy webkit-bug-importer oldest_to_newest 1031508 0 saam 2014-08-26 16:19:58 -0700 There is a layout issue where a popover will show up under the title bar in the Safari webpage for the Web Inspector on not legacy OSs. There might also be other places where this is a problem too. I'll look into it. 1031509 1 webkit-bug-importer 2014-08-26 16:21:13 -0700 <rdar://problem/18141133> 1031700 2 saam 2014-08-27 11:32:50 -0700 I don't think there are any other problems with innerHeight in the code, because the other uses rely on some minimum/maximum size that will never conflict with the extra 22 pixels we don't want to touch on the title bar. here are the other places: ## Main.js (I don't think there is a problem with this, but I do notice some behavior where the console's frame might quickly jump when dragging it, but I think it only happens when resizing a window and has to do with maximally being 55% of the height) function dockedResizerDrag(event) { if (event.button !== 0) return; var height = window.innerHeight - event.pageY - mouseOffset; this._splitConsoleHeightSetting.value = height; this._updateSplitConsoleHeight(height); } WebInspector._updateSplitConsoleHeight = function(height) { const minimumHeight = 64; const maximumHeight = window.innerHeight * 0.55; height = Math.max(minimumHeight, Math.min(height, maximumHeight)); this.splitContentBrowser.element.style.height = height + "px"; } ## CompletionSuggestionsView.js (In my testing, this is never large enough in the smallest window I can shrink the inspector to, to go over the title bar) var aboveHeight = anchorBounds.origin.y; var underHeight = window.innerHeight - anchorBounds.origin.y - anchorBounds.size.height; var maximumHeight = Math.min(absoluteMaximumHeight, Math.max(underHeight, aboveHeight) - margin); var height = Math.min(containerHeight, maximumHeight); ## QuickConsole.js (I'm not actually sure what this is, but I think that it maximally being 33% of innerHeight will prevent any problems w/ covering the title bar) updateLayout: function() { // A hard maximum size of 33% of the window. const maximumAllowedHeight = Math.round(window.innerHeight * 0.33); this.prompt.element.style.maxHeight = maximumAllowedHeight + "px"; }, 1031706 3 237235 saam 2014-08-27 11:44:34 -0700 Created attachment 237235 patch Solve the problem by adding a padding of 22 for newer OS. 1031709 4 237235 joepeck 2014-08-27 11:52:37 -0700 Comment on attachment 237235 patch View in context: https://bugs.webkit.org/attachment.cgi?id=237235&action=review > Source/WebInspectorUI/UserInterface/Views/Popover.js:200 > + var padding = (WebInspector.Platform.isLegacyMacOS ? 0 : 22); The condition needs to check if this is mac: var shouldAddTopWindowPadding = WebInspector.Platform.name === "mac" && !WebInspector.Platform.isLegacyMacOS; Otherwise windows and linux ports would add this 22px of padding unnecessarily. And maybe a better word is "offset" instead of padding. 1031718 5 237236 saam 2014-08-27 12:15:43 -0700 Created attachment 237236 patch 1031724 6 237236 timothy 2014-08-27 12:20:38 -0700 Comment on attachment 237236 patch View in context: https://bugs.webkit.org/attachment.cgi?id=237236&action=review > Source/WebInspectorUI/UserInterface/Views/Popover.js:199 > + const titleBarOffset = (WebInspector.Platform.name === "mac" && !WebInspector.Platform.isLegacyMacOS ? 22 : 0); Could be written as: const titleBarOffset = WebInspector.Platform.isLegacyMacOS ? 0 : 22; Drop the () too. 1031726 7 237236 joepeck 2014-08-27 12:26:12 -0700 Comment on attachment 237236 patch View in context: https://bugs.webkit.org/attachment.cgi?id=237236&action=review >> Source/WebInspectorUI/UserInterface/Views/Popover.js:199 >> + const titleBarOffset = (WebInspector.Platform.name === "mac" && !WebInspector.Platform.isLegacyMacOS ? 22 : 0); > > Could be written as: > > const titleBarOffset = WebInspector.Platform.isLegacyMacOS ? 0 : 22; > > Drop the () too. I disagree. I think we still need to check Platform.name === "mac" to ensure we don't add unnecessary padding on linux and windows. 1031727 8 237236 timothy 2014-08-27 12:27:53 -0700 Comment on attachment 237236 patch View in context: https://bugs.webkit.org/attachment.cgi?id=237236&action=review >>> Source/WebInspectorUI/UserInterface/Views/Popover.js:199 >>> + const titleBarOffset = (WebInspector.Platform.name === "mac" && !WebInspector.Platform.isLegacyMacOS ? 22 : 0); >> >> Could be written as: >> >> const titleBarOffset = WebInspector.Platform.isLegacyMacOS ? 0 : 22; >> >> Drop the () too. > > I disagree. I think we still need to check Platform.name === "mac" to ensure we don't add unnecessary padding on linux and windows. WebInspector.Platform.isLegacyMacOS is only true for Mac. It is in the property name. We do this check in many other places. 1031728 9 237236 timothy 2014-08-27 12:28:28 -0700 Comment on attachment 237236 patch View in context: https://bugs.webkit.org/attachment.cgi?id=237236&action=review >>>> Source/WebInspectorUI/UserInterface/Views/Popover.js:199 >>>> + const titleBarOffset = (WebInspector.Platform.name === "mac" && !WebInspector.Platform.isLegacyMacOS ? 22 : 0); >>> >>> Could be written as: >>> >>> const titleBarOffset = WebInspector.Platform.isLegacyMacOS ? 0 : 22; >>> >>> Drop the () too. >> >> I disagree. I think we still need to check Platform.name === "mac" to ensure we don't add unnecessary padding on linux and windows. > > WebInspector.Platform.isLegacyMacOS is only true for Mac. It is in the property name. We do this check in many other places. Note: I flipped the order. I agree !WebInspector.Platform.isLegacyMacOS is wrong. 1031729 10 saam 2014-08-27 12:30:41 -0700 (In reply to comment #8) > (From update of attachment 237236 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=237236&action=review > > >>> Source/WebInspectorUI/UserInterface/Views/Popover.js:199 > >>> + const titleBarOffset = (WebInspector.Platform.name === "mac" && !WebInspector.Platform.isLegacyMacOS ? 22 : 0); > >> > >> Could be written as: > >> > >> const titleBarOffset = WebInspector.Platform.isLegacyMacOS ? 0 : 22; > >> > >> Drop the () too. > > > > I disagree. I think we still need to check Platform.name === "mac" to ensure we don't add unnecessary padding on linux and windows. > > WebInspector.Platform.isLegacyMacOS is only true for Mac. It is in the property name. We do this check in many other places. Right, so on linux/windows isLegacyMacOS will be false, and we would add the padding for a UI we're not intending to. 1031733 11 237238 saam 2014-08-27 12:33:56 -0700 Created attachment 237238 patch 1031752 12 237238 commit-queue 2014-08-27 13:19:46 -0700 Comment on attachment 237238 patch Clearing flags on attachment: 237238 Committed r173015: <http://trac.webkit.org/changeset/173015> 1031753 13 commit-queue 2014-08-27 13:19:49 -0700 All reviewed patches have been landed. Closing bug. 237235 2014-08-27 11:44:34 -0700 2014-08-27 12:15:43 -0700 patch patch.diff text/plain 1726 saam SW5kZXg6IFNvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwkocmV2aXNpb24gMTczMDA4KQorKysgU291cmNl L1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDE0LTA4LTI3ICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAg IFdlYiBJbnNwZWN0b3I6IExheW91dCBpc3N1ZXMgZm9yIHBvcG92ZXIgb24gbm90IGxlZ2FjeSBP UworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTM2MjY4 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgT24gdGhl IGxhdGVzdCBPUywgcG9wb3ZlcnMgaW52YWRlIHRoZSB0aXRsZSBiYXIncyBzcGFjZSB3aGljaCB3 aWxsCisgICAgICAgIGNhdXNlIGxheW91dCBpc3N1ZXMgYmVjYXVzZSB0aGUgdGl0bGUgYmFyIHdp bGwgcmVuZGVyIGFib3ZlCisgICAgICAgIHRoZSBwb3BvdmVyLiBGaXggdGhpcyBieSBwcm92aWRp bmcgc29tZSBwYWRkaW5nIGZvciBwb3BvdmVycyBvbiAKKyAgICAgICAgbm90IGxlZ2FjeSBPU3Mu CisKKyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1ZpZXdzL1BvcG92ZXIuanM6CisKIDIwMTQtMDgt MjYgIFNhYW0gQmFyYXRpICA8c2JhcmF0aUBhcHBsZS5jb20+CiAKICAgICAgICAgV2ViIEluc3Bl Y3RvcjogRml4IENvZGVNaXJyb3JUb2tlblRyYWNraW5nQ29udHJvbGxlcjo6X3N0YXJ0VHJhY2tp bmcgYXNzZXJ0aW9uCkluZGV4OiBTb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9W aWV3cy9Qb3BvdmVyLmpzCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2Vy SW50ZXJmYWNlL1ZpZXdzL1BvcG92ZXIuanMJKHJldmlzaW9uIDE3MzAwOCkKKysrIFNvdXJjZS9X ZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL1BvcG92ZXIuanMJKHdvcmtpbmcgY29w eSkKQEAgLTE5Nyw3ICsxOTcsOCBAQCBXZWJJbnNwZWN0b3IuUG9wb3Zlci5wcm90b3R5cGUgPSB7 CiAgICAgICAgIH0KIAogICAgICAgICAvLyBUaGUgZnJhbWUgb2YgdGhlIHdpbmRvdyB3aXRoIGEg bGl0dGxlIGluc2V0IHRvIG1ha2Ugc3VyZSB3ZSBoYXZlIHJvb20gZm9yIHNoYWRvd3MuCi0gICAg ICAgIHZhciBjb250YWluZXJGcmFtZSA9IG5ldyBXZWJJbnNwZWN0b3IuUmVjdCgwLCAwLCB3aW5k b3cuaW5uZXJXaWR0aCwgd2luZG93LmlubmVySGVpZ2h0KTsKKyAgICAgICAgdmFyIHBhZGRpbmcg PSAoV2ViSW5zcGVjdG9yLlBsYXRmb3JtLmlzTGVnYWN5TWFjT1MgPyAwIDogMjIpOworICAgICAg ICB2YXIgY29udGFpbmVyRnJhbWUgPSBuZXcgV2ViSW5zcGVjdG9yLlJlY3QoMCwgcGFkZGluZywg d2luZG93LmlubmVyV2lkdGgsIHdpbmRvdy5pbm5lckhlaWdodCAtIHBhZGRpbmcpOwogICAgICAg ICBjb250YWluZXJGcmFtZSA9IGNvbnRhaW5lckZyYW1lLmluc2V0KFdlYkluc3BlY3Rvci5Qb3Bv dmVyLlNoYWRvd0VkZ2VJbnNldHMpOwogCiAgICAgICAgIC8vIFdvcmsgb3V0IHRoZSBtZXRyaWNz IGZvciBhbGwgZWRnZXMuCg== 237236 2014-08-27 12:15:43 -0700 2014-08-27 12:33:56 -0700 patch patch.diff text/plain 1913 saam SW5kZXg6IFNvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwkocmV2aXNpb24gMTczMDA4KQorKysgU291cmNl L1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDE0LTA4LTI3ICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAg IFdlYiBJbnNwZWN0b3I6IExheW91dCBpc3N1ZXMgZm9yIHBvcG92ZXIgb24gbm90IGxlZ2FjeSBP UworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTM2MjY4 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgT24gdGhl IGxhdGVzdCBPUywgcG9wb3ZlcnMgaW52YWRlIHRoZSB0aXRsZSBiYXIncyBzcGFjZSB3aGljaCB3 aWxsCisgICAgICAgIGNhdXNlIGxheW91dCBpc3N1ZXMgYmVjYXVzZSB0aGUgdGl0bGUgYmFyIHdp bGwgcmVuZGVyIGFib3ZlCisgICAgICAgIHRoZSBwb3BvdmVyLiBGaXggdGhpcyBieSBwcm92aWRp bmcgc29tZSBwYWRkaW5nIGZvciBwb3BvdmVycyBvbiAKKyAgICAgICAgbm90IGxlZ2FjeSBPU3Mu CisKKyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1ZpZXdzL1BvcG92ZXIuanM6CisKIDIwMTQtMDgt MjYgIFNhYW0gQmFyYXRpICA8c2JhcmF0aUBhcHBsZS5jb20+CiAKICAgICAgICAgV2ViIEluc3Bl Y3RvcjogRml4IENvZGVNaXJyb3JUb2tlblRyYWNraW5nQ29udHJvbGxlcjo6X3N0YXJ0VHJhY2tp bmcgYXNzZXJ0aW9uCkluZGV4OiBTb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9W aWV3cy9Qb3BvdmVyLmpzCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2Vy SW50ZXJmYWNlL1ZpZXdzL1BvcG92ZXIuanMJKHJldmlzaW9uIDE3MzAwOCkKKysrIFNvdXJjZS9X ZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL1BvcG92ZXIuanMJKHdvcmtpbmcgY29w eSkKQEAgLTE5Niw4ICsxOTYsOSBAQCBXZWJJbnNwZWN0b3IuUG9wb3Zlci5wcm90b3R5cGUgPSB7 CiAgICAgICAgICAgICB0aGlzLl9wcmVmZXJyZWRTaXplID0gbmV3IFdlYkluc3BlY3Rvci5TaXpl KE1hdGguY2VpbChwb3BvdmVyQm91bmRzLndpZHRoKSwgTWF0aC5jZWlsKHBvcG92ZXJCb3VuZHMu aGVpZ2h0KSk7CiAgICAgICAgIH0KIAorICAgICAgICBjb25zdCB0aXRsZUJhck9mZnNldCA9IChX ZWJJbnNwZWN0b3IuUGxhdGZvcm0ubmFtZSA9PT0gIm1hYyIgJiYgIVdlYkluc3BlY3Rvci5QbGF0 Zm9ybS5pc0xlZ2FjeU1hY09TID8gMjIgOiAwKTsKKyAgICAgICAgdmFyIGNvbnRhaW5lckZyYW1l ID0gbmV3IFdlYkluc3BlY3Rvci5SZWN0KDAsIHRpdGxlQmFyT2Zmc2V0LCB3aW5kb3cuaW5uZXJX aWR0aCwgd2luZG93LmlubmVySGVpZ2h0IC0gdGl0bGVCYXJPZmZzZXQpOwogICAgICAgICAvLyBU aGUgZnJhbWUgb2YgdGhlIHdpbmRvdyB3aXRoIGEgbGl0dGxlIGluc2V0IHRvIG1ha2Ugc3VyZSB3 ZSBoYXZlIHJvb20gZm9yIHNoYWRvd3MuCi0gICAgICAgIHZhciBjb250YWluZXJGcmFtZSA9IG5l dyBXZWJJbnNwZWN0b3IuUmVjdCgwLCAwLCB3aW5kb3cuaW5uZXJXaWR0aCwgd2luZG93LmlubmVy SGVpZ2h0KTsKICAgICAgICAgY29udGFpbmVyRnJhbWUgPSBjb250YWluZXJGcmFtZS5pbnNldChX ZWJJbnNwZWN0b3IuUG9wb3Zlci5TaGFkb3dFZGdlSW5zZXRzKTsKIAogICAgICAgICAvLyBXb3Jr IG91dCB0aGUgbWV0cmljcyBmb3IgYWxsIGVkZ2VzLgo= 237238 2014-08-27 12:33:56 -0700 2014-08-27 13:19:45 -0700 patch patch.diff text/plain 1911 saam SW5kZXg6IFNvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwkocmV2aXNpb24gMTczMDA4KQorKysgU291cmNl L1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDE0LTA4LTI3ICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAg IFdlYiBJbnNwZWN0b3I6IExheW91dCBpc3N1ZXMgZm9yIHBvcG92ZXIgb24gbm90IGxlZ2FjeSBP UworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTM2MjY4 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgT24gdGhl IGxhdGVzdCBPUywgcG9wb3ZlcnMgaW52YWRlIHRoZSB0aXRsZSBiYXIncyBzcGFjZSB3aGljaCB3 aWxsCisgICAgICAgIGNhdXNlIGxheW91dCBpc3N1ZXMgYmVjYXVzZSB0aGUgdGl0bGUgYmFyIHdp bGwgcmVuZGVyIGFib3ZlCisgICAgICAgIHRoZSBwb3BvdmVyLiBGaXggdGhpcyBieSBwcm92aWRp bmcgc29tZSBwYWRkaW5nIGZvciBwb3BvdmVycyBvbiAKKyAgICAgICAgbm90IGxlZ2FjeSBPU3Mu CisKKyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1ZpZXdzL1BvcG92ZXIuanM6CisKIDIwMTQtMDgt MjYgIFNhYW0gQmFyYXRpICA8c2JhcmF0aUBhcHBsZS5jb20+CiAKICAgICAgICAgV2ViIEluc3Bl Y3RvcjogRml4IENvZGVNaXJyb3JUb2tlblRyYWNraW5nQ29udHJvbGxlcjo6X3N0YXJ0VHJhY2tp bmcgYXNzZXJ0aW9uCkluZGV4OiBTb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9W aWV3cy9Qb3BvdmVyLmpzCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2Vy SW50ZXJmYWNlL1ZpZXdzL1BvcG92ZXIuanMJKHJldmlzaW9uIDE3MzAwOCkKKysrIFNvdXJjZS9X ZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL1BvcG92ZXIuanMJKHdvcmtpbmcgY29w eSkKQEAgLTE5Niw4ICsxOTYsOSBAQCBXZWJJbnNwZWN0b3IuUG9wb3Zlci5wcm90b3R5cGUgPSB7 CiAgICAgICAgICAgICB0aGlzLl9wcmVmZXJyZWRTaXplID0gbmV3IFdlYkluc3BlY3Rvci5TaXpl KE1hdGguY2VpbChwb3BvdmVyQm91bmRzLndpZHRoKSwgTWF0aC5jZWlsKHBvcG92ZXJCb3VuZHMu aGVpZ2h0KSk7CiAgICAgICAgIH0KIAorICAgICAgICBjb25zdCB0aXRsZUJhck9mZnNldCA9IFdl Ykluc3BlY3Rvci5QbGF0Zm9ybS5uYW1lID09PSAibWFjIiAmJiAhV2ViSW5zcGVjdG9yLlBsYXRm b3JtLmlzTGVnYWN5TWFjT1MgPyAyMiA6IDA7CisgICAgICAgIHZhciBjb250YWluZXJGcmFtZSA9 IG5ldyBXZWJJbnNwZWN0b3IuUmVjdCgwLCB0aXRsZUJhck9mZnNldCwgd2luZG93LmlubmVyV2lk dGgsIHdpbmRvdy5pbm5lckhlaWdodCAtIHRpdGxlQmFyT2Zmc2V0KTsKICAgICAgICAgLy8gVGhl IGZyYW1lIG9mIHRoZSB3aW5kb3cgd2l0aCBhIGxpdHRsZSBpbnNldCB0byBtYWtlIHN1cmUgd2Ug aGF2ZSByb29tIGZvciBzaGFkb3dzLgotICAgICAgICB2YXIgY29udGFpbmVyRnJhbWUgPSBuZXcg V2ViSW5zcGVjdG9yLlJlY3QoMCwgMCwgd2luZG93LmlubmVyV2lkdGgsIHdpbmRvdy5pbm5lckhl aWdodCk7CiAgICAgICAgIGNvbnRhaW5lckZyYW1lID0gY29udGFpbmVyRnJhbWUuaW5zZXQoV2Vi SW5zcGVjdG9yLlBvcG92ZXIuU2hhZG93RWRnZUluc2V0cyk7CiAKICAgICAgICAgLy8gV29yayBv dXQgdGhlIG1ldHJpY3MgZm9yIGFsbCBlZGdlcy4K