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)
Core
Layout: Block and Inline
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+
roc
:
superreview+
|
Details | Diff | Splinter Review |
22.20 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
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.
Reporter | ||
Updated•19 years ago
|
Assignee: nobody → dbaron
Flags: blocking1.9a1?
Comment 1•19 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•19 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•19 years ago
|
||
SO, should I just make this bug block bug 320502 and clear the security flag on this bug?
Assignee | ||
Comment 4•19 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•19 years ago
|
Group: security
Assignee | ||
Updated•18 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•18 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•18 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•18 years ago
|
Flags: blocking1.9+
Assignee | ||
Comment 7•18 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•18 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•18 years ago
|
||
*** Bug 328240 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 9•18 years ago
|
||
*** Bug 329050 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 10•18 years ago
|
||
*** Bug 356380 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Comment 11•18 years ago
|
||
*** Bug 354621 has been marked as a duplicate of this bug. ***
Comment 12•18 years ago
|
||
*** Bug 360385 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 13•18 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+]
Updated•18 years ago
|
Flags: blocking1.9a1+
Reporter | ||
Comment 14•18 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•18 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•17 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•17 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•17 years ago
|
||
Hopefully inline-block support will be landing in the next week or so (bug 9458).
Comment 20•17 years ago
|
||
Comment 22•17 years ago
|
||
Assignee | ||
Comment 23•17 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.
Comment 24•17 years ago
|
||
That seems reasonable. This block would then become the content insertion frame?
Assignee | ||
Comment 25•17 years ago
|
||
Yes.
Comment 26•17 years ago
|
||
Targeting/blocking M9 per request by dbaron.
Target Milestone: --- → mozilla1.9 M9
Assignee | ||
Comment 27•17 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?
Comment 28•17 years ago
|
||
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•17 years ago
|
||
Sorry, but I don't actually know what the difference between the two is.
Assignee | ||
Updated•17 years ago
|
Whiteboard: [dbaron-1.9:RwCr]
Assignee | ||
Updated•17 years ago
|
Whiteboard: [dbaron-1.9:RwCr] → [dbaron-1.9:RwCs]
Assignee | ||
Comment 30•17 years ago
|
||
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•17 years ago
|
||
Attachment #286043 -
Attachment is obsolete: true
Assignee | ||
Comment 32•17 years ago
|
||
Assignee | ||
Comment 33•17 years ago
|
||
Attachment #286054 -
Attachment is obsolete: true
Assignee | ||
Comment 34•17 years ago
|
||
...now with some basic reftests.
Attachment #286053 -
Attachment is obsolete: true
Reporter | ||
Comment 35•17 years ago
|
||
Domino WebMail AKA iNotes works well with these 2 patches installed!
Assignee | ||
Comment 36•17 years ago
|
||
... with more reftests.
Attachment #286073 -
Attachment is obsolete: true
Attachment #286102 -
Flags: superreview?(roc)
Attachment #286102 -
Flags: review?(roc)
Assignee | ||
Comment 37•17 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•17 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•17 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•17 years ago
|
||
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 41•17 years ago
|
||
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•17 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•17 years ago
|
||
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•17 years ago
|
||
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•17 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?
Comment 48•17 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.
Assignee | ||
Comment 49•17 years ago
|
||
Assignee | ||
Comment 50•17 years ago
|
||
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•17 years ago
|
Attachment #286310 -
Flags: review?(mconnor)
Updated•17 years ago
|
Attachment #286310 -
Flags: review?(mconnor) → review+
Reporter | ||
Comment 51•17 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•17 years ago
|
||
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•17 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•17 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•17 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
Closed: 17 years ago
Resolution: --- → FIXED
Comment 58•17 years ago
|
||
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
Comment 59•17 years ago
|
||
See also bug 407419, "Text dynamically added to -moz-inline-box is not shown".
Comment 60•16 years ago
|
||
Marking as "in-testsuite+" based on the six reftests for this bug in layout/reftests/bugs.
Flags: in-testsuite+
Assignee | ||
Updated•16 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.
Description
•