Bug 29961 - Position of "Close" button in docked mode doesn't match the position of window "Close" button in detached mode
Summary: Position of "Close" button in docked mode doesn't match the position of windo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-01 07:36 PDT by Mikhail Naganov
Modified: 2009-10-05 09:03 PDT (History)
2 users (show)

See Also:


Attachments
propsed change (2.94 KB, patch)
2009-10-01 07:44 PDT, Mikhail Naganov
timothy: review-
Details | Formatted Diff | Diff
[IMAGE] close to the right (don't pay attention to shifted search field - i have an old and strange WebKit build) (222.40 KB, image/png)
2009-10-01 07:59 PDT, Pavel Feldman
no flags Details
Screenshot with Safari 4.0.3 and WebKit 48959 (36.47 KB, image/png)
2009-10-01 08:49 PDT, Mikhail Naganov
no flags Details
update patch (3.85 KB, patch)
2009-10-01 08:55 PDT, Mikhail Naganov
timothy: review-
Details | Formatted Diff | Diff
removed substring matching (4.00 KB, patch)
2009-10-01 12:51 PDT, Mikhail Naganov
timothy: review+
timothy: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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