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

VERIFIED FIXED in mozilla1.9beta1

Status

()

Core
Layout: Block and Inline
P1
major
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: WG9s, Assigned: dbaron)

Tracking

({regression, testcase})

Trunk
mozilla1.9beta1
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dbaron-1.9:RwCs], URL)

Attachments

(6 attachments, 8 obsolete attachments)

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

Description

12 years ago
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.
(Reporter)

Updated

12 years ago
Assignee: nobody → dbaron
Flags: blocking1.9a1?

Comment 1

12 years ago
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?
(Reporter)

Comment 2

12 years ago
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. 
(Reporter)

Comment 3

12 years ago
SO, should I just make this bug block bug 320502 and clear the security flag on this bug?
(Assignee)

Comment 4

12 years ago
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
(Assignee)

Updated

12 years ago
Group: security
Blocks: 307992
(Assignee)

Updated

12 years ago
No longer blocks: 307992
(Assignee)

Updated

11 years ago
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))
(Assignee)

Comment 5

11 years ago
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).
(Assignee)

Comment 6

11 years ago
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.
(Assignee)

Updated

11 years ago
Flags: blocking1.9+
(Assignee)

Comment 7

11 years ago
(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.)
Whiteboard: [blocking1.9a1+]
(Assignee)

Updated

11 years ago
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))
(Assignee)

Comment 8

11 years ago
*** Bug 328240 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 9

11 years ago
*** Bug 329050 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 10

11 years ago
*** Bug 356380 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 11

11 years ago
*** Bug 354621 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

11 years ago
Blocks: 350456

Comment 12

11 years ago
*** Bug 360385 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 13

11 years ago
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.
Whiteboard: [blocking1.9a1+]
Flags: blocking1.9a1+
(Reporter)

Comment 14

11 years ago
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.

(Reporter)

Comment 15

11 years ago
(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.
(Reporter)

Comment 16

11 years ago
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; }
(Reporter)

Comment 17

11 years ago
(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.
(Assignee)

Comment 18

11 years ago
Hopefully inline-block support will be landing in the next week or so (bug 9458).

Updated

10 years ago
Depends on: 384221

Updated

10 years ago
Duplicate of this bug: 384221
Created attachment 268317 [details]
Martijn's minimalish testcase

Updated

10 years ago
Keywords: testcase
Target Milestone: mozilla1.9alpha1 → ---

Updated

10 years ago
No longer depends on: 384221

Updated

10 years ago
Duplicate of this bug: 389171

Comment 22

10 years ago
Created attachment 276284 [details]
Reduced testcase based on Domino Web Mail
(Assignee)

Comment 23

10 years ago
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?
(Assignee)

Comment 25

10 years ago
Yes.
Targeting/blocking M9 per request by dbaron.
Target Milestone: --- → mozilla1.9 M9
(Assignee)

Comment 27

10 years ago
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....

Comment 29

10 years ago
Sorry, but I don't actually know what the difference between the two is.
(Assignee)

Updated

10 years ago
Whiteboard: [dbaron-1.9:RwCr]
(Assignee)

Updated

10 years ago
Whiteboard: [dbaron-1.9:RwCr] → [dbaron-1.9:RwCs]

Updated

10 years ago
Blocks: 307992

Updated

10 years ago
Blocks: 400824
(Assignee)

Comment 30

10 years ago
Created attachment 286043 [details] [diff] [review]
patch

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

Comment 31

10 years ago
Created attachment 286053 [details] [diff] [review]
patch 1: wrap contents of some boxes in a block
Attachment #286043 - Attachment is obsolete: true
(Assignee)

Comment 32

10 years ago
Created attachment 286054 [details] [diff] [review]
patch 2: make basic XUL not trigger this wrapping
(Assignee)

Comment 33

10 years ago
Created attachment 286072 [details] [diff] [review]
patch 1: make basic XUL not trigger wrapping
Attachment #286054 - Attachment is obsolete: true
(Assignee)

Comment 34

10 years ago
Created attachment 286073 [details] [diff] [review]
patch 2: wrap contents of some boxes in a block

...now with some basic reftests.
Attachment #286053 - Attachment is obsolete: true
(Reporter)

Comment 35

10 years ago
Domino WebMail AKA iNotes works well with these 2 patches installed! 
(Assignee)

Comment 36

10 years ago
Created attachment 286102 [details] [diff] [review]
patch 2: wrap contents of some boxes in a block

... with more reftests.
Attachment #286073 - Attachment is obsolete: true
Attachment #286102 - Flags: superreview?(roc)
Attachment #286102 - Flags: review?(roc)
(Assignee)

Comment 37

10 years ago
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)
(Assignee)

Updated

10 years ago
Attachment #286072 - Flags: review?(roc)
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.
(Assignee)

Comment 39

10 years ago
(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.
(Assignee)

Comment 40

10 years ago
Created attachment 286179 [details] [diff] [review]
patch 1: make basic XUL not trigger wrapping
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+
(Assignee)

Comment 42

10 years ago
(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.
(Assignee)

Comment 45

10 years ago
Created attachment 286203 [details] [diff] [review]
patch 1: make basic XUL not trigger wrapping

Oh, oops.
Attachment #286179 - Attachment is obsolete: true
Attachment #286203 - Flags: superreview?(roc)
Attachment #286203 - Flags: review?(roc)
Attachment #286179 - Flags: superreview?(roc)
(Assignee)

Comment 46

10 years ago
Created attachment 286204 [details] [diff] [review]
patch 2: wrap contents of some boxes in a block
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+

Comment 47

10 years ago
(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?
(Assignee)

Updated

10 years ago
Depends on: 401176
Depends on: 401226

Comment 48

10 years ago
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.

Updated

10 years ago
Depends on: 401266
(Assignee)

Comment 49

10 years ago
Created attachment 286309 [details] [diff] [review]
fix addons dialog
(Assignee)

Comment 50

10 years ago
Created attachment 286310 [details] [diff] [review]
fix badly namespaced spacers (addons dialog and xpfe consoleBindings)

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

Updated

10 years ago
Attachment #286310 - Flags: review?(mconnor)

Updated

10 years ago
Attachment #286310 - Flags: review?(mconnor) → review+
(Reporter)

Comment 51

10 years ago
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.
(Assignee)

Comment 52

10 years ago
Created attachment 286344 [details] [diff] [review]
less risky approach:  wrap only -moz-inline-box

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?
(Assignee)

Comment 54

10 years ago
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.

Comment 56

10 years ago
> 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?
(Assignee)

Comment 57

10 years ago
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
Last Resolved: 10 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

Updated

10 years ago
Blocks: 401820
Depends on: 403505

Comment 59

10 years ago
See also bug 407419, "Text dynamically added to -moz-inline-box is not shown".

Comment 60

10 years ago
Marking as "in-testsuite+" based on the six reftests for this bug in 
layout/reftests/bugs.
Flags: in-testsuite+
(Assignee)

Updated

10 years ago
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.