Closed
Bug 181453
Opened 22 years ago
Closed 21 years ago
url box too close to personal toolbar in modern theme
Categories
(SeaMonkey :: Themes, defect)
SeaMonkey
Themes
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: peppe, Assigned: shliang)
References
Details
(Keywords: fixed1.4.1, regression)
Attachments
(4 files, 3 obsolete files)
41.45 KB,
image/png
|
Details | |
555 bytes,
patch
|
janv
:
review+
|
Details | Diff | Splinter Review |
714 bytes,
patch
|
shliang
:
review+
jag+mozilla
:
superreview+
mkaply
:
approval1.4.1+
|
Details | Diff | Splinter Review |
10.15 KB,
image/png
|
Details |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1) Gecko/20020826 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1) Gecko/20020826 Ever since the toolbar collapsing buttons were removed from the left side of the toolbars, the url box has been too low and too close to the personal toolbar in the modern theme. Reproducible: Always Steps to Reproduce: 1. Make sure the modern theme is used 2. Open a browser window 3. Look at the URL input field and the box around it and notice its vertical alignment 4. Notice how the url box and input field are not vertically centered in the navigation toolbar but rather they are aligned to the bottom of the toolbar and much too close to the personal toolbar below it
Comment 1•22 years ago
|
||
While the URL bar is no longer vertically centered, it is not yet close enough to the personal toolbar, and the space above it needs to be reduced to match. See bug 135457 and the bugs it depends on, all of which are liable to be moot when fix for bug 15144 lands.
Reporter | ||
Comment 2•22 years ago
|
||
Bug 135457 represents your personal subjective opinion about the toolbars in general whereas this bug is reporting an unvoluntary change in the looks of the toolbar in one specific theme (Modern). This is about restoring visual esthetics and correctness and can be fixed regardless of 135457. Bug 15144 may not be fixed for quite some time since it's pretty major and bug 135457 may never be fixed. This bug however, is a very simple matter of vertically centering a user interface control.
Comment 3•22 years ago
|
||
I'm seeing this on Linux. It happens for both 1.2.1 and 1.3b. I don't have other versions to compare with (and for some reason mozilla's ftp server isn't responding right now so I can't download another).
Comment 4•21 years ago
|
||
I don't know how to submit a patch for this, but I do know how to fix it. In skin/modern/navigator/navigator.css (modern.jar), line 382, attribute for #nav-bar-inner: margin: 4px 0px 3px 5px; The existing values are |7px 0px 0px 5px|, so there is no bottom margin and the top margin is a bit big. The new values result in an aesthetically pleasing display.
Comment 5•21 years ago
|
||
Vertically aligns the urlbar so that it is centered on the Navigation Toolbar.
Updated•21 years ago
|
Attachment #124118 -
Flags: review?
Comment 6•21 years ago
|
||
The attached screenshot shows the original (mozilla 1.0) appearance, the current (mozilla 1.3) appearance, and that of the 'margin' patch. The patch restores the spacing between the URL bar and the edge of the panel, but does not fix the corresponding spacing w.r.t. the back button nor the throbber. In particular, the tops of these items no longer line up with the URL bar. I suspect the problem arose not from someone tweaking navigator.css, but from a bug introduced or fixed in the code that interprets it. But if it can be fixed from within navigator.css then all's well that ends well.
Updated•21 years ago
|
Attachment #124118 -
Flags: review? → review?(neil.parkwaycc.co.uk)
Updated•21 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•21 years ago
|
Attachment #124118 -
Flags: review?(neil.parkwaycc.co.uk) → review-
Comment 7•21 years ago
|
||
I removed the 45px minimum height on primary toolbars, which allowed the toolbar to shrink to 40px to "fit" the urlbar; this patch adds some margin which should restore the previous height.
Attachment #124118 -
Attachment is obsolete: true
Attachment #124736 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #124815 -
Flags: superreview?(jaggernaut)
Attachment #124815 -
Flags: review?(varga)
Comment 8•21 years ago
|
||
Comment on attachment 124815 [details] [diff] [review] My fix because I broke it :-) looks good, tested on linux
Attachment #124815 -
Flags: review?(varga) → review+
Comment 9•21 years ago
|
||
This is an improvement on attachment 124815 [details] [diff] [review] that I believe more accurately reproduces the mozilla-1.0 behavior. In the next comment I'll explain more.
Comment 10•21 years ago
|
||
Attachment 124886 [details] [diff] is the result of tweaking navigator.css to get the pixel distances between the various UI components to match those of mozilla-1.0. I think this is important because the previously proposed patches still have some elements in different positions than what the theme designer intended. I also happen to think they look better in their original positions. To validate this, I measured the distances between UI elements, counting only the solid gray pixels that come from the panel background pixmap. You can compare these counts to the images in Attachment 124736 [details]. Below are those measurements. I apologize for the length of this comment, but I wanted to move away from the imprecise "looks ok to me" approach, since it's subjective. This is what I found: Version A: mozilla-1.0: summary: this is the original layout of the nav bar in the modern theme, as far as I'm aware, and the goal is to get mozilla to once again have this layout back button: top: 6 bottom: 6 (does not include nearly-gray from pixmap) down-arrow near back button: top: 29 bottom: 3 throbber: top: 6 bottom: 3 menu/nav divider 6 (pixels between elements) urlbox top edge 3 urlbar text widget 3 urlbox bottom edge 3 nav/personal divider Version B: mozilla-1.3: summary: like Version A, but the entire nav bar is 3px shorter, with slack entirely taken from the bottom margins except for the throbber, which took 1px from top and 2px from bottom back button: top: 6 bottom: 3 down-arrow: top: 29 bottom: 0 throbber: top: 5 bottom: 1 menu/nav divider 6 urlbox top edge 3 urlbar text widget 3 urlbox bottom edge 0 nav/personal divider Version C: mozilla-1.3, nav-bar-inner margin "4px 0px 3px 5px" (this is Attachment 124118 [details] [diff]) summary: like Version B, but urlbox is moved 3px higher back button: top: 6 bottom: 3 down-arrow: top: 29 bottom: 0 throbber: top: 5 bottom: 1 menu/nav divider 3 urlbox top edge 3 urlbar text widget 3 urlbox bottom edge 3 nav/personal divider Version D: mozilla-1.3, nav-bar-inner margin "7px 0px 5px 5px" (this is Attachment 124815 [details] [diff]) summary: like Version A, but: - down-arrow is shifted down 2px - throbber is shifted down 1px - urlbar text widget's margin top & bottom margins reduced by 1px, slack given to the urlbox bottom margin back button: top: 6 bottom: 6 down-arrow: top: 31 bottom: 1 throbber: top: 7 bottom: 2 menu/nav divider 6 urlbox top edge 2 urlbar text widget 2 urlbox bottom edge 5 nav/personal divider Version E: mozilla-1.3, with: #nav-bar-buttons margin "0px 3px 3px 0px" #nav-bar-inner margin "7px 0px 3px 5px" #throbber-box margin "0px 0px 2px 0px" (this is my Attachment 124886 [details] [diff]) measurements are identical to version A!
Comment 11•21 years ago
|
||
From Attachement 124886:
> #nav-bar-buttons {
>- margin-left: 3px;
>+ margin: 0px 3px 3px 0px;
Shouldn't this be
+ margin: 0px 0px 3px 3px;
That is, the margin-left is mantained, rather than shifted to margin-right?
ps. OS/hardware should be all/all
Comment 12•21 years ago
|
||
Yes, tryandguessit is right. Included is a fixed patch. (For some reason bugzilla isn't listing the old patch as being among those I can make obsolete, oh well...)
Updated•21 years ago
|
Attachment #124886 -
Attachment is obsolete: true
Attachment #124886 -
Attachment is patch: true
Updated•21 years ago
|
OS: Windows 2000 → All
Hardware: PC → All
Updated•21 years ago
|
Keywords: nsbeta1,
regression
Attachment #124969 -
Flags: review+
Updated•21 years ago
|
Attachment #124969 -
Flags: superreview+
Attachment #124969 -
Flags: approval1.4?
Comment 13•21 years ago
|
||
Actually, do we really need this change: #nav-bar-buttons { - margin-left: 3px; + margin: 0px 0px 3px 3px; } That part makes the toolbar three pixels taller. Is there another way to center the buttons / throbber better?
Comment 14•21 years ago
|
||
It looks like we could take a pixel or two from the top.
Comment 15•21 years ago
|
||
Comment on attachment 124969 [details] [diff] [review] Yet another improved patch Cancelling sr= for now
Attachment #124969 -
Flags: superreview?(jaggernaut)
Attachment #124969 -
Flags: superreview+
Attachment #124969 -
Flags: approval1.4?
Comment 16•21 years ago
|
||
According to my measurements, and as shown in the attached screenshot, if I remove the change to "nav-bar-inner" then we have: Version F: same as E but removing the nav-bar-buttons change: summary: nav bar is two pixels shorter than in Version A, with the down-arrow much too close (0px) to the bottom edge back button: top: 6 bottom: 4 down-arrow: top: 30 bottom: 0 throbber: top: 5 bottom: 2 nav/menu divider 6 urlbox top edge 2 urlbar text widget 2 urlbox bottom edge 3 nav/personal divider My goal was to get the navbar to have the same inter-element spacings as in mozilla-1.0, since I prefer it that way and that was how the theme designer had it. But even if we want it 2px shorter than in 1.0, the back button's little down-arrow should (IMO) not be touching the bottom edge.
Comment 17•21 years ago
|
||
>if I remove the change to "nav-bar-inner" then we have:
I meant "nav-bar-buttons" here.
Comment 18•21 years ago
|
||
Comment on attachment 124969 [details] [diff] [review] Yet another improved patch Scott: understood. I was wondering if we made the toolbar a little less high on purpose (to save some vertical space, every pixel counts), and if that's the case, could we leave it at the current height, and just move all the buttons up a little so the little arrow on back/forward/print doesn't touch the bottom of the toolbar? However, since you have r=shliang, sr=jag.
Attachment #124969 -
Flags: superreview?(jaggernaut) → superreview+
Comment 19•21 years ago
|
||
This bug is tricky because it involves people's subjective judgment about aesthetics; one person's pleasing space is another's wasted pixels. Obviously, alternative themes are inteded to address the demand for tighter UI layouts, but alternative themes usually don't get the same QA that the default shipping themes do. So I understand the tendency to opportunistically improve the default themes. But I'd point out that if I were the original theme designer, having spent time getting it to look the way I want and attracting a following of happy users in the process, I'd be skeptical about some engineers "fixing" my theme just because they were already changing related code to solve another problem. We (engineers) often have a poor sense of how a UI will be perceived by users (case in point: I failed to notice the reversed margin around nav-bar-buttons, due to focusing on vertical spacing). Therefore, in the absence of additional input from the theme designer, I think the first priority is to do our best to restore the original design. It shows respect for the design and designer, and thereby encourages non-engineers to continue to contribute. We can then consider preferences like those expressed in Bug 135457 as a separate issue, perhaps soliciting input from theme designers for that process. But in that vein, consider that Bug 135457 has 0 votes, while Bug 15144 (which addresses horizontal spacing) has 59.
Comment 21•21 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 22•21 years ago
|
||
Comment on attachment 124969 [details] [diff] [review] Yet another improved patch polish up the browser. I don't think it's a high risk patch.
Attachment #124969 -
Flags: approval1.4?
Updated•21 years ago
|
QA Contact: pmac → gbush
Updated•21 years ago
|
Attachment #124815 -
Flags: superreview?(jaggernaut)
Comment 24•21 years ago
|
||
Comment on attachment 124969 [details] [diff] [review] Yet another improved patch moving approval request forward.
Attachment #124969 -
Flags: approval1.4? → approval1.4.x?
Comment 25•21 years ago
|
||
*** Bug 175170 has been marked as a duplicate of this bug. ***
Comment 26•21 years ago
|
||
Comment on attachment 124969 [details] [diff] [review] Yet another improved patch a=mkaply
Attachment #124969 -
Flags: approval1.4.x? → approval1.4.x+
Comment 27•21 years ago
|
||
Please add the fixed1.4.1 keyword when this is checked in.
Flags: blocking1.4.x+
Keywords: fixed1.4.1
Comment 28•21 years ago
|
||
fixed on 1.4.x branch.
Updated•16 years ago
|
Product: Core → SeaMonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•