The Quick Add event dialog has been broken in Minefield as long as I can remember moving to trunk nightlies (so after Cairo) and I'd been told that there was already a bug on file, but can't find one, so filing this for coverage. STR: 1. Get a Zimbra account. 2. Login to your Zimbra account. 3. Go to the "Calendar" tab. 4. Drag out a new event on the calendar to open the "Quick Add" dialog. 5. Look at the start/stop time fields Expected: See entry fields for month and date, a drop down for a calendar widget, and then drop-downs for time Actual: the drop-down widgets for time are missing Works on branch as expected. (screenshots attached)

Need a regression range.

re: regression range - this has been around at least as long as the initial cocoa widget focus bug. Nominating for blocking, as I don't know how many sites it affects. Sam: any word on a testcase?

Moving to bloking and setting P2

This broke on Mac between 2006120706 and 2006120806. Regression range: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-12-07+06&maxdate=2006-12-08+06&cvsroot=%2Fcvsroot Go go gadget reflow. Note that in build 2006120806, this is way worse and somewhere along the way we improved it, but it's still not fixed. Creating at testcase for Zimbra has been really hard. The mess of code/ajax isn't fun to debug, but I can work on that more. Since Zimbra is used by Comcast(.net), which is a top500 site, adding that keyword. Moving to Core::Layout even though this doesn't yet have a testcase.

