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)

2.0 Branch
x86
All
defect
Not set
normal

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?
Related, but seemingly not identical bugs:
bug 347398
bug 347416
bug 347613
bug 347616
bug 347751
Flags: blocking-firefox2?
Target Milestone: --- → Firefox 2 beta2
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.
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.
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
Whiteboard: [Fx2 theme change]
Assignee: nobody → pamg.bugs
Blocks: 347616
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!
Attached patch WIP patch, winstripe only so far (obsolete) — Splinter Review
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.
(In reply to comment #7)
> Created an attachment (id=235518) [edit]
> Screenshot of fixed Windows buttons

That is totally rad.  Awesome work, Pam.
Pam - looks great - eta on patch?
Couple of days for the RTL and Mac work, if the basic approach and level of risk it introduces is acceptable.
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-
(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.
(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.
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.
*** Bug 351436 has been marked as a duplicate of this bug. ***
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+
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.
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.
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)
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?
Attached file Images for testing
Put these in themes/winstripe/browser/.
Here's the same patch, but diff -w (ignoring whitespace) for easier review.
Blocks: 351618
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]
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.
(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.
> 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.
(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.
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 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+
(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 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+
(In reply to comment #31)

> +#urlbar-container {
> +  -moz-box-orient: horizontal;
> +  -moz-box-align: stretch;
> +}

Right, I'll get that before checkin.

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.
(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?
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 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+
Rollup patch landed in big 351618.
Attachment #237842 - Attachment is obsolete: true
Landing rollup patch on 1.8.1 branch.
Status: NEW → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
(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.

Attachment

General

Creator:
Created:
Updated:
Size: