Closed
Bug 348138
Opened 18 years ago
Closed 18 years ago
Go and Search button endcaps do not scale with location/search bars
Categories
(Firefox :: Toolbars and Customization, defect)
Tracking
()
RESOLVED
FIXED
Firefox 2
People
(Reporter: pkasting, Assigned: pamg.bugs)
References
Details
(Keywords: fixed1.8.1, Whiteboard: [Fx2 theme change][would take patch])
Attachments
(4 files, 5 obsolete files)
When using large system fonts (or a userChrome.css rule), the location bar scales (and, once bug 347428 is fixed, the search bar will too). However, the endcap buttons on them do not scale, which looks seriously odd. See https://bugzilla.mozilla.org/attachment.cgi?id=232189 , the screenshot from bug 347428. Is there a way to fix this that's easier than specifying a min-height on the buttons and then breaking them into multiple pieces?
Reporter | ||
Comment 1•18 years ago
|
||
Related, but seemingly not identical bugs: bug 347398 bug 347416 bug 347613 bug 347616 bug 347751
Reporter | ||
Updated•18 years ago
|
Flags: blocking-firefox2?
Target Milestone: --- → Firefox 2 beta2
Reporter | ||
Comment 2•18 years ago
|
||
Nominating for beta 2 on the basis that while this does not block browser usage, is is technically scary to fix. Pam Greene and I spent a while talking about this and we're pretty sure we can fix it nicely (there are a couple different variations on how to do it), but it will take a bit of effort. Conversation with beltzner in IRC leads me to worry that this is the sort of thing we should at least try to get halfway done for B2.
Reporter | ||
Comment 3•18 years ago
|
||
If people specifically want to know how we propose to solve this: There doesn't seem to be a way that's easier than breaking the images into pieces. There are four things we need to tackle: the Go endcap, Search endcap, search engine dropdown, and urlbar non-native dropdown (bug 347616). The search engine dropdown is probably the easiest of these. Most of the rest have roughly the same set of problems; we looked closely at the Go button, so I'll describe the possibilities for it below and assume they apply to the others as well. Fix 1: "As hackish as possible" * We found a single row of white space in the image just above the icon (the "Go" triangle). We could use this strip as a vertically tileable strip to stretch the button: draw top image, tile strip image, draw middle+bottom image. * Pros: requires no image hackery (or, at most, breaking the existing image into separate unchanged pieces), simplest implementation * Cons: gradiant and icon are no longer centered on button. Looks a bit odd at both small AND large stretch values. Fix 2: "More tiling" * Since the gradient on the bottom of the button is subtle, it won't be too hideous for us to tile strips from both the top AND the bottom of the button. * Pros: Keeps icon centered, requires no (or little) image hackery. * Cons: There will be one repeated row near the bottom of the gradiant with an unchanging background, which might look a bit weird, especially if the button is scaled very tall. Harder to implement. Fix 3: "Scaling" * In order to fix the gradiant issues, we could instead pull the icon out of the image, and break the background into three parts: top, middle, and bottom. The top and bottom are drawn as-is; the middle is stretched to fit as necessary; and the icon is then drawn back over the top of everything. * Pros: Keeps icon centered, makes gradiant look a bit better. * Cons: Gradiant still goes from stretched to unstretched section. Even harder to implement due to putting icon back atop the composited background. Fix 4: "Modified images" * To make our lives easier, we could do something like fix #2, but modify the gradiants to be flatter or in some other way look appropriate scaled to any height. This would require new images. * Pros: will look equally good at any scale * Cons: Most image work, may make gradient look worse for all users due to losing "roundedness" I think we should consider doing fix #1 at the very least for B2. I'm probably a proponent of ultimately fixing this using fix #2 or fix #3.
Comment 4•18 years ago
|
||
Definitely need to fix this - and we should aim for the right solution and get that done as fast as possible - but I'm not sure we are going to hold b2 for this...
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: Firefox 2 beta2 → Firefox 2
Updated•18 years ago
|
Whiteboard: [Fx2 theme change]
Assignee | ||
Updated•18 years ago
|
Assignee: nobody → pamg.bugs
Comment 5•18 years ago
|
||
Really, we just need to follow a similar approach to that used by the tabstrip, with a stacked and vertically centered image for the go icon. Not scary, but fixing sooner rather than later is a good thing! More fun with theme XBL!
Assignee | ||
Comment 6•18 years ago
|
||
Here's a winstripe fix, not including the 28 new images. (Yes, really.) The corresponding pinstripe fix and all the images will follow next week.
Assignee | ||
Comment 7•18 years ago
|
||
Reporter | ||
Comment 8•18 years ago
|
||
(In reply to comment #7) > Created an attachment (id=235518) [edit] > Screenshot of fixed Windows buttons That is totally rad. Awesome work, Pam.
Comment 9•18 years ago
|
||
Pam - looks great - eta on patch?
Assignee | ||
Comment 10•18 years ago
|
||
Couple of days for the RTL and Mac work, if the basic approach and level of risk it introduces is acceptable.
Comment 11•18 years ago
|
||
Pam, this is great work, but mconnor's nervous about the depth of change required for the fix. We tested this with "Large Fonts" and "Extra Large Fonts" in Windows, and everything still looks OK with our default size icons. Further, IE7 is breaking the exact same way. I think the best way to continue this fabulous work would be to do so for Firefox 3, and ... - we should provide larger icons as well that scale to fit (64px?) - put the 28 images into a single image and use moz-region We feel badly because this is really awesome work and know this is the second time we've done this to you :(
Flags: blocking-firefox2+ → blocking-firefox2-
Assignee | ||
Comment 12•18 years ago
|
||
(In reply to comment #11) > Pam, this is great work, but mconnor's nervous about the depth of change > required for the fix. I can certainly understand deferring a fix that's too big for this late in the cycle, but I don't yet see why this change qualifies. Since I don't know exactly what aspect of it is scary, I can't tell if it could be modified to mitigate that impact.
Reporter | ||
Comment 13•18 years ago
|
||
(In reply to comment #12) > (In reply to comment #11) > > Pam, this is great work, but mconnor's nervous about the depth of change > > required for the fix. > > I can certainly understand deferring a fix that's too big for this late in the > cycle, but I don't yet see why this change qualifies. Also, if you minus this change, then you're also minusing bug 347616, which would be using this same technique. Please reconsider.
Assignee | ||
Comment 14•18 years ago
|
||
While debate rages on about whether the Go button should be stretchable or removable, or perhaps detached and both, here's a diff -w version (i.e., ignoring whitespace changes) of the above WIP patch so folks can better assess its risk.
Comment 15•18 years ago
|
||
*** Bug 351436 has been marked as a duplicate of this bug. ***
Comment 16•18 years ago
|
||
Comment on attachment 235969 [details] [diff] [review] Same patch (slightly de-bitrotted), no spaces This patch scared me too when I first looked at it, but I've since taken a closer, more careful look, and I don't think it's as scary as it appeared at first glance. Without the images, I can't test it, but the code looks good. I think we should take another look at this for Fx2.
Attachment #235969 -
Flags: superreview?(mconnor)
Attachment #235969 -
Flags: review+
Assignee | ||
Comment 17•18 years ago
|
||
If this is going to be considered again, let me know and I'll make a patch that uses <image> and -moz-image-region rather than background-image and background-position, which would allow all the broken-up images to be recombined into only a couple more than are currently used.
Reporter | ||
Comment 18•18 years ago
|
||
In reply to comment #17) > If this is going to be considered again, let me know and I'll make a patch that > uses <image> and -moz-image-region rather than background-image and > background-position, which would allow all the broken-up images to be > recombined into only a couple more than are currently used. I would really like to re-nominate if you made that patch. I think with myk's r+, a patch that doesn't explode the image count, and bug 347754 comment 33 saying this patch would not preclude us from making the Go button movable/removable, we would have changed the context on the bug enough to reconsider the blocking request.
Assignee | ||
Comment 19•18 years ago
|
||
Okay, here's a more or less completed patch that includes fixes for pinstripe as well as for RTL in winstripe. It recombines all the broken-up images and uses -moz-image-region where possible, reducing the number of new images to 8 (separate icons and backgrounds for two buttons, for LTR and RTL). The "more or less" quality is because it hasn't been tested in Linux. I suspect that will need some additional changes, which could be combined with the Linux fixes for bug 347616 in a followup bug.
Attachment #235517 -
Attachment is obsolete: true
Attachment #235969 -
Attachment is obsolete: true
Attachment #237027 -
Flags: superreview?(mconnor)
Attachment #237027 -
Flags: review?(myk)
Attachment #235969 -
Flags: superreview?(mconnor)
Reporter | ||
Comment 20•18 years ago
|
||
Renominating since the following has changed since the original decision to minus: * The technique here has been successfully used and reviewed to fix bug 347616 * A method has been proposed which makes this bug and bug 347754 not mutually exclusive as we'd initially supposed * The original patch here has been examined and deemed "less scary than it appeared" by Myk, who gave r+ * The patch here has been modified to reduce the number of new images and resulting impact on themers Once again, a large fraction of the size of the patch on this bug is due to whitespace changes; it's not as scary as it seems. Please consider.
Flags: blocking-firefox2- → blocking-firefox2?
Assignee | ||
Comment 21•18 years ago
|
||
Put these in themes/winstripe/browser/.
Assignee | ||
Comment 22•18 years ago
|
||
Here's the same patch, but diff -w (ignoring whitespace) for easier review.
Comment 23•18 years ago
|
||
This wasn't a blocker because the impact really wasn't big enough. Would probably still take the patch, but patch risk was not the deciding factor here.
Flags: blocking-firefox2? → blocking-firefox2-
Whiteboard: [Fx2 theme change] → [Fx2 theme change][would take patch]
Comment 24•18 years ago
|
||
I haven't reviewed the new patch yet, but I tested it on Linux (Ubuntu 6.06 "Dapper Drake"), and it introduces artifacts into the buttons. The Go button has an artifact at the default font size (10px), while the other buttons develop artifacts at 13px. Here's a screenshot of the location and search bars at various font sizes.
Assignee | ||
Comment 25•18 years ago
|
||
(In reply to comment #24) > screenshot showing horkage with patch on linux I'm not surprised. Fixing Linux's CSS is currently in a followup, bug 351618. I expect it's mostly a matter of copying the winstripe css to gnomestripe, but bug 347616 will have to be fixed on Linux too before it really looks reasonable.
Comment 26•18 years ago
|
||
> Fixing Linux's CSS is currently in a followup, bug 351618.
> I expect it's mostly a matter of copying the winstripe css to gnomestripe, but
> bug 347616 will have to be fixed on Linux too before it really looks
> reasonable.
As far as I can tell, Linux uses the CSS in browser/themes/winstripe/ just as Windows does, so changes to the CSS in that directory should impact both Windows and Linux, no? Linux might not use the CSS in toolkit/themes/winstripe/ (or the CSS in toolkit/themes/gnomestripe/ might override it), but you aren't making any changes to toolkit/ in this patch.
So it's unclear to me how Linux will get fixed by copying CSS from winstripe to gnomestripe.
Assignee | ||
Comment 27•18 years ago
|
||
(In reply to comment #26) > So it's unclear to me how Linux will get fixed by copying CSS from winstripe to > gnomestripe. Sorry, that was pure speculation. "Fix the buttons on Linux" is the goal, which I haven't begun looking into at all.
Comment 28•18 years ago
|
||
In the process of reviewing this, here's a version of the last patch which applies cleanly to the tip of the branch.
Attachment #237027 -
Attachment is obsolete: true
Attachment #237027 -
Flags: superreview?(mconnor)
Attachment #237027 -
Flags: review?(myk)
Comment 29•18 years ago
|
||
Comment on attachment 237797 [details] [diff] [review] Pam's last patch updated to branch tip >+ <stack id="go-button-stack"> >+ <vbox> >+ <!-- These image segments allow the button to stretch nicely >+ in larger urlbars. --> >+ <image id="go-button-top" >+ class="go-button-background" >+ chromedir="&locale.dir;" /> >+ <vbox flex="1"> >+ <image flex="1" >+ id="go-button-mid-top" >+ class="go-button-background" >+ chromedir="&locale.dir;" /> >+ <image flex="1" >+ id="go-button-mid-bottom" >+ class="go-button-background" >+ chromedir="&locale.dir;" /> >+ </vbox> >+ <image id="go-button-bottom" >+ class="go-button-background" >+ chromedir="&locale.dir;" /> >+ </vbox> Is the inner vbox actually necessary? It seems like putting the four images one after another inside a single (non-flexible) vbox would have the same effect (the two "mid" images, being flexed, would stretch to fill the extra space). >+.searchbar-engine-image-container { >+ -moz-box-pack: center; >+} Nit: common practice seems to be to specify the default alignment, orientation, and packing of elements via HTML attributes rather than CSS, so you should probably do the same here and elsewhere in the patch. In this particular case, the hbox in search.xml with this class already has align="center". >Index: themes/winstripe/browser/browser.css >+toolbar[mode="text"] #go-button-stack { >+ padding: 0px; >+} >+ >+#go-button-stack { > -moz-margin-end: 5px; >+ padding: 2px 0px 2px 0px; Nit: common practice seems to be to specify zero margin, border, and padding widths as "0" rather than "0px", so you should probably do the same here and elsewhere in the code. Otherwise this looks fine, but sadly, bug 351764 looks neither easy nor low-risk to fix, which means that we'll have to use separate "mid" images instead of image regions on Linux. It'll probably make sense to do the same on Windows too, for simplicity, but we can do that in the Linux followup bug.
Attachment #237797 -
Flags: superreview?(mconnor)
Attachment #237797 -
Flags: review+
Assignee | ||
Comment 30•18 years ago
|
||
(In reply to comment #29) > Is the inner vbox actually necessary? Nope, it's not. I took it out of all three buttons. > Nit: common practice seems to be to specify the default alignment, orientation, > and packing of elements via HTML attributes rather than CSS, so you should > probably do the same here and elsewhere in the patch. That's the only one I could find in the patch. I moved the pack="center" into the binding. It can't be overridden by a theme there, but perhaps that's not important. > Nit: common practice seems to be to specify zero margin, border, and padding > widths as "0" rather than "0px", so you should probably do the same here and > elsewhere in the code. "0px" is a bit more common than "0" in this file, and particularly in these rulesets (i.e., for these three buttons), so I'd prefer to stick to that style. > Otherwise this looks fine, but sadly, bug 351764 looks neither easy nor > low-risk to fix, which means that we'll have to use separate "mid" images > instead of image regions on Linux. It'll probably make sense to do the same on > Windows too, for simplicity, but we can do that in the Linux followup bug. Deferring that to bug 351618, which however will become a [mustfix] if this one lands. But it won't be hard, just a little bit more image clutter.
Attachment #237797 -
Attachment is obsolete: true
Attachment #237842 -
Flags: superreview?(mconnor)
Attachment #237842 -
Flags: approval1.8.1?
Attachment #237797 -
Flags: superreview?(mconnor)
Comment 31•18 years ago
|
||
Comment on attachment 237842 [details] [diff] [review] Fresh patch addressing Myk's comments > That's the only one I could find in the patch. I moved the pack="center" > into the binding. It can't be overridden by a theme there, but perhaps > that's not important. The other one I noticed is: +#urlbar-container { + -moz-box-orient: horizontal; + -moz-box-align: stretch; +}
Attachment #237842 -
Flags: review+
Assignee | ||
Comment 32•18 years ago
|
||
(In reply to comment #31) > +#urlbar-container { > + -moz-box-orient: horizontal; > + -moz-box-align: stretch; > +} Right, I'll get that before checkin.
Reporter | ||
Comment 33•18 years ago
|
||
Since it sounded like mconnor was concerned about this on the phone, this bug does not clash with bug 347754 ; we don't have to pick and choose whether to take a removable Go button vs. a stretchable one.
Comment 34•18 years ago
|
||
(In reply to comment #33) > this bug does not clash with bug 347754 It wouldn't, if we had enough time, but if both bugs are to be fixed tonight... My main nit about the proposed patch is that the new stack to make the Go button flexible is implemented in browser.xul instead of its own binding inside the theme (where it should be contained, since other themes might not need such a hack). As it happens, my solution to bug 347754 requires a new binding for the Go button anyway, so these two patches could be combined. Pam: Would you have time early today to see if we can coordinate these two bugs?
Assignee | ||
Comment 35•18 years ago
|
||
Since I think that a removable Go button will be useful to more people than stretchable buttons, I'm working on bug 347754 first, and will merge this patch afterward.
Comment 36•18 years ago
|
||
Comment on attachment 237842 [details] [diff] [review] Fresh patch addressing Myk's comments ok, I think we should at least fix this, if we can get this all in tonight
Attachment #237842 -
Flags: superreview?(mconnor)
Attachment #237842 -
Flags: superreview+
Attachment #237842 -
Flags: approval1.8.1?
Attachment #237842 -
Flags: approval1.8.1+
Assignee | ||
Comment 37•18 years ago
|
||
Rollup patch landed in big 351618.
Assignee | ||
Updated•18 years ago
|
Attachment #237842 -
Attachment is obsolete: true
Assignee | ||
Comment 38•18 years ago
|
||
Landing rollup patch on 1.8.1 branch.
Comment 39•18 years ago
|
||
Pam, what is this: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/browser/locales/en-US/profile&command=DIFF_FRAMESET&file=localstore.rdf&rev1=1.2.18.4&rev2=1.2.18.5&root=/cvsroot This has been checked in with the patch for this bug. This is a +84 lines change in supposedly frozen localization files!
Assignee | ||
Comment 40•18 years ago
|
||
(In reply to comment #39) > This has been checked in with the patch for this bug. Sorry, that's a file that got changed as a result of bug 299149 and then checked in by mistake. I've reverted it.
You need to log in
before you can comment on or make changes to this bug.
Description
•