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)

All
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: faaborg, Assigned: kliu)

References

Details

(Keywords: polish)

Attachments

(3 files)

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.
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?
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
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.
Attached image a few options (deleted) —
I'm not sure which I prefer.
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)
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.
Attached patch patch (deleted) — Splinter Review
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)
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.
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.
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.
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?
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
(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 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+
(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 on attachment 318135 [details] [diff] [review] patch a1.9+=damons
Attachment #318135 - Flags: approval1.9? → approval1.9+
Whiteboard: [has patch][has review] → [has patch][has review][has approval]
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
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?
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.
>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.
(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.
Should this be better done in a new bug or do you want to recycle this one? I could file a new one.
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.

Attachment

General

Created:
Updated:
Size: