Closed Bug 321402 Opened 19 years ago Closed 17 years ago

bug 320502 caused text/inlines inside XUL boxes (often -moz-inline-box) to disappear (Folder view does not display header info in Domino Web Mail (inotes))

Categories

(Core :: Layout: Block and Inline, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta1

People

(Reporter: wgianopoulos, Assigned: dbaron)

References

()

Details

(Keywords: regression, testcase, Whiteboard: [dbaron-1.9:RwCs])

Attachments

(6 files, 8 obsolete files)

675 bytes, text/html
Details
244 bytes, text/html
Details
2.19 KB, patch
roc
: review+
Details | Diff | Splinter Review
22.20 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.62 KB, patch
mconnor
: review+
Details | Diff | Splinter Review
9.09 KB, patch
Details | Diff | Splinter Review
Domino Web Mail is essentially useless in current trunk builds of Firefox.  1.0.x and branch builds work fine.

This is a major showstopper for using Firefox in many corporate environments.

This is a regression caused by the checkin for bug 307992.  Backing out that checkin resolves the issue.  I set the security flag on this bug as bug 307992 has the security flag set and thought it best to set the flag here in case the discussion in this bug accidently reveled the as yet undisclosed security issue.

As bug 307992 is a security bug I suspect it is being considered to be included in the upcoming 1.5.0.1 update.
Assignee: nobody → dbaron
Flags: blocking1.9a1?
The patch in question from bug 307992 is not being considered for Firefox 1.5.0.1, so don't panic :)

Do you have a simple testcase that demonstrates this bug?
If I did I would have attached it.  I will try to see if I can sort through the maze of inotes stuff to get to what is actually triggering the issue.  Not sure how successful I will be.

By the way I was trying to set this bug to block 307992, but since I don't have permsssion to acesss that bug it won't permit me to do that. 
SO, should I just make this bug block bug 320502 and clear the security flag on this bug?
Presumably they're using -moz-inline-box (where they really want inline-block or -moz-inline-block), which with that patch cannot contain inline content without an intervening block.
Blocks: 320502
Blocks: 307992
Flags: blocking1.9a1? → blocking1.9a1+
Summary: Folder view does not display header info in Domino Web Mail (inotes) → bug 320502 caused disappearing-content regressions (Folder view does not display header info in Domino Web Mail (inotes))
Making inline frames behave sensibly inside a box requires a lot of the logic that's in nsBlockFrame.  (Consider having <box> <inline/> <block/> <inline/> </box>.)  So I think we should just be putting a block frame in between during frame construction; either unconditionally (the easiest), around consecutive runs of inlines (a bit harder), or around each inline (easy, but noticeably different behavior).
Then again, we might be able to get things partly-working again by just initializing a single nsLineLayout during nsFrame::BoxReflow, although that would break some invariants and probably bring some of the assertions back.
(In reply to comment #5)
> </box>.)  So I think we should just be putting a block frame in between during
> frame construction; either unconditionally (the easiest), around consecutive
> runs of inlines (a bit harder), or around each inline (easy, but noticeably
> different behavior).

This isn't as hard as I was thinking it was if we're also willing to put the block around any nsTextFrames that we create.  (XUL users should normally be doing things that make nsTextBoxFrames.)
Summary: bug 320502 caused disappearing-content regressions (Folder view does not display header info in Domino Web Mail (inotes)) → bug 320502 caused text/inlines inside XUL boxes (often -moz-inline-box) to disappear (Folder view does not display header info in Domino Web Mail (inotes))
*** Bug 328240 has been marked as a duplicate of this bug. ***
*** Bug 329050 has been marked as a duplicate of this bug. ***
*** Bug 356380 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
*** Bug 354621 has been marked as a duplicate of this bug. ***
*** Bug 360385 has been marked as a duplicate of this bug. ***
So I was discussing this with roc, and it's actually pretty complicated to do this, and probably won't happen for a1.  I'm inclined to leave the regression in a1 rather than back out the patch that caused it, though.  We really do want to discourage Web authors from doing this -- maybe shipping an alpha with the bug will reduce the number who do, even if we're hoping to fix it for final.
Flags: blocking1.9a1+
This issue also breaks http://www.giganews.com .

The login and search boxes are not displayed.

http://www.giganews.com/styles/gn/css contains:

  display: -moz-inline-box;

throughout.

(In reply to comment #14)
> This issue also breaks http://www.giganews.com .
Sorry.  Didn't realize this was already reported in bug 360385 already duped to this.  For some odd reason that bug fails to mention giganews in the summary, so it did not come up in my search.
For the Domino inotes case, a workaround is the following in userContent.css

.vl-cell-x { display: inline;float:left !important; }
.vl-cell-xx { display: inline;float:left !important; }
(In reply to comment #13)
> So I was discussing this with roc, and it's actually pretty complicated to do
> this, and probably won't happen for a1.  I'm inclined to leave the regression
> in a1 rather than back out the patch that caused it, though.  We really do want
> to discourage Web authors from doing this -- maybe shipping an alpha with the
> bug will reduce the number who do, even if we're hoping to fix it for final.
> 
I doubt it.  The reason web authors do this is because inline-block does not work in Mozilla.  The workaround that authors seem to use is to specify both inline-block (which Mozilla ignores) and -moz-inline-box (which other browsers ignore).  Leaving this broken will probably make it impossible to do this in a way that works cross browser and also works in trunk and branch.
Hopefully inline-block support will be landing in the next week or so (bug 9458).
Depends on: 384221
Keywords: testcase
Target Milestone: mozilla1.9alpha1 → ---
No longer depends on: 384221
So my plan for fixing this is the following:

If a box, at any time, has a child that is not a box or a block, we'll make the box wrap all its children in a block.  This will introduce odd behavior if some of those children are boxes, and the behavior will persist if the offending children are removed, but it's essentially an error handling case:  something XUL authors shouldn't be doing, but that some (out on the Web) are doing.  I might want to toss a warning onto the error console if we do this or something like that.

I'm really not comfortable doing anything more complicated in the frame construction code right now.
That seems reasonable.  This block would then become the content insertion frame?
Targeting/blocking M9 per request by dbaron.
Target Milestone: --- → mozilla1.9 M9
So messing with frame construction is always worse than I expect.

It's not realistic to destroy and rebuild just the child frames, since we don't have a way to reconstruct just the children (and not out-of-flows).

I probably don't want to use frame reparenting since I don't trust it.  Although maybe I should.

I could do a ContentRemoved, set some state **on the content**, and then do a ContentInserted, and then make all XUL frame construction check that state.





The really evil solution would be to rename -moz-inline-box to something else and add the "new" -moz-inline-box as an alias for inline-block.  It's actually sort of tempting.  Does anyone who uses XUL intentionally actually care about -moz-inline-box?
The only uses of moz-inline-box in our tree are in XBL form controls.  Most uses I can find through google are as a replacement for inline-block.  Searching on "moz-inline-box -inline-block" gives me 171 hits, and again the first few pages are not actual use.

ccing the neils, and we should probably file a bug to get AMO grepped for this, but I suspect the "really evil solution" is the way to go here.  Let's hope no one on the web relies on the "inline-box doesn't line-wrap" thing that everyone works around by putting a block inside it....
Sorry, but I don't actually know what the difference between the two is.
Whiteboard: [dbaron-1.9:RwCr]
Whiteboard: [dbaron-1.9:RwCr] → [dbaron-1.9:RwCs]
Blocks: 307992
Blocks: 400824
Attached patch patch (obsolete) — Splinter Review
Here's a preliminary patch -- still needs a lot more testing, reftests, etc.

And I've actually noticed that it's already breaking some things, which means it's probably overzealous in some cases.
...now with some basic reftests.
Attachment #286053 - Attachment is obsolete: true
Domino WebMail AKA iNotes works well with these 2 patches installed! 
... with more reftests.
Attachment #286073 - Attachment is obsolete: true
Attachment #286102 - Flags: superreview?(roc)
Attachment #286102 - Flags: review?(roc)
Comment on attachment 286072 [details] [diff] [review]
patch 1: make basic XUL not trigger wrapping

So there are two patches on this bug; this one touches xul.css (and one other toolkit file) and therefore I'm requesting review from mconnor on that part of it.

I'm thinking that given that I've run into a few chrome issues (and fixed the ones I've hit so far) that maybe this should wait until after beta, although they're pretty simple to fix if we catch them.  It's a tradeoff -- it'll fix a bunch of Web pages, but there's a risk it might break some chrome (I wouldn't be surprised to see some other things like what I patched in consoleBindings.xml).  (And they're pretty easy to catch, too, since this patch puts a warning on the JS console whenever it does something.)

What's going on here is the following:
 * I'm changing XUL so that it wraps frames the contents of any XUL boxes that have things that have to be in a line in order to work in a box.
 * There were a few pieces of existing XUL that used things that were display:inline.  These things haven't worked for over a year -- meaning they were always blank, and any contents wouldn't show up.  But it turns out the things that were using them were spacers -- and nobody really cared if we had a bug that made spacers empty.


I split the changes into two patches because they're logically separate and touch the same file.  This one adds display:-moz-empty to create an nsFrame instead of an nsInlineFrame for these spacer cases.  The other patch is the main fix:  it uses code quite similar to our block-in-inline code to do the wrapping.  (Each time I wanted to do something differently, it wasn't as easy as I thought, and I went back to doing it the same as the block-in-inline code.)
Attachment #286072 - Flags: superreview?(roc)
Attachment #286072 - Flags: review?(mconnor)
Why do we need -moz-empty? Can't we just use an empty XUL box here?

+    // XXX Should we really need to do this for text frames?  Will we
+    // ever do it for whitespace-only text frames?

I think so, in both cases. Things would be cleaner if we can be sure text frames are always reflowed in a proper inline context.

+          for (nsIFrame *f = childItems.childList; f; f = f->GetNextSibling()) {
+            f->SetParent(blockFrame);
+          }

I think you should call nsCSSFrameConstructor::ReparentFrame here. That takes care of NS_FRAME_HAS_CHILD_WITH_VIEW for you.

Why don't you call InitAndRestoreFrame instead of Init directly? I'm following nsCSSFrameConstructor::FlushAccumulatedBlock here...

+          childItems.~nsFrameItems();
+          new (&childItems) nsFrameItems();

You can just do childItems = nsFrameItems();

Should there be a reftest for a float inside a XUL box?

This is quite similar to the wrapping of MathML children in blocks. The MathML code differs from yours in two ways:
-- instead of wrapping all the kids in a block, it wraps each run of consecutive inline kids in a block
-- if there are any DOM changes to the kids, we just reconstruct the world.
I guess the latter point would be no good for XUL, so we probably shouldn't try to merge the approaches.
(In reply to comment #38)
> Why do we need -moz-empty? Can't we just use an empty XUL box here?

That's equvalent to removing the rules -- I'm still trying to figure out why they're there.  The checkin comment for the spacer rule is "etc.":
http://bonsai.mozilla.org/cvslog.cgi?file=/mozilla/xpfe/global/resources/content/xul.css&rev=1.50
and for the toolbar stuff it's "polishing the customize toolbar dialog even more":
http://bonsai.mozilla.org/cvslog.cgi?file=/mozilla/toolkit/content/xul.css&rev=1.10

Then again, I don't see anything that breaks if I just remove the rules, so I guess I should try that.
Attachment #286072 - Attachment is obsolete: true
Attachment #286179 - Flags: superreview?(roc)
Attachment #286179 - Flags: review?(mconnor)
Attachment #286072 - Flags: superreview?(roc)
Attachment #286072 - Flags: review?(roc)
Attachment #286072 - Flags: review?(mconnor)
Comment on attachment 286179 [details] [diff] [review]
patch 1: make basic XUL not trigger wrapping

r=mconnor
Attachment #286179 - Flags: review?(mconnor) → review+
(In reply to comment #38)
> +    // XXX Should we really need to do this for text frames?  Will we
> +    // ever do it for whitespace-only text frames?
> 
> I think so, in both cases. Things would be cleaner if we can be sure text
> frames are always reflowed in a proper inline context.

I'd thought that wasn't the case, but it turned out it is:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsTextFrameThebes.cpp&rev=3.117&mark=5096-5104#5068
so I should just remove the comment.

> +          for (nsIFrame *f = childItems.childList; f; f = f->GetNextSibling())
> {
> +            f->SetParent(blockFrame);
> +          }
> 
> I think you should call nsCSSFrameConstructor::ReparentFrame here. That takes
> care of NS_FRAME_HAS_CHILD_WITH_VIEW for you.

You mean nsHTMLContainerFrame::ReparentFrameViewList?  It also does a lot more work needed to handle the general case; in this case we know the wrapper block doesn't have a view.  (I should probably assert that, though.)

> Why don't you call InitAndRestoreFrame instead of Init directly? I'm following
> nsCSSFrameConstructor::FlushAccumulatedBlock here...

I couldn't think of any state we'd need to restore, and I wasn't 100% sure the state restoration code would handle the anonymous box correctly -- although I checked, and it'll clearly exit early for any  frames that don't implement nsIStatefulFrame.  This means there's no harm to it except the extra work, so I guess I'll switch in case we put more stuff in that function.

> +          childItems.~nsFrameItems();
> +          new (&childItems) nsFrameItems();
> 
> You can just do childItems = nsFrameItems();

ok.

> Should there be a reftest for a float inside a XUL box?

Probably -- although I think it just floats to outside the box, which really isn't a behavior we like very much.

I should also add reftests for the dynamic change cases (append, insert, remove).

> This is quite similar to the wrapping of MathML children in blocks. The MathML
> code differs from yours in two ways:
> -- instead of wrapping all the kids in a block, it wraps each run of
> consecutive inline kids in a block

This part is much harder to maintain dynamically (which I guess is why the latter part is true).

> -- if there are any DOM changes to the kids, we just reconstruct the world.
> I guess the latter point would be no good for XUL, so we probably shouldn't try
> to merge the approaches.
(In reply to comment #42)
> (In reply to comment #38)
> > +          for (nsIFrame *f = childItems.childList; f; f = f->GetNextSibling())
> > {
> > +            f->SetParent(blockFrame);
> > +          }
> > 
> > I think you should call nsCSSFrameConstructor::ReparentFrame here. That takes
> > care of NS_FRAME_HAS_CHILD_WITH_VIEW for you.
> 
> You mean nsHTMLContainerFrame::ReparentFrameViewList?

No, actually I meant static ReparentFrame in nsCSSFrameConstructor.cpp.

> > Why don't you call InitAndRestoreFrame instead of Init directly? I'm following
> > nsCSSFrameConstructor::FlushAccumulatedBlock here...
> 
> I couldn't think of any state we'd need to restore, and I wasn't 100% sure the
> state restoration code would handle the anonymous box correctly -- although I
> checked, and it'll clearly exit early for any  frames that don't implement
> nsIStatefulFrame.  This means there's no harm to it except the extra work, so I
> guess I'll switch in case we put more stuff in that function.

OK.

> > Should there be a reftest for a float inside a XUL box?
> 
> Probably -- although I think it just floats to outside the box, which really
> isn't a behavior we like very much.

Yeah, that sounds scary...
Comment on attachment 286179 [details] [diff] [review]
patch 1: make basic XUL not trigger wrapping

xpfe/global/resources/content/xul.css is still using -moz-empty.
Oh, oops.
Attachment #286179 - Attachment is obsolete: true
Attachment #286203 - Flags: superreview?(roc)
Attachment #286203 - Flags: review?(roc)
Attachment #286179 - Flags: superreview?(roc)
Attachment #286102 - Attachment is obsolete: true
Attachment #286204 - Flags: superreview?(roc)
Attachment #286204 - Flags: review?(roc)
Attachment #286102 - Flags: superreview?(roc)
Attachment #286102 - Flags: review?(roc)
Attachment #286203 - Flags: superreview?(roc)
Attachment #286203 - Flags: superreview+
Attachment #286203 - Flags: review?(roc)
Attachment #286203 - Flags: review+
Attachment #286204 - Flags: superreview?(roc)
Attachment #286204 - Flags: superreview+
Attachment #286204 - Flags: review?(roc)
Attachment #286204 - Flags: review+
(In reply to comment #39)
>(In reply to comment #38)
>>Why do we need -moz-empty? Can't we just use an empty XUL box here?
>That's equvalent to removing the rules -- I'm still trying to figure out why
>they're there.
Possibly because empty frames are cheaper than box frames?
Depends on: 401226
Opening the Add-ons manager and clicking extensions now triggers this:
Warning: XUL box for hbox element contained an inline spacer child, forcing all its children to be wrapped in a block.
Source File: chrome://mozapps/content/extensions/extensions.xul

This affects the layout of the buttons for each extension.
Depends on: 401266
I searched the whole tree, and there was one other occurrence, which was the xpfe version of the consoleBindings.xml change in attachment 286203 [details] [diff] [review].
Attachment #286309 - Attachment is obsolete: true
Attachment #286310 - Flags: review?(mconnor) → review+
The console2 extension has the badly named spacer issue so gets into a loop reporting errors.  I filed a bug on the extension including the fix.
I think this sohuld be less risky for the beta, and probably permanently.  The change here is that it does the wrapping only for -moz-inline-box and not for any other box types (so for those these frames will continue to disappear, and they'll still need to null-check the line layout in their reflow methods).
Attachment #286344 - Flags: superreview?(roc)
Attachment #286344 - Flags: review?(roc)
Wouldn't this be a good time to get chrome and extensions fixed?
Well, there's no reason the less risky approach can't be permanent.
The "less risky approach" leaves inline and text frames occurring in non-block contexts, which is something I'd like to eliminate eventually.
> I searched the whole tree, and there was one other occurrence, which was the
> xpfe version of the consoleBindings.xml

On trunk SeaMonkey is using the toolkit Error Console. Are there any more consumers of the XPFE version?
OK, I guess I'll leave the riskier version in.

Note that this patch did regress a reftest; see bug 401176 for details.

Checked in the afternoon of 2007-10-25.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007103004 Minefield/3.0a9pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007103004 Minefield/3.0a9pre. I verified using the two testcases listed in the bug.
Status: RESOLVED → VERIFIED
Blocks: 401820
See also bug 407419, "Text dynamically added to -moz-inline-box is not shown".
Marking as "in-testsuite+" based on the six reftests for this bug in 
layout/reftests/bugs.
Flags: in-testsuite+
Attachment #286344 - Flags: superreview?(roc)
Attachment #286344 - Flags: review?(roc)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: