Closed
Bug 430714
Opened 17 years ago
Closed 17 years ago
Move the identity contextual dialog up to avoid double line
Categories
(Firefox :: Theme, defect)
Tracking
()
VERIFIED
FIXED
Firefox 3
People
(Reporter: faaborg, Assigned: kliu)
References
Details
(Keywords: polish)
Attachments
(3 files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Gavin
:
review+
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
In the attached screen shot the bookmark contextual dialog is placed three pixels higher than the identity contextual dialog. This placement has the advantage of not having a double line appearance on the top.
Comment 1•17 years ago
|
||
The popup opening code takes care of positioning the popup relative to the identity box:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.1029&mark=6679,6680#6656
We can probably fix this by using the favicon to anchor the popup instead?
Comment 2•17 years ago
|
||
You can move popups relative to the anchor by specifying X and Y after the position
http://developer.mozilla.org/en/docs/XUL:Method:openPopup
We should be able to specify Y of -3
Comment 3•17 years ago
|
||
Ah, yeah. -1 for the y offset works fine, but because the top left pixel of the popup is white (due to -moz-appearance: tooltip styling), it's impossible to line it up with the button border without that annoying pixel showing up.
Comment 4•17 years ago
|
||
I'm not sure which I prefer.
Comment 5•17 years ago
|
||
the last one hides it nicely on the default button, but it would be noticeable on SSL. The second one seems to be the least noticeable otherwise because there's no dark point of reference.
It's too bad we turn off the drop shadow when you specify -moz-border-radius, otherwise you could just chop the spare pixels off with that (including that one that sticks out on the bottom right)
Reporter | ||
Comment 6•17 years ago
|
||
Good call on the last one hiding the white pixel, let's go with that.
Actually, it's not being hidden on the site identity button. It's still on the bottom border; it's more of an optical illusion that it appears to be on the button and not the button border.
Also, if we go with making the address bar native for Aero (are you still toying with that idea?), then the white pixel would be moot because the pixel row immediately below the site button would be a white gap and not the border.
Shift up by 1px to get rid of the double border.
Shift right by 1px in Aero as requested by comment 6.
Assignee: nobody → kliu.bugzilla.3c9f
Status: NEW → ASSIGNED
Attachment #318135 -
Flags: review?(gavin.sharp)
Comment 9•17 years ago
|
||
We should avoid using the margin to do this, and instead use the offset parameters of openPopup in code, so that we don't need to keep the margin's in sync.
Assignee | ||
Comment 10•17 years ago
|
||
I agree, but I also think that this is a special case.
With respect to the vertical offset, the patch I posted will be undoing and getting rid of a 1px margin offset that was added by bug 414698. The "natural" position of the Larry dialog when anchored to the site identity button with no offset (either from CSS or from openPopup) has no double border and it's what we want.
With respect to the horizontal offset, anchoring to the site identity button poses a problem in that the start margin of the site identity button is negative and varies between Classic, Luna, and Aero, so there must be a matching offset in CSS (as we cannot make those distinctions in JS), and AFAIK, the 1px rightward shift only applies to Aero as the other appearances do not have the rounded border issue.
Assignee | ||
Comment 11•17 years ago
|
||
FWIW, if the Larry button and the Larry panel both had no margins at all, the desired appearance referred to by comment 6 would be achieved with a 0,0 offset in openPopup.
Since the deviation from the desired appearance was the result of margins set in the theme, it makes sense that the deviation is resolved by margins set in the theme, instead of making a change to the JS, which would affect all other themes.
Comment 12•17 years ago
|
||
I think I'd rather "break" those themes now than have to forever carry the cost of maintaining these CSS hacks. Are there really that many themes that are already styling Larry?
Comment 13•17 years ago
|
||
I agree with Kai, the top margin that's currently in the stylesheet isn't right. Removing it will fix this bug.
As for the horizontal margin, I think he's right that this is a special case for the curve and the popup styling on Vista/Aero. We don't want that offset elsewhere.
Summary: Move the identity contextual dialog up three pixels to avoid double line → Move the identity contextual dialog up to avoid double line
Assignee | ||
Comment 14•17 years ago
|
||
(In reply to comment #12)
> forever carry the cost of maintaining these CSS hacks.
>
I agree. And that's why I'm standing by my proposal. :) My proposal isn't to add more CSS hacks to solve the problem, but rather, it is to undo some of the existing CSS hacks to fix the problem. Since 0,0 results in the desired appearance if there were no CSS hacks present, changing the offset in JS would be using a JS hack to fix a CSS hack.
Aside from third-party themes, a JS change would also "break" pinstripe and maybe gnomestripe too.
Comment 15•17 years ago
|
||
Comment on attachment 318135 [details] [diff] [review]
patch
Ah, yes, you're both right. Thanks for explaining.
>Index: browser/themes/winstripe/browser/browser-aero.css
>-/* Bug 413060, comment 14: Match #identity-box's -moz-margin-start, less 1px */
>+/* Match #identity-box[chromedir="ltr"]'s -moz-margin-start */
> #identity-popup[chromedir="ltr"]:-moz-system-metric(windows-default-theme) {
>- -moz-margin-start: 5px;
>+ -moz-margin-start: 6px;
This means we're picking the last option from attachment 317604 [details], right?
Attachment #318135 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 16•17 years ago
|
||
(In reply to comment #15)
> This means we're picking the last option from attachment 317604 [details], right?
>
Yes.
Whiteboard: [has patch][has review]
Attachment #318135 -
Flags: approval1.9?
Comment 17•17 years ago
|
||
Comment on attachment 318135 [details] [diff] [review]
patch
a1.9+=damons
Attachment #318135 -
Flags: approval1.9? → approval1.9+
Keywords: checkin-needed,
polish
Whiteboard: [has patch][has review] → [has patch][has review][has approval]
Comment 18•17 years ago
|
||
mozilla/browser/themes/winstripe/browser/browser-aero.css 1.11
mozilla/browser/themes/winstripe/browser/browser.css 1.213
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has approval]
Target Milestone: --- → Firefox 3
Comment 19•17 years ago
|
||
With Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050206 Minefield/3.0pre ID:2008050206 the identity contextual dialog is moved to a higher position. But it is still positioned lower as the bookmarks contextual dialog.
Alex, shouldn't both contextual dialogs be positioned at the same height?
Comment 20•17 years ago
|
||
The identity button is the anchor for the identity panel, and the star is the anchor for the bookmark panel. But the identity button and the star have different heights, so that's kinda expected.
Reporter | ||
Comment 21•17 years ago
|
||
>Alex, shouldn't both contextual dialogs be positioned at the same height?
Yeah, I think they should be positioned at the same height. Dao's point about the items that produce the panels being at different heights is correct, but I think that distinction is subtle enough that if they are at different heights it will look like a mistake.
Assignee | ||
Comment 22•17 years ago
|
||
(In reply to comment #21)
> Yeah, I think they should be positioned at the same height.
>
It could be done, but it would be fragile, as it would require hard-coding the distance from the bottom of the star icon to the bottom of the address bar.
Comment 23•17 years ago
|
||
Should this be better done in a new bug or do you want to recycle this one? I could file a new one.
Comment 24•17 years ago
|
||
No double line anymore. Marking verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050920 Minefield/3.0pre
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•