This testcase demonstrates the issue. I think there might be two things at play, but I haven't created the second testcase yet. The first is that the background of the <table> is full width on trunk but not on branch. That's this testcase and may or may not be an actual bug. The second, which may or may not truly exist (I haven't reduced it yet) is that it appears the background of this <table> overlaps other elements in later <table>s. I'm not sure if that should be happening (or even if it's what *is* happening), but I'll reduce it and update this bug with my findings.

Samuel: Can you attach an image of what your testcase is supposed to look like? (from a branch run, perhaps?) Thanks.

Here's the second testcase I was working on. This is probably the same bug despite what I said above. This testcase might be easier to work with.

The testcases here don't seem like bug 403708; do we have our signals crossed somewhere?

Brian: The testcases here are highly reduced from what exists in Zimbra. If you want an un-reduced testcase, I'll gladly create one. ;)

It's not that they're small, it's just that the interfaces seem to be using entirely different elements. (not that I've looked at Zimbra's code specifically yet) Thanks for the quick reply, though.

(In reply to comment #14) > it's just that the interfaces seem to be using entirely different elements. > (not that I've looked at Zimbra's code specifically yet) It's not. Zimbra is using <table>, <td>, and <div> elements with borders and backgrounds (lots of CSS) to achieve the look and style.

Gonna take a crack at this.

Here's a third testcase, based on Testcase 2. The first cell in the outer table contains a 10%-wide fixed-layout table, which doesn't actually take up much horizontal space. Yet, for some reason, it makes the second cell's contents hidden. The second cell contains a div with the "overflow: hidden" attribute, so I guess this entire cell is being treated as overflowed, and that's why it's not visible...? I don't know why it'd be overflowed, though, because there's plenty of space for it. FWIW, the settings 'overflow: scroll' and 'overflow: auto' show the bug as well (and look exactly the same as the 'overflow: hidden' case.)

FWIW, here's how IE7 handles Testcase 3. Trunk differs from this by having blue nothingness at the spot where IE7 has the tiny yellow "bar" cell.

This Testcase 4 is similar to Testcase 3, but with overall width fixed to 100px, and with a background-color (orange) added for the first cell. This demonstrates (if it weren't already obvious) that the first cell takes up the full width of the table in Trunk.

Here's what happens when we render Testcase 4 in Trunk: 1. Compute column intrinsic widths: a) First cell: min = 0, pref = nscoord_MAX ** The prefCoord is nscoord_MAX because FixedTableLayoutStrategy::GetPrefWidth always returns that. b) Second cell: min = 0, pref = 11px (the width of the text) ** The minWidth is 0px because all "overflow" variants make the contents get wrapped in a nsHTMLScrollFrame, and nsHTMLScrollFrame::GetMinWidth() always returns 0. 2. Compute column widths, in BasicTableLayoutStrategy::ComputeColumnWidths a) Both columns start at their min width (0px), with 600px of spare width to be divided among them b) The first column absorbs all 600px of spare width because its prefWidth is nscoord_MAX c) This leaves the second column still at its min-width of 0px 3. Inner table computes its *real* size a) The inner table has "width: 10%", so now that we know it's cell 600px wide, we assign the inner table to be 10% of that == 60px This leaves us with the final result: - First cell is 600px wide - Inner table (in first cell) is 60px wide - Second cell is 0px wide This all seems correct to me. IE7 / WebKit both differ from us, though (giving the second cell enough width to hold its text). So maybe I'm missing something.

(In reply to comment #20) > ** The minWidth is 0px because all "overflow" variants make the contents > get wrapped in a nsHTMLScrollFrame, and nsHTMLScrollFrame::GetMinWidth() always > returns 0. That may not be true in other browsers (but I think it is what we've done in the past; I recall trying to fix it and running into other problems related to a separate IE quirk that's much harder for us to implement or that we don't want to implement).

> > nsHTMLScrollFrame::GetMinWidth() always > > returns 0. > > That may not be true in other browsers Right you are, as shown by this testcase. Here's the browser breakdown on this issue, FWIW: FF2, FF3, Opera: ScrollFrame min-width = 0 IE7, WebKit: ScrollFrame min-width = its contents' min-width

This patch makes nsHTMLScrollFrame::GetMinWidth() return the min-width of the scrolled frame. (matching IE / Konqueror, as described in my prev. comment.) This patch... - passes reftests - fixes the Zimbra problem described in comment 0 - makes us match IE7 and WebKit on Testcase2, Testcase3, Testcase4, and 'scrollframe min-width demo'. (and we already matched them on the first Testcase) Not 100% sure this is correct, though... dbaron mentioned trying to do something like this in the past and running into problems -- do you remember what the problems were and/or what sorts of things I should test?

Note: We still differ from Branch on most of these testcases, but that's a good thing -- it's because Branch's fixed table layout implementation isn't entirely correct, and that's been fixed in Trunk.

The last time I fixed this, the regression I got (filed against the reflow branch) was bug 359510. See bug 359510 and bug 309110 (of which this is a duplicate) for analysis.

... of which this patch is a duplicate, at least -- not necessarily the entire bug report.

(In reply to comment #25) > The last time I fixed this, the regression I got (filed against the reflow > branch) was bug 359510. This change doesn't cause that particular regression anymore -- gmail looks the same after the patch. (even with long contact names) Maybe gmail has improved their code. Or maybe something else was involved on our end that's now been separately fixed. > bug 309110 (of which this is a > duplicate) for analysis. From the discussion towards the end of that bug, it sounds like we probably *should* make this change, e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=309110#c34 and https://bugzilla.mozilla.org/show_bug.cgi?id=309110#c36 However -- this change does break these testcases, as roc suggests in https://bugzilla.mozilla.org/show_bug.cgi?id=309110#c18 - bug 295459 -- most of the testcases, e.g. attachment 184490 [details] - bug 292690 -- just testcase9 (attachment 184443 [details]) I haven't looked into those testcases in detail, so I'm not sure yet how important they are or how easy it'd be to fix them while still adding this change.

I don't really know what the impact on web compat will be, but if Webkit and IE7 are passing through the min-width then I guess we should just bite the bullet and do it for the sake of broad compatibility, even if some sites serving browser-specific content break.

Comment on attachment 289541 [details] [diff] [review] fix v1 (Note: ignore the commented-out "nscoord result = 0;" in this patch -- I'll be removing that.) Is there anything else obvious that I'd need to do here, or should fix v1 be sufficient? (I'm not particularly familiar with the intricacies of our scrollframe implementation, so I don't know if there's something else obvious that I'm missing.)

You should do what GetPrefWidth does and include the width of a scrollbar if necessary. Preferably by sharing code.

Adding a doctype declaration to the scrollframe min-width demo (as in this testcase) shows that IE does indeed add extra min-width for the scrollbars, in their standards mode. WebKit doesn't allocate this extra min-width, but that just makes them look broken, because the scrollbars end up crammed inside of a space where they don't fit, on top of the scrolled frame's contents.

This patch adds the scrollbar width if necessary. This makes us match IE on "scrollframe min-width demo, standards mode" (attachment 289743 [details]), showing scrollbars on the "overflow: scroll" div. In the quirks-mode (no-doctype) version, IE hides their scrollbars even when overflow: scroll is set. We don't match that quirk, but I think that's a good thing.

Comment on attachment 289747 [details] [diff] [review] fix v2 + if (mInner.mVScrollbarBox) { Just do "if (!...) early exit;" to save on indentation Call it GetVerticalScrollbarWidth().

Comment on attachment 289747 [details] [diff] [review] fix v2 sr=dbaron Looks like what this is doing is adding the scrollbar width for min-width given overflow-y:scroll, just like we do for pref-width given overflow-y:scroll or overflow-y:auto.

Oh, right, you're using the scrolled frame width too. I guess the sr still stands, but I won't be surprised if you break more than you fix.

Though maybe it won't be that bad. (It's not all an issue of browser-specific content -- the issue I hit last time was two browser differences that canceled each other out.)

Although I'm actually not sure if you should add the scrollbar width (the scrollbars can go away in cases where there isn't room -- although that requires that the box be small in the opposite dimension), or if the scrollbar width condition for min-width should differ from the condition for pref-width the way it does. Or does that match what other browsers do?

(In reply to comment #37) > Or does that match what other browsers do? Yup, fix v2 matches IE7 on the "scrollframe min-width demo (standards mode)" testcase, attachment 289743 [details]. IE7 only adds scrollbar width to min-width in the overflow:scroll case. > [not sure] if the scrollbar > width condition for min-width should differ from the condition for pref-width > the way it does. If we make the min-width condition match the pref-width condition (as fix v1 / attachment 289541 [details] [diff] [review] did), then we look broken. We leave *space* for scrollbars in the "overflow: auto" case, but we don't actually put the scrollbars there. So we get a min-width that's too big. I'm not actually immediately sure why we haven't hit that same problem with **pref width** on overflow:auto divs, though. (i.e. why don't we allocate extra pref-width for scrollbars, and then end up looking weird because we don't insert the scrollbars) -- I'll take a look at that. > (the scrollbars can go away in cases where there isn't room -- although that > requires that the box be small in the opposite dimension) Good point -- I'll take a look at some testcases where the box is too small vertically for scrollbars. (to see what we & other browsers do.)

> If we make the min-width condition match the pref-width condition (as fix v1 / > attachment 289541 [details] [diff] [review] did), then we look broken. oops, I lied -- fix v1 doesn't add any scrollbar width at all in GetMinWidth. When I said fix v1, I was thinking of an intermediate patch between fix v1 and fix v2 that I guess I didn't end up posting, which just used the same conditions in GetMinWidth as in GetPrefWidth.

This testcase is similar to the "scrollframe min-width demo", but with min-heights set on the "overflow: -----" divs.

"scrolltest 2" (attachment 291349 [details]) demonstrates that fix v2 doesn't actually match IE7 when our height is restricted. Here are the differences between IE7 vs. trunk with fix v2. I use "extra width" to mean dedicated width allocated for a vertical scrollbar (in excess of the scrolledFrame's min-width.) overflow: auto - IE7: vertical scrollbar, with extra width - trunk w/ fix v2: vertical scrollbar, without extra width overflow: scroll - IE7: squashed vertical scrollbar, with extra width - trunk w/ fix v2: no vertical scrollbar, but there is extra width for one fix v2's behavior on this testcase doesn't make much sense, for two reasons: - When an "overflow: auto" frame has scrollbars, it should behave similarly to an overflow: scroll frame. - It's kind of broken to allocate min-width for a scrollbar, and then let our content fill that space instead because the scrollbar doesn't fit vertically. Considering the above, I think I now disagree with comment 30; I don't think we should add any min-width for the vertical scrollbar. Instead, we should do something like fix v1 (attachment 289541 [details] [diff] [review]), with GetMinWidth() just returning the min-width of the scrolled frame. That strategy matches WebKit behavior -- they don't add any min-width for scrollbars. As mentioned in comment 31, this makes them look bad in some cases, because their scrollbars draw on top of the scrolledFrame's content. (e.g. the "overflow: scroll" div in the scrollframe min-width and scrolltest 2 testcases) But I think that's reasonable behavior when the size is that constrained, and it's certainly nicer than our existing behavior of setting min-width = 0.

(In reply to comment #41) > Considering the above, I think I now disagree with comment 30; I don't think we > should add any min-width for the vertical scrollbar. Instead, we should do > something like fix v1 (attachment 289541 [details] [diff] [review]), with GetMinWidth() just returning > the min-width of the scrolled frame. > > That strategy matches WebKit behavior -- they don't add any min-width for > scrollbars. As mentioned in comment 31, this makes them look bad in some cases, > because their scrollbars draw on top of the scrolledFrame's content. (e.g. the > "overflow: scroll" div in the scrollframe min-width and scrolltest 2 testcases) > But I think that's reasonable behavior when the size is that constrained, and > it's certainly nicer than our existing behavior of setting min-width = 0. That sounds fine to me.

> Instead, we should do > something like fix v1 (attachment 289541 [details] [diff] [review] [details]), with GetMinWidth() just returning > the min-width of the scrolled frame. This fix does this. It makes nsHTMLScrollFrame::GetMinWidth return the scrolled frame's minWidth. (instead of always returning 0) All of the reftests basically just check that scrollFrame min-width == scrolled frame's min-width. - 402567-1 checks this for overflow:hidden and overflow:auto by removing the scrollframes in the reference case. - 402567-2 checks this for overflow:scroll by adding invisible (white) text to be shrink-wrapped along with the scrollframe in the reference case. - 402567-3 is the same as 402567-2, but with wider, multi-line content. - 402567-4 is the same as 402567-3, but with overflow:auto and with a limited height so that a scrollbar is forced to appear. I've verified that these reftests fail pre-patch and pass post-patch. I've also verified that the patch passes the rest of the reftest suite.

Comment on attachment 291471 [details] [diff] [review] fix v3b (one more reftest) r+sr=dbaron

Landed: Checking in generic/nsGfxScrollFrame.cpp; /cvsroot/mozilla/layout/generic/nsGfxScrollFrame.cpp,v <-- nsGfxScrollFrame.cpp new revision: 3.322; previous revision: 3.321 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402567-1-ref.html,v done Checking in reftests/bugs/402567-1-ref.html; /cvsroot/mozilla/layout/reftests/bugs/402567-1-ref.html,v <-- 402567-1-ref.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402567-1.html,v done Checking in reftests/bugs/402567-1.html; /cvsroot/mozilla/layout/reftests/bugs/402567-1.html,v <-- 402567-1.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402567-2-ref.html,v done Checking in reftests/bugs/402567-2-ref.html; /cvsroot/mozilla/layout/reftests/bugs/402567-2-ref.html,v <-- 402567-2-ref.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402567-2.html,v done Checking in reftests/bugs/402567-2.html; /cvsroot/mozilla/layout/reftests/bugs/402567-2.html,v <-- 402567-2.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402567-3-ref.html,v done Checking in reftests/bugs/402567-3-ref.html; /cvsroot/mozilla/layout/reftests/bugs/402567-3-ref.html,v <-- 402567-3-ref.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402567-3.html,v done Checking in reftests/bugs/402567-3.html; /cvsroot/mozilla/layout/reftests/bugs/402567-3.html,v <-- 402567-3.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402567-4-ref.html,v done Checking in reftests/bugs/402567-4-ref.html; /cvsroot/mozilla/layout/reftests/bugs/402567-4-ref.html,v <-- 402567-4-ref.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/402567-4.html,v done Checking in reftests/bugs/402567-4.html; /cvsroot/mozilla/layout/reftests/bugs/402567-4.html,v <-- 402567-4.html initial revision: 1.1 done Checking in reftests/bugs/reftest.list; /cvsroot/mozilla/layout/reftests/bugs/reftest.list,v <-- reftest.list new revision: 1.248; previous revision: 1.247 done

verified fixed using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9b2pre) Gecko/2007120504 Minefield/3.0b2pre. I verified by using the steps in Comment 0 to verify the dropdown widget for time is present.

This maybe caused a regression bug #407016.

This also caused the regression bug 407257. (Turns out that comment 27 was regarding the Gmail "New Version" working fine. However, the "Old Version" contacts pane *is* broken after this bug's patch. See the bug page for more details)

The decisions in comment #41 and #42 are being reversed in bug 405952.

Regressed bug 421324 too?