402567 - Zimbra's Calendar Quick Add event dialog broken in minefield

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

Attachment: missing time drop-downs on Minefield

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)

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Attachment: time drop downs working on Firefox 2.0.0.9

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Need a regression range.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

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?

Comment 4

[Mike Schroepfer]

Moving to bloking and setting P2

Comment 5

[Samuel Sidler (old account; do not CC)]

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.

Comment 6

[Samuel Sidler (old account; do not CC)]

Attachment: 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.

Comment 7

[Brian Crowder]

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

Comment 8

[Samuel Sidler (old account; do not CC)]

Attachment: Screenshot of Testcase (top is trunk, bottom is branch)

Comment 9

[Samuel Sidler (old account; do not CC)]

Attachment: Testcase 2

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.

Comment 10

[Samuel Sidler (old account; do not CC)]

Attachment: Screenshot of Testcase 2 (top is trunk, bottom is branch)

Comment 12

[Brian Crowder]

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

Comment 13

[Samuel Sidler (old account; do not CC)]

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

Comment 14

[Brian Crowder]

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.

Comment 15

[Samuel Sidler (old account; do not CC)]

(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.

Comment 16

[Daniel Holbert [:dholbert]]

Gonna take a crack at this.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: Testcase 3

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.)

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: (wrong file attached -- ignore)

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.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: Testcase 4

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.

Comment 20

[Daniel Holbert [:dholbert]]

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. 

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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).

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: scrollframe min-width demo (quirks mode)

> > 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

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: fix v1

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?

Comment 24

[Daniel Holbert [:dholbert]]

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.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 27

[Daniel Holbert [:dholbert]]

(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.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 29

[Daniel Holbert [:dholbert]]

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.)

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 31

[Daniel Holbert [:dholbert]]

Attachment: scrollframe min-width demo (standards mode)

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.

Comment 32

[Daniel Holbert [:dholbert]]

Attachment: fix v2

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 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 38

[Daniel Holbert [:dholbert]]

(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.)

Comment 39

[Daniel Holbert [:dholbert]]

> 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.

Comment 40

[Daniel Holbert [:dholbert]]

Attachment: scrolltest 2 (w/ height restriction)

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

Comment 41

[Daniel Holbert [:dholbert]]

"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.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 43

[Daniel Holbert [:dholbert]]

Attachment: fix v3 (with reftests)

Comment 44

[Daniel Holbert [:dholbert]]

Attachment: fix v3b (one more reftest)

> 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 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 291471 [details] [diff] [review]
fix v3b (one more reftest)

r+sr=dbaron

Comment 46

[Daniel Holbert [:dholbert]]

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

Comment 47

[Marcia Knous [:marcia]]

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.

Comment 48

[Gabriel Chadwick]

This maybe caused a regression bug #407016.

Comment 49

[Daniel Holbert [:dholbert]]

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)

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 51

[Frédéric Buclin]

Regressed bug 421324 too?