The page layout is broken in Trunk, on the URL provided. ( http://blogs.computerworld.com/recycle_electronics_free_by_mail ) OBSERVED BEHAVIOR (shown in Trunk, 2008031704 nightly) - Navigation panel ("Home", "News", "E-mail Newsletters", etc.) is in the center of the page, with the article contents shifted down below it. EXPECTED BEHAVIOR (shown by FF2): - Navigation panel aligned on left side of page, with article contents to its right.

(In reply to comment #0) > OBSERVED BEHAVIOR (shown in Trunk, 2008031704 nightly) > - Navigation panel ("Home", "News", "E-mail Newsletters", etc.) is in the > center of the page, with the article contents shifted down below it. Also, the article contents + comments are shifted too far to the left, overlapping the gray stripe. (as compared to being on top of the white area of the page in FF2.)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031722 Minefield/3.0b5pre Page how it is displayed on my screen: http://img101.imageshack.us/img101/1999/naamloosay5.png

Sorry Daniel, for some reason I misread that it was filed for Linux, probably Linux only.

This is also worksforme here, using current trunk build on windows.

This regressed just 2 days ago, between 2008031604 and 2008031704. Bonsai link: http://tinyurl.com/yq6usk Bonsai link for just layout directory: http://tinyurl.com/263otf It looks like Bug 421632 was the only non-trivial layout checkin (all the others seem to just add/modify reftests). Martijn and/or Ria, could you retest making sure you're using a build from yesterday or today? I did leave this marked "Linux" when filing 'cause I hadn't tested Windows, but I didn't actually expect that it'd be a Linux-only bug :)

Hmm. My Linux tree with the fix for bug 421632 isn't showing this problem. And more importantly, that fix only changed the areas we invalidate. It should not have affected layout at all. Could the native theming changes be the issue here? I tried applying them locally, and I still don't see the bug, but it could depend on the exact GTK theme...

(In reply to comment #6) > Could the native theming changes be the issue here? I tried applying them > locally, and I still don't see the bug, but it could depend on the exact GTK > theme... Ah, yes -- it looks like the "Jump to [More Resources]" combobox is taller in the broken builds. This would explain why / confirm that this is Linux-only.

Ah, yeah. If that's too tall and oveflows the 28px height available there, then the other floats have to move either down or right. Ideally, this site would have a clear on that sidebar float to avoid problems like this, but oh well. Note that this means that the bug will also depend on exact font metrics, etc.

Here's an attachment demonstrating the change -- namely, that comboboxes are now 4px taller, due to increased border-width. Pre-bug 415830 (using ff3b4), I get these values: Height of select: 19px Top Border: 3px Bottom Border: 3px Total Combined Height: 25px And in latest trunk, I get these values: Height of select: 19px Top Border: 5px Bottom Border: 5px Total Combined Height: 29px

(In reply to comment #9) > Created an attachment (id=310325) [details] Here are the values on Firefox 2: Height of select: 19px Top Border: 2px Bottom Border: 2px Total Combined Height: 23px So, current trunk makes comboboxes 6px taller than in FF2, which is a pretty huge increase, and may break other web pages as well... I don't know much about our GTK widget code, but is there any chance we could roll back our borderWidth values to 3px instead of 5px?

(In reply to comment #9) > Created an attachment (id=310325) [details] Here are the values on Mac OS X, using FF3b4, for a cross-platform comparison: Height of select: 16px Top Border: 2px Bottom Border: 2px Total Combined Height: 20px So comboboxes are almost 50% taller in Linux, as compared to Mac... that seems hard for web developers to code for.

For what it's worth, the non-native-themed implementation made sure that comboboxes and textboxes were the same height cross-platform, and the same height as IE-windows as much as possible, to prevent just such bustage... I don't know whether there's something sane native theme code can do here to not have the huge 10px space around the text, but if there is imo we should.

This should block B5 as it is a big regression content-wise. I have a patch.

The fix here is to not count the focus padding as the actual border, considering forms.css adds stuff later. This was what we did with the deprecated optionmenu, and what we do with buttons in HTML mode.

dbaron: see comment 13, and please let me know if this blocks. If not, set the TM to mozilla1.9.

Yes - let's get this fix for b5.

I'm not sure that this is the right fix. The modified "calculate_button_inner _rect" function is also used for the editable menulist dropdown button (moz_gtk_combo_box_entry_button_paint). Perhaps we should override the forms.css padding, either by CSS (setting it to 0 and keeping the border as is now), or perhaps preferably by forcing the padding of comboboxes to be the focus padding, with nsNativeThemeGTK::GetWidgetPadding(). Are there really uses cases where websites rely on the possibility to set comboboxes' padding ?

Also note that a solution which wouldn't change comboboxes size in XUL would be greater, since in XUL the code makes the combobox exactly the right size (with the right padding). Perhaps the problem really comes from forms.css. Is it overridable for gnomestripe only, preferably in a way that would be inherited by third-party skins ?

> Are there really uses cases Since this is the web, the answer is probably yes. If we can't handle that, we should drop native theming when they do.

I tried to understand what rules are responsible for the padding in forms.css but didn't succeed in finding a big padding responsible for the oversize. Is the padding applied to an inner frame of the select ?

Checking in widget/src/gtk2/gtk2drawing.c; /cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v <-- gtk2drawing.c new revision: 1.100; previous revision: 1.99 done

This checkin makes comboboxes smaller than buttons (height-wise), both in XUL and HTML.

In fact, it makes the combobox padding be less than the button padding. For HTML at least, it doesn't seem to directly translate to smaller comboboxes, since no <select> of bugzilla search page seem to obey the default font size. So if I choose a default font size small enough in preferences, I can get buttons with the same height, or even smaller, than the comboboxes. I think, but am not sure, that for HTML the heights match roughly when the font sizes match. So what we want is a mean to keep the focus_padding in XUL at least. Not keeping it in HTML is OK since such a special casing is already done for buttons for instance.

This patch makes native theming stuff use full padding when in XUL, and reduced (as with the previous patch) padding when in HTML. The rendering is also adapted (or else the text could run into the separator). Note that this code always add the combobox button border width even on HTML, to mirror what our button code does. (I think it's 0 on a lot of themes who advertise their border only with x/ythickness, in any case, but I did it for concistency).

Comment on attachment 310777 [details] [diff] [review] Patch which reverts the focus ignore, but only for XUL (so that in HTML comboboxes aren't too big) roc's out for a while... vlad, can you take a look?

Comment on attachment 310777 [details] [diff] [review] Patch which reverts the focus ignore, but only for XUL (so that in HTML comboboxes aren't too big) sr=me, but can you create a reftest for this stuff?

Yeah, need a test. Approval pending reftest.

(In reply to comment #26) > (From update of attachment 310777 [details] [diff] [review]) > sr=me, but can you create a reftest for this stuff? You mean like something under layout/reftests/native-theme/? I see no form of reftests under widget/ at all.

I thought reftests were comparing the rendering of something ? Here we can only check if the width/height are sane... Sure we could probably make a hack with an hidden combobox and a visible thing next to it with reliable rendering, but I'm not sure we really want to impose pixel precision on the size of a combobox.

layout/reftests/native-theme would work, yes. > Sure we could probably make a hack with an hidden combobox and a visible thing > next to it with reliable rendering Yep. No need for "next to" either: just put a visibility:hidden combobox in a div, and give the div a background. > I'm not sure we really want to impose pixel precision on the size of a > combobox. We don't want to do that, no. But it should be possible to test that a combobox is not "too much bigger" than, say, a one-line textfield, right? Ideally they'd actually be the same height, like they used to be, but I'm pretty sure that GTK screws this up in general so our native theme code might not be able to do it.

Has anyone decided the best way to make a reftest, or even if we're going to do reftests? This needs to land soon...

You are going to do a reftest.

Ventnor: any progress on that reftest?

So we would put an hidden combobox into a div with an explicit width/height, and if the combobox is small enough, the rendering will be the same as the reference with only the div and no inner combobox ? Or with another object in the div (for both the reference and the test) which would enable us to compare the combobox width/height to an other widget instead of an explicit width ? By the way, do we want to compare the height, or the width and the height ?

I can't seem to recreate this problem on my linux box this morning, so this test should be suspect. But, from reading the comments, I think this might be something like what is wanted. This compares the combobox height with the height of a textbox (per Bz's comment up above). Since it looks like these heights are not the same with native theming (I'm eyeballing it atm), we might want to do some height manipulation. Anyway, I hope it helps to get ideas flowing at least. Let me know if I can help out.

(In reply to comment #35) > I can't seem to recreate this problem on my linux box this morning ctalbert: FWIW, the original issue on this bug was fixed by the checkin in comment 21, so that issue is indeed no longer visible in trunk builds.

Comment on attachment 311415 [details] [diff] [review] An idea for a test ctalbert: Well done -- that reftest does indeed fail on my machine, using the 2008031704 linux nightly build (which includes the bug), but it passes in a current nightly. The reftest essentially asserts that a combobox has the same height as a one-line textfield, as Boris was essentially suggesting in comment 30. r=dholbert on this test, pending confirmation that it passes on Windows/Mac

Comment on attachment 310777 [details] [diff] [review] Patch which reverts the focus ignore, but only for XUL (so that in HTML comboboxes aren't too big) Requesting approval now that there's a reftest to make sure this doesn't get regressed. This fixes issues that were only half-fixed by the original patch.

(In reply to comment #37) > (From update of attachment 311415 [details] [diff] [review]) > ctalbert: Well done -- that reftest does indeed fail on my machine, using the > 2008031704 linux nightly build (which includes the bug), but it passes in a > current nightly. > I completely missed comment 21. Doh. > The reftest essentially asserts that a combobox has the same height as a > one-line textfield, as Boris was essentially suggesting in comment 30. > Right, that was what I was using for inspiration. > r=dholbert on this test, pending confirmation that it passes on Windows/Mac > Thanks, but I don't think we're quite there yet. It fails on mac.

It might be that what we really want to assert is that the combobox is no taller than, say, textbox plus 2px. Or 4px. Or some such. How much taller was it when this bug was originally filed? How do the heights compare on Mac and Windows?

(In reply to comment #38) > (From update of attachment 310777 [details] [diff] [review]) > Requesting approval now that there's a reftest to make sure this doesn't get > regressed. This fixes issues that were only half-fixed by the original patch. > I'll wait for approval from bz that the tests are good enough..

Here's an improved version of the reftest. It works much like the original broken computerworld.com page -- basically, it asserts that a native-select isn't much taller than an unthemed select. (where "isn't much taller" = at most, 4px taller.) I've posted the test files at http://people.mozilla.com/~dholbert/tests/423599/reftest/

Comment on attachment 311443 [details] [diff] [review] improved reftest (In reply to comment #41) > I'll wait for approval from bz that the tests are good enough.. K, requesting approval from bz on new test. (You can try out the test via the URL in comment 42)

Btw, I relocated the reftest to the "bugs" directory, from the "native-theme" directory. I did this because the tests in "native-theme" appear to all fall into two categories: -- Tests that assert(form control != about:blank) -- Tests that "will fail if the platform does not have native theme support" (source: http://mxr.mozilla.org/seamonkey/source/layout/reftests/native-theme/reftest.list ) ... and this reftest doesn't fall into either of those categories. So, I figure it fits best in the catch-all "bugs" directory, as it's a regression test for this bug's fix, plus that's where most reftests end up anyway. :)

> it asserts that a native-select isn't much taller than an unthemed select. Wouldn a simpler way to assert that be something like: Test: <div style="background: blue"> <select class="unthemed" style="margin: 2px; visibility: hidden"></select> <select class="native" style="visibility: hidden"></select> </div> Reference: <div style="background: blue"> <select class="unthemed" style="margin: 2px; visibility: hidden"></select> </div>

Updated reftest per comment 45. I confirmed that this passes on Mac using FF3b4, passes on Linux using 2008032104 nightly (non-buggy), and fails on linux using 2008031904 (buggy)

Comment on attachment 311464 [details] [diff] [review] more-improved reftest d'oh, attached wrong patch, one sec...

Comment on attachment 310777 [details] [diff] [review] Patch which reverts the focus ignore, but only for XUL (so that in HTML comboboxes aren't too big) (Still waiting on the reftest before checking this in)

See reftest in action at: http://people.mozilla.com/~dholbert/tests/423599/reftest/new/ Only difference between reference case and test case is the "<select></select>" line.

Comment on attachment 311467 [details] [diff] [review] even-more-improved reftest Looks good.

Comment on attachment 310777 [details] [diff] [review] Patch which reverts the focus ignore, but only for XUL (so that in HTML comboboxes aren't too big) a1.9b5=beltzner along with reftest

Checked in reftest (attachment 310777 [details] [diff] [review]) and "Patch which reverts the focus ignore" (attachment 310777 [details] [diff] [review]). RCS file: /cvsroot/mozilla/layout/reftests/bugs/423599-1-ref.html,v done Checking in layout/reftests/bugs/423599-1-ref.html; /cvsroot/mozilla/layout/reftests/bugs/423599-1-ref.html,v <-- 423599-1-ref.htm l initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/423599-1.html,v done Checking in layout/reftests/bugs/423599-1.html; /cvsroot/mozilla/layout/reftests/bugs/423599-1.html,v <-- 423599-1.html initial revision: 1.1 done Checking in layout/reftests/bugs/reftest.list; /cvsroot/mozilla/layout/reftests/bugs/reftest.list,v <-- reftest.list new revision: 1.409; previous revision: 1.408 done Checking in toolkit/themes/gnomestripe/global/menulist.css; /cvsroot/mozilla/toolkit/themes/gnomestripe/global/menulist.css,v <-- menulist.css new revision: 1.9; previous revision: 1.8 done Checking in widget/src/gtk2/gtk2drawing.c; /cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v <-- gtk2drawing.c new revision: 1.102; previous revision: 1.101 done Checking in widget/src/gtk2/nsNativeThemeGTK.cpp; /cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v <-- nsNativeThemeGTK.cpp new revision: 1.156; previous revision: 1.155 done Re-closing bug.

The reftest bugs/423599-1.html fails if the browser default font is changed, either to a font such as Charis SIL that has larger ascent/descent metrics, or to a bigger pixel size. This affects the height of the themed <select> in the test case, but not the unthemed one in the reference. Is this expected behavior?

For an unthemed select, we use: font: -moz-list; from forms.css. Do we use some other font for themed select? If so, that sounds pretty odd....

Ha! The issue isn't the size of the <select> after all.... it's the space between the two <select>s in the testcase, which gets its height from the browser's default font, and thus messes up the height of the <div> if the font is tall.

This patch simply removes the space between the <select>s in the reftest, so that it doesn't break with large default fonts.

Comment on attachment 401883 [details] [diff] [review] eliminate space from reftest to make it more reliable Bonus points for replacing the space with <!-- No space to make this more robust in the face of different default fonts --> or some such.

pushed (with bonus points) :) http://hg.mozilla.org/mozilla-central/rev/440ef24cd6b3