Bug 29961

Summary: Position of "Close" button in docked mode doesn't match the position of window "Close" button in detached mode
Product: WebKit Reporter: Mikhail Naganov <mnaganov>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: pfeldman, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows Vista   
Attachments:
Description Flags
propsed change
timothy: review-
[IMAGE] close to the right (don't pay attention to shifted search field - i have an old and strange WebKit build)
none
Screenshot with Safari 4.0.3 and WebKit 48959
none
update patch
timothy: review-
removed substring matching timothy: review+, timothy: commit-queue-

Description Mikhail Naganov 2009-10-01 07:36:37 PDT
Currently on Windows "Close" button is on the left side in docked mode, but the "Close" button of a window in detached mode is on the right side. Let's place "Close" button in docked mode on the same side as the window "Close" button in detached mode.
Comment 1 Mikhail Naganov 2009-10-01 07:44:46 PDT
Created attachment 40440 [details]
propsed change
Comment 2 Timothy Hatcher 2009-10-01 07:50:55 PDT
Can you attach a screenshot?
Comment 3 Timothy Hatcher 2009-10-01 07:53:19 PDT
Comment on attachment 40440 [details]
propsed change



> +    // Place "Close" button in docked mode on the same side as the window "Close" button in detached mode.
> +    var closeElement;
> +    if (InspectorController.platform().substr(0, 3) == "mac") {
> +        closeElement = document.getElementById("close-left");
> +    } else {
> +        closeElement = document.getElementById("close-right");
> +    }
> +    var closeButton = document.createElement("button");
> +    closeButton.id = "close-button";
> +    closeButton.addEventListener("click", this.close, true);
> +    closeElement.appendChild(closeButton);

You can do more this in CSS. Using the .platfor-mac and .platform-win classes.

So attach the listener to both elements and let CSS decided what one to show and hide. And have the button in both.
Comment 4 Pavel Feldman 2009-10-01 07:59:59 PDT
Created attachment 40441 [details]
[IMAGE] close to the right (don't pay attention to shifted search field - i have an old and strange WebKit build)
Comment 5 Mikhail Naganov 2009-10-01 08:49:38 PDT
Created attachment 40446 [details]
Screenshot with Safari 4.0.3 and WebKit 48959 

I've implemented button positioning using CSS. Here is a screenshot of the latest Safari + WebKit with the patch that I'll attach in a few minutes.
Comment 6 Mikhail Naganov 2009-10-01 08:55:56 PDT
Created attachment 40447 [details]
update patch
Comment 7 Timothy Hatcher 2009-10-01 09:06:17 PDT
Comment on attachment 40447 [details]
update patch


> +body.attached.platform-qt .toolbar-item[class*="close-"] {

It would be better and faster for the engine to write this as:

body.attached.platform-qt .toolbar-item.close-right, body.attached.platform-qt .toolbar-item.close-left

Also more correct if we ever add somthing like "enclose-foo".

> +body[class*="platform-mac"] .toolbar-item.close-right { 

Should be: body.platform-mac .toolbar-item.close-right


> +body:not([class*="platform-mac"]) .toolbar-item.close-left { 

Should be: body:not(.platform-mac) .toolbar-item.close-left
Comment 8 Mikhail Naganov 2009-10-01 12:51:41 PDT
Created attachment 40471 [details]
removed substring matching

For mac Inspector actually has "mac-tiger" and "mac-leopard", so my initial intent was to merge these two cases into one (and also accept future possibilities, like "mac-snow-leopard"). But if you insist on not using substring matches, here is a plain version.

Doing a negation on more than one class selector isn't intuitive. I've done it according to this W3C test: http://www.w3.org/Style/CSS/Test/CSS3/Selectors/current/html/full/flat/css3-modsel-14e.html

I've checked that it works on Windows, and that after swapping these two :not's button isn't get hidden, too.
Comment 9 Pavel Feldman 2009-10-05 09:03:28 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/inspector.html
	M	WebCore/inspector/front-end/inspector.js
Committed r49097