Closed
Bug 63895
Opened 24 years ago
Closed 11 years ago
positioned internal table elements not abs pos containing block
Categories
(Core :: Layout: Positioned, defect)
Core
Layout: Positioned
Tracking
()
VERIFIED
FIXED
mozilla31
People
(Reporter: stephenc, Assigned: seth)
References
(Depends on 2 open bugs, Blocks 2 open bugs, )
Details
(Keywords: dev-doc-needed, relnote, testcase, Whiteboard: [awd:tbl])
Attachments
(12 files, 18 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
roc
:
review+
seth
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
dbaron
:
review+
seth
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
seth
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
seth
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
seth
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lsblakk
:
approval-mozilla-aurora+
seth
:
checkin+
|
Details | Diff | Splinter Review |
From Bugzilla Helper: User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows 98) BuildID: 2000122805 Mozilla seems to be disregarding a relative positioned table as an ancestral containing block, positioning the absolute positioned div inside the table from the edge of the viewport rather than from the edge of the table. Reproducible: Always Steps to Reproduce: Open supplied URL Actual Results: Overlayed div is positioned from viewport. Expected Results: Overlayed div should be positioned from relative positioned containing table. The page *does* render properly if I encase both the table and div inside another div and set that div to relative (switching the margin and other properties currently on the table to the div, of course). But, both table and div are valid containing blocks according to CSS2. Why does the positioning work inside div but not inside table?
Comment 1•24 years ago
|
||
Reporter Several of your <INPUT...> statements are lacking the neccesary / at the end of them as stated in http://www.w3.org/TR/2000/REC-xhtml1-20000126/ My guess is you might want to try and fix those and see if it still occurs.
Reporter | ||
Comment 2•24 years ago
|
||
As predicted, fixing the very minor unrelated errors on the page does nothing for the positioning. The errors are due to an external HTML library and aren't under my control. To satisfy the pedantic people, I put the script output into an HTML file and fixed things there. I've changed the bug's URL to point to this page instead. This page validates as XHTML 1.0 Strict but the bad positioning remains. As I noted in the original text, the exact same page (with or without bad input tags) is positioned properly if the entire page body is wrapped in a div and position based off that. This should not be necessary according to the spec.
Comment 3•24 years ago
|
||
Ok going ahead and marking it NEW since I see it as well on the Linux 2001010221 Build.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 98 → All
Hardware: PC → All
Comment 4•24 years ago
|
||
Netscape's standard compliance QA team reorganised itself once again, so taking remaining non-tables style bugs. Sorry about the spam. I tried to get this done directly at the database level, but apparently that is "not easy because of the shadow db", "plus it screws up the audit trail", so no can do...
QA Contact: chrisd → ian
Updated•24 years ago
|
QA Contact: ian → amar
Comment 6•24 years ago
|
||
retitling, ->Layout
Assignee: pierre → karnaze
Component: Style System → Layout
QA Contact: amar → petersen
Summary: Absolute positioned div inside relative positioned table using viewport instead of table as containing block → relative/absolute positioned table not abs pos containing block
Comment 7•23 years ago
|
||
This code probably needs to be fixed in 2 places, the second of which (for dynamic changes) is nsCSSFrameConstructor::GetAbsoluteContainingBlock.
Comment 8•23 years ago
|
||
Temporarily moving to future until a milestone can be assigned.
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Comment 9•23 years ago
|
||
Comment 10•23 years ago
|
||
URL (http://fs1.theiqgroup.com/iqclogin.html) is 404, removing it.
Keywords: testcase
Whiteboard: [awd:tbl][NEED TESTCASE] → [awd:tbl]
Comment 11•23 years ago
|
||
Updated•23 years ago
|
Attachment #76045 -
Attachment description: Testcase → Testcase #1, abs in rel
Comment 14•23 years ago
|
||
Identical behavior also occurs if the absolutely positioned DIV is inside a staticly positioned (default) DIV. It ignores the static DIV and positions the absolute DIV relative to the page as a whole.
Comment 15•23 years ago
|
||
Never mind. Since the parent DIV isn't positioned, it's not considered a containing block, so my comment doesn't mean anything.
Comment 16•22 years ago
|
||
This seems like a pretty significant bug, considering that CSS2 positioning is supposed to end dependence on table-based layouts.
Comment 17•22 years ago
|
||
I looked at the source for nsCSSFrameConstructor::GetAbsoluteContaining Block: http://lxr.mozilla.org/seamonkey/source/layout/html/style/src/nsCSSFrameConstructor.cpp#7615 and I found the following comment: 7624 // Is it positioned? 7625 // If it's a table then ignore it, because for the time being tables 7626 // are not containers for absolutely positioned child frames Does anyone know the reason for this change in behavior? Derek
Comment 18•22 years ago
|
||
.
Assignee: karnaze → position
Status: ASSIGNED → NEW
Component: Layout → Layout: R & A Pos
QA Contact: petersen → madhur
Updated•22 years ago
|
Comment 19•22 years ago
|
||
Should this bug have css2 keyword added? It would be really nice to have this fixed for 1.3. This bug is a pet peeve of mine, and it's been around for a very long time :)
Updated•22 years ago
|
Priority: -- → P4
Comment 21•22 years ago
|
||
I've run into this one too. For the time being, I'm surrounding my tables with a rel. pos. DIV element to make it work. Ugly hack, though.
![]() |
||
Comment 23•22 years ago
|
||
This is the sort of change that belongs at the beginning of an alpha milestone, not at the end of a release cycle.
Comment 24•22 years ago
|
||
Why? this aint an enhancement. this is a bug. It could well be considered for a major release version. Not to mention it is tagged for future and severity to minor. When are we going to see Mozilla swimming? Arent you guys bored of fixing web browser features that belongs to bronze age? I see tons of new features on 1.3b although this kinda bugs are all still tagged for an NP future. All the Iframe bugs are tagged future, for example. Is the purpose no longer having a next generation browser anymore? AOL version 9 pressure to drivers perhaps ;)?
![]() |
||
Comment 25•22 years ago
|
||
> Why?
Because it would be a complicated, delicate change with lots of potential for
regressions? Thus not acceptable for a "stable" release without a _lot_ of
prior testing.
Drivers have nothing to do with anything. This is a volunteer project; people
work on what they work on. The number of people who want to work on layout is
very low, largely (in my opinion_ due to pushy bug reporters who demand things
as their right without thinking about the fact that developers may have very
limited time to work on Mozilla at all....
For your information,right now the focus of the few people working on layout is
on basic arch work that will make the code simpler. Once that's done, fixing
this bug should be a lot easier.
Comment 26•22 years ago
|
||
>Because it would be a complicated, delicate change with lots of potential for >regressions? Thus not acceptable for a "stable" release without a _lot_ of >prior testing. very well, >Drivers have nothing to do with anything. This is a volunteer project; people >work on what they work on. The number of people who want to work on layout is >very low, largely (in my opinion_ due to pushy bug reporters who demand things >as their right without thinking about the fact that developers may have very >limited time to work on Mozilla at all.... Yes it is a volunteer project. but for the few that is although there is nothing wrong with it and seems like has been working just fine. However, it has been quite a while since version 1.0,and I still dont see a serious attempt to clean up DOM/Layout level bugs by the Netscape engineers(not the volunteers). >For your information,right now the focus of the few people working on layout is >on basic arch work that will make the code simpler. Once that's done, fixing >this bug should be a lot easier. Uh I see now! Although we all thank you very much for all the effort and time you guys spent on Mozilla development, I regretfully see above quote as a serious problem. That certainly would require a lot of rewrite and reinterfacing in the spagetti(no offense) of Mozilla codebase. It gives me the signal that "future" tag meant for this bug and its alikes is really an unforeseeable future. In any case, I sincerely thank you all very much for the incredible effort put. regards, ps: how is the vacation going?
![]() |
||
Comment 27•22 years ago
|
||
> I still dont see a serious attempt to clean up DOM/Layout level bugs by the
> Netscape engineers
You do know that Netscape pulled pretty much everyone off of layout, right? I
think there is basically 1 person actively doing layout work at Netscape right
now (the layout group has 4 people technically, but 3 of them are being
distracted with other projects by management).
Comment 28•22 years ago
|
||
I came across this behavior recently, wondered if it was a bug, or not, and so went to check the spec. http://www.w3.org/TR/REC-CSS2/visudet.html#containing-block-details "If the element has 'position: absolute', the containing block is established by the nearest ancestor with a 'position' other than 'static', in the following way: 1. In the case that the ancestor is block-level, the containing block is formed by the padding edge of the ancestor. 2. In the case that the ancestor is inline-level, the containing block depends on the 'direction' property of the ancestor: 1. If the 'direction' is 'ltr', the top and left of the containing block are the top and left content edges of the first box generated by the ancestor, and the bottom and right are the bottom and right content edges of the last box of the ancestor. 2. If the 'direction' is 'rtl', the top and right are the top and right edges of the first box generated by the ancestor, and the bottom and left are the bottom and left content edges of the last box of the ancestor. If there is no such ancestor, the content edge of the root element's box establishes the containing block." In this case, the ancestor is not block-level, or inline-level. It is display: table-something. So are we in the "If there is no such ancestor, ..." clause? If we are, then Mozilla is currently doing the right thing, and this bug is INVALID. Of course, this may be an unitentional feature of the CSS2 spec, however they have not changed it in CSS2.1. Perhaps someone should check with the CSS2.1 people. http://www.w3.org/TR/CSS21/visudet.html#containing-block-details For reference, IE6 renders as if this bug was fixed, while Opera 7.03 reders this like Moz 2003030908 does (the alledgedly buggy behavior).
Comment 29•22 years ago
|
||
A table may not be 'display: block', but it is clearly still a block-level box. From http://www.w3.org/TR/REC-CSS2/tables.html: The following 'display' values assign table semantics to an arbitrary element: table (In HTML: TABLE) Specifies that an element defines a block-level table: it is a rectangular block that participates in a block formatting context.
Comment 30•22 years ago
|
||
A table may not be 'display: block', but it is clearly still a block-level box. From http://www.w3.org/TR/REC-CSS2/tables.html: The following 'display' values assign table semantics to an arbitrary element: table (In HTML: TABLE) Specifies that an element defines a block-level table: it is a rectangular block that participates in a block formatting context.
Comment 31•22 years ago
|
||
1. Tables are block-level. 2. for 'position: absolute', things are more complicated (CSS2 9.7) but this is still valid.
Comment 32•22 years ago
|
||
Resummarizing based on duplicates.
Summary: relative/absolute positioned table not abs pos containing block → positioned table or internal table elements not abs pos containing block
Comment 34•21 years ago
|
||
(In reply to comment #17) > 7624 // Is it positioned? > 7625 // If it's a table then ignore it, because for the time being tables > 7626 // are not containers for absolutely positioned child frames It's been there since the very beginning of that file. Looks like this is a seriously dated decision, possibly pre-CSS1.
Comment 38•20 years ago
|
||
(In reply to comment #37) > Wow... 12-28-2000... Don't hold my breath eh? Please keep those remarks to yourself, this doesn't help.
Comment 43•20 years ago
|
||
How are advantages of this behavior? According my opinion is it only disadvantage. How much authors use this behavior? How many pages can change of this behavior cause regression? Actual situation, why Opera and Mozilla implements this variously, is problem for webdesigner which can use it. What about rounded corner in table block? I wanted use this in this page: http://tom.gonet.cz/projekty/seznamka/_test2/, but I canβοΏ½. This is the same situation as with <ul>, <li> behaviors of margin and padding in Mozilla and Opera. This is bad for community of web designers and users.
Comment 45•20 years ago
|
||
Would it not suffice to remove && disp->mDisplay != NS_STYLE_DISPLAY_TABLE from /layout/base/nsCSSFrameConstructor.cpp:7807?
Comment 46•20 years ago
|
||
No, since the elements in question would need to be changed to support absolutely positioned children.
Comment 49•19 years ago
|
||
Is my marked-blocked bug 217002 a dupe of this one, or a derivative issue?
Comment 50•19 years ago
|
||
I would like this to be fixed as most others in this thread (save for the "engineering" issues, which are valid enough, but five years?).
Comment 52•19 years ago
|
||
Hi all, I am celebrating today my third anniversary as a reporter of this bug. I reported it Dec 23, 2002 (http://bugzilla.mozilla.org/show_bug.cgi?id=186581)and was soon notified that it was a duplicate of this one. In just a few days the bug will pass its fifth anniversary since originally reported by Stephen Clouse on Dec. 28, 2000, near the end of the last century. I've subscribed to the "progress" of the bug over the years since then. Here are my picks for each year's highlights: o 2001: A comment by Hixie (https://bugzilla.mozilla.org/show_bug.cgi?id=63895#c4). I didn't understand this one really ( I don't understand any of the technical comments really) but it seems really cool that Hixie has been on the case. O 2002: Derek (https://bugzilla.mozilla.org/show_bug.cgi?id=63895#c16): "This seems like a pretty significant bug, considering that CSS2 positioning is supposed to end dependence on table-based layouts." o 2003: Boris (https://bugzilla.mozilla.org/show_bug.cgi?id=63895#c25) complaining about "pushy bug reporters who demand things as their right without thinking about the fact that ..." yada yada. o 2004: Joe (https://bugzilla.mozilla.org/show_bug.cgi?id=63895#c37) "Wow... 12-28-2000... Don't hold my breath eh?", followed by Martin's riposte (https://bugzilla.mozilla.org/show_bug.cgi?id=63895#c38) "Please keep those remarks to yourself, this doesn't help." o 2005: ATom (https://bugzilla.mozilla.org/show_bug.cgi?id=63895#c43): "How are advantages of this behavior? According my opinion is it only disadvantage. How much authors use this behavior? How many pages can change of this behavior cause regression?" I'm looking forward to lots more analysis and opinion on this bug in 2006! Happy holidays, Hugh
Comment 53•19 years ago
|
||
(In reply to comment #52) Hi Hugh, I can agree with you that this is a long standing bug. I attempted to help find a resolution in December of 2004 with 250763. Unfortunately, the developers seem to think this is an IE issue not a Mozilla issue....and that my report was fixed, but alas it was not and then became a duplicate of 63895. I find it disconcerting that when you follow the bug blocks that there are so many dependencies and duplicates being shown for the positioning that I believe the probelm lies in the fact that no one has really gathered all the pieces together to determine the real underlying principle to the cause. I've read several of the postings and followed many of the duplicates and there is one underlying principle to most: CSS compliance to positioning blocks does not seem to be happening properly. The behaviour does not go to the 'expected right'. This my perspective of the issue. I think we as a community need to rally all reporters to vote for the open bugs, blocks and dependencies.
Comment 54•18 years ago
|
||
Hi all, Just thought I'd log in to commemorate the seventh anniversary of this bug. I guess there's no progress to report, and there hasn't even been opinion expressed. (but thanks for your reply of last year, Shelley). I worked around this bug four years ago, and I guess everyone else did too. Regards, Hugh
Comment 55•18 years ago
|
||
It's a shame that a web standards bug that predates the release of Internet Epxlorer 6 is still not fixed...
Comment 56•18 years ago
|
||
Comment 57•18 years ago
|
||
I just went through the dependency list again and found these links to still be open and all related to positioning. Granted one is Firefox, but the underlying complaint is positioning of a block not going to the right of the parent. If I had more time, I would look at the other crossed out links. I would bet there are still other similiar links that are open. 19637 63895 217002 222220 203225 203229 243519 285871 314585 320865 340946
Comment 58•18 years ago
|
||
table is'nt only display type with this problem. The same problem is in display: inline-block elements, here is Example: http://atom.mamto.cz/projekty/opera/bug15/index2.html I tested also dev build of Firefox 3 which support inline-block but has the same problem with relative position. Why this is only minor problem I think, that this is very big problem. This work also in Explorer, and this is very usefull function for developer which use CSS layout.
Comment 59•17 years ago
|
||
I am having a problem that has the same results as test case 1. The only difference with my scenario is that I am not actually using a table but instead using a div with display:table; It seems that once you do that, the contents can no longer be positioned inside like they can when the containing element is display:block; It seems to me that the problem is that display:table; needs to still be recognized as a "block" element. My scenario actually works perfect in IE6 and IE7 which is usually not the case.
Comment 61•17 years ago
|
||
Copied from <http://lists.w3.org/Archives/Public/www-style/2007Sep/0187.html> (it seems to be reflected in Summary): On Wednesday 2007-09-26 21:31 -0500, Boris Zbarsky wrote: > Kornel Lesinski wrote: > >I think the latter: > > > >"The effect of 'position:relative' on table-row-group, > >table-header-group, table-footer-group, table-row, table-column-group, > >table-column, table-cell, and table-caption elements is undefined." > >http://www.w3.org/TR/CSS21/visuren.html#choose-position > > I don't recall that being there before. Must have been added so CSS21 can > actually exit CR... It was added in response to Issue #20 on the issues list for http://www.w3.org/TR/2003/WD-CSS21-20030915/ , which, given the lack of a URL, was presumably raised by Ian, and concerns what happens when the above elements get a stacking context (due to 'position:relative' or 'opacity') being undefined. -David
Comment 64•16 years ago
|
||
(In reply to comment #34) > (In reply to comment #17) > > 7624 // Is it positioned? > > 7625 // If it's a table then ignore it, because for the time being tables > > 7626 // are not containers for absolutely positioned child frames > > It's been there since the very beginning of that file. Looks like this is a > seriously dated decision, possibly pre-CSS1. As far as I kan se this points directly to the problem (the decision must have been done later). borderCollapse: and position: was defined by CSS2 (12-May-1998). Table attributes BORDER, CELLSPACING and CELLPADDING defined in HTML3.2 combined with CSS2.borderCollapse: introduced very complex rules for sharing space occupied by common margins, border and padding, possibly resulting in fractional px values. position: absolute complicated position calculations for tables further. CSS2 did not define all implications of this and was probably the reason for the defensive decision above. 10 years has gone since and the need for advanced dynamic web-applications has increased considerably, causing the problem to grow in significance and need for reconsideration. Geckoβs implementation for absolute positioning also gives nice opportunities not in IE, eg. it is possible to move a parent without moving children. This facility is not defined in any CSS as far as I know? What should happen to an object when parent moves? Would have been nice to have the possibility in CSS to set βmove_separateβ or βmove_with_parentβ Sorry if this is considered βadvocacyβ but I do`nt know where else to post it.
Comment 66•16 years ago
|
||
Hey fellas, its 2009 now and the 63895 bug is still going. Lets face it, if a web page viewer can not see the web page because its overlapped then there is a browser problem. (don't ya think?) I understand that Firefox is a volunteer project but I would have thought this would have been fixed 7 or 8 years ago. Do you think this will be fixed in the "near future" or should I go to crome or back to IE? Your call.
Comment 68•16 years ago
|
||
Just a note that I have encountered this bug in FF v3.0.10 while trying to implement a hover expansion of the contents of a table cell, like so: <td class="notworking"> <div class="note">blah yadda yadda </div> </td> with td.notworking { position: relative; width: 5em; height: 1em; } div.note { position: absolute; z-index: 0; width: 4em; } td.notworking:hover { top: -10em; left: -5em; z-index: 1; width: 40em; height: 20em; overflow-y: scroll; } [think I got that right-- untested] The hover works, sort of. But the position is relative to the div containing the entire table; it should be relative to the cell. A work-around with an added div in the cell gets what is intended-- at the cost of more mark-up and a fussier style sheet. I understand that the problem arose out of a design decision around 1999 or so, but with 10 years' advances in CSS and DOM, it would be good to revisit that decision.
Comment 70•16 years ago
|
||
(In reply to comment #68) > <td class="notworking"> > td.notworking { > position: relative; > } The effect of 'position:relative' on table-row-group, table-header-group, table-footer-group, table-row, table-column-group, table-column, **table-cell**, and table-caption elements is undefined. (from http://www.w3.org/TR/CSS2/visuren.html#choose-position) This means your note is not a valid bug.
Comment 71•16 years ago
|
||
Re #70 (re #68): No. The bug is still there. That the W3C has not provided a standard behavior in this situation does not mean that the current behavior is excusable. There are two ways of resolving this bug that I can see: 1. either implement the intuitively obvious behavior, OR 2. make it so that any attempt to use relative positioning within the listed elements (see #70) breaks completely, in the same way that a divide by zero error completely breaks a mathematical expression. The current approach of simply ignoring a position:relative attribute in these instances is just plain wrong. That leads to a partial fail condition where instead of the styling breaking completely on the undefined attribute, it mysteriously works in a totally unexpected way by failing silently and passing directives up to the containing block. On reflection, all instances like this where a stylist might call for something that is explicitly undefined in the standards should result in a total failure of the styling. And possibly an alert box...
Updated•15 years ago
|
Assignee: layout.r-and-a-pos → nobody
QA Contact: madhur → layout.r-and-a-pos
Comment 75•15 years ago
|
||
(In reply to comment #74) > *** Bug 558665 has been marked as a duplicate of this bug. *** Sorry about the comment dudes but...10 years should be enough to fix such an irritating bug.
Comment 76•14 years ago
|
||
What an incredibly fun and interesting read this is. I've just posted this bug on the forums (http://forums.mozillazine.org/viewtopic.php?f=25&t=1956209&p=9675587), only to be told it was reported 10 years ago. I love the "anniversary" posts! :) Imagine that.. a Firefox hack, well I'll be. I'm sure Firefox devs are the first to talk about "ugly IE hacks", IE this, IE that.. well the shoe is firmly on the other foot here. Chrome, Safari, Opera, IE.. all behave the same, but Firefox needs to be different. Why don't the devs want to touch this one? Too boring? Surely can't be complicated. Anyway, welcome to the age of the Firefox hack!
Comment 77•14 years ago
|
||
(In reply to comment #76) > Imagine that.. a Firefox hack, well I'll be. I'm sure Firefox devs are the > first to talk about "ugly IE hacks", IE this, IE that.. well the shoe is firmly > on the other foot here. Chrome, Safari, Opera, IE.. all behave the same, but > Firefox needs to be different. Why don't the devs want to touch this one? Too > boring? Surely can't be complicated. > > Anyway, welcome to the age of the Firefox hack!
Comment 78•14 years ago
|
||
Ouch, I did not mean to send an email, just to add myself to the CC list. Anyway, since I got there... (In reply to comment #76) > Imagine that.. a Firefox hack, well I'll be. Do you have a working hack for when the relatively positioned ancestor is supposed to be a table cell?
Updated•14 years ago
|
tracking-fennec: --- → ?
Comment 84•14 years ago
|
||
As of today, 25 other bugs have been retired by reporting them as duplicates of this 10 year old bug. That suggests there is probably a lot more frustration over this problem than the level shown in these comments. Do the votes for fixing this one get incremented properly when another bug report is declared to be a duplicate of this one? Currently there are 49 votes to get this fixed, but should that number be at least 74 (assuming each of the other retired bug reports got at least one vote?)
Updated•14 years ago
|
tracking-fennec: ? → 2.0-
Updated•14 years ago
|
tracking-fennec: 2.0- → 2.0next+
Updated•14 years ago
|
tracking-fennec: 2.0next+ → 6+
Comment 88•14 years ago
|
||
(In reply to comment #71) > The bug is still there. That the W3C has not provided a standard behavior in > this situation does not mean that the current behavior is excusable. There are > two ways of resolving this bug that I can see: > > 1. either implement the intuitively obvious behavior, OR > > 2. make it so that any attempt to use relative positioning within the listed > elements (see #70) breaks completely I agree. (In reply to comment #78) > Do you have a working hack for when the relatively positioned ancestor is > supposed to be a table cell? I too am wondering what the workaround for this is until this gets fixed.
Comment 89•14 years ago
|
||
(In reply to comment #88) > I too am wondering what the workaround for this is until this gets fixed. Never mind, I found the workaround: <td> <div style="position:relative; height: 100%"> <div style="position: absolute; top: -25px; left: -25px;"> Content </div> </div> </td> I should also note that Chromium also seems to ignore position:relative for td's.... not that that makes this behavior correct.
Comment 90•14 years ago
|
||
(In reply to comment #71) > The bug is still there. That the W3C has not provided a standard behavior in > this situation does not mean that the current behavior is excusable. Afaik the W3C is planning to revamp the display properties and split them up into display-inside and display-outside in some future version of CSS. For that they'll probably have to re-specify how the containing block is determined too. That might finally give us some clear behavior for this particular problem.
Comment 91•14 years ago
|
||
(In reply to comment #89) > (In reply to comment #88) > > I too am wondering what the workaround for this is until this gets fixed. > > Never mind, I found the workaround: > <td> > <div style="position:relative; height: 100%"> > <div style="position: absolute; top: -25px; left: -25px;"> > Content > </div> > </div> > </td> That's hardly a workaround, because the for example vertical-align doesn't work in those (as it does in everything table-cell).
Comment 92•14 years ago
|
||
I'm curious how jQuery can work out the position of a table cell within a table cell, if you can't tell what's relative to it? http://pastebin.com/u1bJfAcz Still, it does, so that at least lets us position things relative to a cell, even if we have to use a javascript to do it.
Updated•14 years ago
|
tracking-fennec: 6+ → 7+
Comment 93•13 years ago
|
||
My patches in bug 10209 and bug 659828 fix this. Here's the reftest based on the testcase in this bug.
Comment on attachment 538160 [details] [diff] [review] Reftest Review of attachment 538160 [details] [diff] [review]: -----------------------------------------------------------------
Attachment #538160 -
Flags: review?(roc) → review+
Comment 96•13 years ago
|
||
(In reply to comment #89) > Never mind, I found the workaround: > <td> > <div style="position:relative; height: 100%"> > <div style="position: absolute; top: -25px; left: -25px;"> > Content > </div> > </div> > </td> This didn't work for me in Firefox 5. However, setting the outside div to display: inline did work. I.e: <td> <div style="position: relative; display: inline;"> <img style="position: absolute; bottom: 10px;" src=... /> </div> </td>
Updated•13 years ago
|
tracking-fennec: 7+ → +
Comment 99•13 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #93) > Created attachment 538160 [details] [diff] [review] [diff] [details] [review] > Reftest > > My patches in bug 10209 and bug 659828 fix this. Here's the reftest based > on the testcase in this bug. Those bugs landed, so this is now fixed. I've also landed the test: https://hg.mozilla.org/mozilla-central/rev/f59216ae6553
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Future → mozilla10
Comment 100•13 years ago
|
||
Congratulations folks, and thanks, and I take back the snarks -- undeserved for sure. You've done a great job to keep tracking and pursuing this bug until you got it. Good work.
![]() |
||
Comment 101•13 years ago
|
||
Actually, this isn't fixed as filed. This bug included rows, cells, captions, etc, not just tables; lots of the duplicates are about those. And those are not fixed...
![]() |
||
Comment 102•13 years ago
|
||
In fact I think we should just reopen this bug, since it's tracking all the duplicates and dependencies for this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
![]() |
||
Updated•13 years ago
|
Summary: positioned table or internal table elements not abs pos containing block → positioned internal table elements not abs pos containing block
![]() |
||
Comment 104•13 years ago
|
||
This is actually a bit of a pain to do for table cells and rows, because of the size changes they do _after_ reflow... :(
![]() |
||
Updated•13 years ago
|
Severity: minor → normal
Priority: P4 → --
Target Milestone: mozilla10 → ---
![]() |
||
Comment 105•13 years ago
|
||
Comment 106•13 years ago
|
||
Interesting problem. Will someone kindly explain to a web author why cells are handled differently from a % width <div> or a <div> sized with flexbox? Thank you!
Comment 107•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #104) > This is actually a bit of a pain to do for table cells and rows, because of > the size changes they do _after_ reflow... :( They do that?! Wouldn't that break tons of other things? Also, should we use FinishAndReflowAbsoluteFrames here instead of ReflowAbsoluteFrames? Of course, depending on what really happens with those size changes, the answer might be no...
![]() |
||
Comment 108•13 years ago
|
||
I apparently missed comment 106 and 107 somehow. :( > Will someone kindly explain to a web author why cells are handled differently Because they have to have totally different behavior, mostly. ;) > They do that?! Absolutely. Row vertical alignment does that. > Wouldn't that break tons of other things? Apparently not. In terms of how to implement this... It seems to me that the right thing to do is to do the reflow of absolutely positioned stuff hanging off cells and rows after layout of the table as a whole finishes. The only question is what happens for scrollable cells, assuming those are supported.
Comment 109•13 years ago
|
||
Didn't read all the comments (I know...) but I encountered the same bug and found a possible workaround: http://jsfiddle.net/delphiki/ZjqMF/7/
Comment 110•13 years ago
|
||
(In reply to Julien Villetorte from comment #109) > Didn't read all the comments (I know...) but I encountered the same bug and > found a possible workaround: http://jsfiddle.net/delphiki/ZjqMF/7/ That workaround doesn't seem to work... it doesn't place the span on the bottom right corner of EACH cell
Comment 111•13 years ago
|
||
(In reply to Aldi from comment #110) > (In reply to Julien Villetorte from comment #109) > > Didn't read all the comments (I know...) but I encountered the same bug and > > found a possible workaround: http://jsfiddle.net/delphiki/ZjqMF/7/ > > That workaround doesn't seem to work... it doesn't place the span on the > bottom right corner of EACH cell That's because only one cell is on display:inline-block.
Comment 116•12 years ago
|
||
Updated•12 years ago
|
Attachment #669904 -
Attachment mime type: text/plain → text/html
Comment 117•12 years ago
|
||
For reference: test of already fixed case when offsetParent is table itself; last broken in Firefox 3.6 (?), OK now. Problem remains in relatively positioned cells, rows, etc. What about making bug 35168 duplicate of this one?
Updated•12 years ago
|
Attachment #670295 -
Attachment mime type: text/plain → text/html
Comment 118•12 years ago
|
||
This is definitely still a major bug. Just to put an extremely fine, and hopefully painful, point on just how bad this is, this bug was filed in the year 2000 and is still a problem. And even IE gets it right!
Comment 122•12 years ago
|
||
Wow you people are a mess. This bug has been around for 12 years!! are you kidding? It works in IE!! I can't believe that Firefox is now the browser that I am having to code around.
Comment 124•12 years ago
|
||
I just found out about this bug today as I noticed my layout looked all messed up in Firefox. I made a small reduced test case [1] to prove it, but apparently that's not really necessary, seeing that you've known about this already bug back in Netscape times... ^^ Anyway, I also took a ran the test case on browsershots, shows the problem pretty clearly. [2] [1] http://labs.avd.io/bugz/table-bug.html [2] http://browsershots.org/http://labs.avd.io/bugz/table-bug.html
Comment 125•12 years ago
|
||
same testcase available at http://jsfiddle.net/3Dsd2/1/ 12 years, and still counting ...
Attachment #715427 -
Attachment mime type: text/plain → text/html
Comment 126•12 years ago
|
||
Guys, please fix this problem, as it's breaking the web. I've run into this issue 6 or 7 times now over the past 4 years and the workarounds really stink - and they're only needed for THIS browser. Industry standards are often as important as W3C standards...
Comment 127•12 years ago
|
||
Guys, please fix this problem, as it's breaking the web. I've run into this issue 6 or 7 times now over the past 4 years and the workarounds really stink - and they're only needed for THIS browser. Industry standards are often as important as W3C standards...
Comment 129•12 years ago
|
||
Very frustrating. All the other browsers (including recent versions of IE) support this behaviour. Using βpositionβ to move table cells around is one thing, but using βposition: relativeβ (and βoverflow:hiddenβ) to create a new positioning context works everywhere else, and makes many layout tasks easier. [Iβm currently using a hack, β@-moz-document url-prefix() {β¦}β to work around this for Firefox, the first time Iβve had to target Mozilla with a hack ever.]
Comment 131•11 years ago
|
||
Really? 13 years and still no solution? Mozilla, Stop making an OS, prettifying your GUI, and taking photos with your mascot and fix this bug! Really, it's little things like this that keep me believing that Google really knows what they're doing and Mozilla doesn't.
Comment 134•11 years ago
|
||
Alex: as it will celebrate it's 12.5 year anniversary in 4 days time, I wouldn't hold my breath. I seriously doubt this will ever really get fixed...
Comment 135•11 years ago
|
||
Hello I performed some tests and I found a workaround, at least for my condition. Inside the TD I added a DIV with position:relative and I moved all my INPUT to inside that DIV. This way the INPUTs respected the absolute positioning inside each TD and I was successful to reach the layout I need. I hope this will be helpful to others! Alex
Comment 136•11 years ago
|
||
I'm not very aware of many techniques that make vertical-align: middle usable than setting divs to display: table-cell. The layout I'm using in this codepen works as expected in Chrome and Safari: http://codepen.io/amboy00/pen/jyvBu IMO, the positioned elements in FF don't work as expected.
Comment 137•11 years ago
|
||
As for as I know, at some point the similar positioning problem occured in the old Flexbox implementation (display: -moz-box etc.). My tests show that in the new Flexbox implementation (Firefox 22+) there is no such problem, flexible boxes with position:relative serve as containing blocks for their positioned content quite well. Since flexible box model and table model have some similarities, can't the experience of implementing positioning in flexible boxes be used in fixing this annoying bug?
Comment 141•11 years ago
|
||
(In reply to Josh S from comment #118) > this bug was filed in the year 2000 and is still a problem. Should we call it Firefox's Y2K bug?
Comment 142•11 years ago
|
||
You're kidding, right? It isn't funny. Probably I will be dead, when this bug will be fixed. What is "your tests"? Are you a preschool child? You can't read first sentence from first comment in this topic? I can do it for you http://jsfiddle.net/QtEgG/2/ Stop increasing versions. Fix bugs first!
Assignee | ||
Updated•11 years ago
|
Assignee: ehsan → seth
Comment 145•11 years ago
|
||
No Steven, Firefox does not "suck" Without Firefox, we'd still have an IE monopoly with all kinds of blatant standards-ignoring goodness to deal with. For the most part Firefox does a fantastic job rendering things as they should... there's only a few minor bugs from time to time, and they generally fix them in a reasonable timeframe. This one is just an exception in an otherwise outstanding product.
Comment 146•11 years ago
|
||
This exception brings down the entire product. Data-driven apps need very dynamic tables and Firefox fails at it. IE is pretty good nowadays, thanks to Firefox or not.
Comment 147•11 years ago
|
||
It seems unreal that a bug like this could still be around after almost 13 YEARS! Yet another demo: http://codepen.io/FStop/pen/GebwD Fails in nightly as well. Is the really a hard one to fix? Or is it just not important enough?
Comment 148•11 years ago
|
||
Please fix this, or I may have to redirect my Firefox traffic to the Chrome (or even IE10, eww) download page. Thanks.
![]() |
||
Comment 150•11 years ago
|
||
> Is the really a hard one to fix? Yes. It requires more or less completely redoing when absolutely positioned items are actually positioned... See comment 104.
Comment 151•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #150) > > Is the really a hard one to fix? > > Yes. It requires more or less completely redoing when absolutely positioned > items are actually positioned... See comment 104. Well it's about time it was fixed, eh? Literally every other browser does this properly, and I'd be willing to bet that this very bug has changed web design paradigms to work around it.
Comment 152•11 years ago
|
||
This is relatively high on our priority list, but I can't make any promises about when it will be fixed. Sorry for failing to recognize more quickly when this bug transitioned from being a bug where we were the only major browser that implemented enough features for it to make sense to even report it (which led it to be relatively low priority) to a bug that all of the other major browsers had fixed (which makes it high priority now).
Comment 155•11 years ago
|
||
Shouldn't this bug be tagged with [parity:ie] [parity:Safari] [parity:Chrome] [parity:opera]?
Comment 156•11 years ago
|
||
(In reply to Carlos AlΓ©n Silva from comment #155) > Shouldn't this bug be tagged with [parity:ie] [parity:Safari] > [parity:Chrome] [parity:opera]? If "parity" means the other browser doesn't have this bug, then yes. However, as far as I know, it means it's the same on other browsers. Just adding my 2 cents
Comment 158•11 years ago
|
||
Comment on attachment 538160 [details] [diff] [review] Reftest [Triage Comment]
Attachment #538160 -
Flags: approval-mozilla-b2g26+
Attachment #538160 -
Flags: approval-mozilla-b2g18+
Updated•11 years ago
|
Attachment #538160 -
Flags: approval-mozilla-b2g26+
Attachment #538160 -
Flags: approval-mozilla-b2g18+
Assignee | ||
Comment 159•11 years ago
|
||
This patch allows all internal table objects other than 'table-column's and 'table-column-group's to serve as absolute containing blocks for their descendants. This is implemented by tracking positioned internal table objects during frame construction and reflowing their absolutely positioned descendants at the end of the table reflow process.
Attachment #8368705 -
Flags: review?(dbaron)
Assignee | ||
Comment 160•11 years ago
|
||
These are positioning tests for all of the internal table objects that can now serve as absolute containing blocks.
Attachment #8368706 -
Flags: review?(dbaron)
Comment 161•11 years ago
|
||
Haven't had a chance to look much, but what does nsIFrame::eCanContainOverflowContainers have to do with this?
Assignee | ||
Comment 162•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #161) > Haven't had a chance to look much, but what does > nsIFrame::eCanContainOverflowContainers have to do with this? Without adding that bit, I experienced assertion failures and crashes when the absolutely positioned child ended up outside its parents.
Comment 163•11 years ago
|
||
Overflow containers are related to pagination of overflowing out-of-flows (floats, abs pos). So is stuff on those lists correctly managed, reflowed, and painted?
Comment 164•11 years ago
|
||
Oops, I see how it's related; it's worth making sure that you're doing all the things expected of frames that support overflow containers. I'm not sure off the top of my head what those are. That might be complicated enough that it's worth a separate patch or bug, but really not sure.
![]() |
||
Comment 165•11 years ago
|
||
Two questions about part 1: 1) Don't we need to unregister things if the child frames get destroyed before the table does? 2) Why are we checking whether the _parent_ supports transforms if the child is transformed?
Comment 166•11 years ago
|
||
Comment on attachment 8368705 [details] [diff] [review] (Part 1) - Support internal table objects as absolute containing blocks. >+ nsHTMLReflowState reflowState(aPresContext, this, >+ aReflowState.rendContext, >+ nsSize(size.width, size.height), >+ nsHTMLReflowState::DUMMY_PARENT_REFLOW_STATE); You almost certainly want NS_UNCONSTRAINEDSIZE rather than size.height; that seems likely to be why you're getting overflow containers (and incorrectly paginating). I suspect once that's fixed, doing pagination correctly can probably go to a separate bug.
Assignee | ||
Comment 167•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #165) > 1) Don't we need to unregister things if the child frames get destroyed > before the table does? Yeah, we do. I originally had code in there to do that but somehow it didn't make it into the patch. =\ > 2) Why are we checking whether the _parent_ supports transforms if the > child is transformed? Sounds like you're referring to the code in nsCSSFrameConstructor? Probably just a mistake. I'll take a look when I get a chance.
![]() |
||
Comment 168•11 years ago
|
||
> Sounds like you're referring to the code in nsCSSFrameConstructor?
Yes.
Assignee | ||
Comment 169•11 years ago
|
||
This updated patch should address all of the concerns that have been raised so far. It seems probable to me that the vast majority of the time, internal table objects will be getting destroyed at the same time as the table that contains them (for example, when we navigate away from a page) so I went ahead and optimized for that case. If the table itself is getting torn down, we don't bother updating the positioned objects list and let the table clear it out when it gets destroyed. This lets us elide a lot of potentially unnecessary mutation of the list.
Attachment #8372016 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8368705 -
Attachment is obsolete: true
Attachment #8368705 -
Flags: review?(dbaron)
Assignee | ||
Comment 170•11 years ago
|
||
Per today's discussion, make sure that we only store these properties on the first continuation frame of the nsTableFrame.
Attachment #8372711 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8372016 -
Attachment is obsolete: true
Attachment #8372016 -
Flags: review?(dbaron)
Assignee | ||
Comment 171•11 years ago
|
||
Try job here: https://tbpl.mozilla.org/?tree=Try&rev=4ba2abcd6bb8
Assignee | ||
Comment 172•11 years ago
|
||
Ah yes. That reminds me that I need to fix the author field on these patches.
Attachment #8372720 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8372711 -
Attachment is obsolete: true
Attachment #8372711 -
Flags: review?(dbaron)
Assignee | ||
Comment 173•11 years ago
|
||
Same here. Sorry for bugspam.
Attachment #8372721 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8368706 -
Attachment is obsolete: true
Attachment #8368706 -
Flags: review?(dbaron)
Assignee | ||
Comment 174•11 years ago
|
||
Add some helper methods to retrieve both visual and scrollable overflow areas in the frame's local coordinate space. This is needed to get correct overflow areas in part 2. Without it, we get some oranges due to asserts at nsFrame.cpp:6922 that "Computed overflow area must contain frame bounds", because GetOverflowAreas returns the overflow areas *after* CSS transforms are applied.
Attachment #8373778 -
Flags: review?(dbaron)
Assignee | ||
Comment 175•11 years ago
|
||
This new part 2 (previously part 1) is updated to use the new helper methods.
Attachment #8373779 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8372720 -
Attachment is obsolete: true
Attachment #8372720 -
Flags: review?(dbaron)
Assignee | ||
Comment 176•11 years ago
|
||
Just bumped the part number on this part.
Attachment #8373780 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8372721 -
Attachment is obsolete: true
Attachment #8372721 -
Flags: review?(dbaron)
Assignee | ||
Comment 177•11 years ago
|
||
Here's a new try job. The new patches I pushed just now should have taken care of all of the oranges revealed by the last try push, so hopefully this is nice and green. https://tbpl.mozilla.org/?tree=Try&rev=9b5fb38c3b32
Comment 178•11 years ago
|
||
Comment on attachment 8373778 [details] [diff] [review] (Part 1) - Add helper methods for retrieving overflow areas in the frame's local coordinate space. Could you make GetOverflowAreasRelativeToSelf look like GetScrollable/VisualOverflowRectRelativeToSelf instead of calling them, since then it will only do the property lookup once? r=dbaron with that
Attachment #8373778 -
Flags: review?(dbaron) → review+
Comment 179•11 years ago
|
||
Do you still need the eCanContainOverflowContainers changes in this patch (part 2)? I'd have expected those to be in bug 967870.
Assignee | ||
Comment 180•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #179) > Do you still need the eCanContainOverflowContainers changes in this patch > (part 2)? I'd have expected those to be in bug 967870. I don't think so; I'll upload a new version without them. By the way, this patch is leaking on some reftest, as you can see the from the try job. (Looks like it's leaking the nsTArray, which is to say, it's leaking the frame property. I'm very confused as to how that can happen.) I'm still tracking that down. It will get addressed before checkin.
Assignee | ||
Comment 182•11 years ago
|
||
Per comment 179, removed the eCanContainOverflowContainers changes.
Attachment #8375990 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8373779 -
Attachment is obsolete: true
Attachment #8373779 -
Flags: review?(dbaron)
Assignee | ||
Comment 183•11 years ago
|
||
Addressed the review comments. GetOverflowAreasRelativeToSelf will now do only one property lookup.
Assignee | ||
Updated•11 years ago
|
Attachment #8373778 -
Attachment is obsolete: true
Assignee | ||
Comment 184•11 years ago
|
||
This version of the patch fixes the leak and renames things so that "internal table objects" is replaced by "table parts".
Attachment #8376603 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8375990 -
Attachment is obsolete: true
Attachment #8375990 -
Flags: review?(dbaron)
Assignee | ||
Comment 185•11 years ago
|
||
OK, all known issues have been addressed, so unless more problems are uncovered during review I'd say we're good to go here. Just to be sure (and in particular to verify the leak is gone), here's one more try job. https://tbpl.mozilla.org/?tree=Try&rev=8a23d399de9e
Comment 186•11 years ago
|
||
Comment on attachment 8376603 [details] [diff] [review] (Part 2) - Support table parts as absolute containing blocks. nsCSSFrameConstructor: The code in nsCSSFrameConstructor should have comments about why it's setting the NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN more conservatively than other code that sets that bit -- in particular, because there's a registration/unregistration cost to having that bit set, so you only set it when the style says that it's a containing block, rather than only based on the frame's capability to have the child list. (I'm really not sure why the other code isn't more conservative, though.) I say this because the comment would warn somebody trying to "clean it up" later. The repeated pattern: aDisplay->HasTransformStyle() && newFrame->IsFrameOfType(nsIFrame::eSupportsCSSTransforms) can be replaced with aDisplay->HasTransform(newFrame) (It wouldn't be unreasonable to assert that styleContext->GetStyleDisplay() == aDisplay as well. I'm sure assuming it is, and it appears HasTransform also asserts it.) nsTableFrame: Could you change aMustPassThru -> aMustPassThrough ? In GetTableFramePassingThrough, you can (and I think should) drop the boolean hitPassThruFrame by swapping the order of the two ifs inside the loop, and just returning null if you hit the pass-through frame. Simpler code, and less looping. >+typedef nsTArray<nsIFrame*> nsIFrameArray; Please call this FrameTArray; things starting with nsI look like interfaces. >+ auto positionedObjs = static_cast<nsIFrameArray*>(aPropertyValue); I don't think we can use auto yet, though I might be wrong. I suppose if it passed try on all platforms then maybe we can? There's a cleaner way to do this: >+ FramePropertyTable* props = tableFrame->PresContext()->PropertyTable(); >+ void* positionedPartsProp = >+ props->Get(tableFrame, nsTableFrame::PositionedTablePartArray()); ... >+ props->Set(tableFrame, >+ nsTableFrame::PositionedTablePartArray(), >+ positionedParts); which is (using FrameProperties, which is a thin wrapper that does this): FrameProperties props = tableFrame->Properties(); void* positionedPartsProp = props.Get(PositionedTablePartArray()); ... props.Set(PositionedTablePartArray(), positionedParts); Similar in UnregisterPositionedTablePart. >+ // If there are any positioned table parts, we need to reflow their >+ // absolutely-positioned descendants now that their dimensions are final. "positioned table parts" -> "relatively-positioned table parts", since there's no such thing as absolutely-positioned table parts, and so it makes things a little clearer. FixupPositionedTableParts: >+ FramePropertyTable* props = PresContext()->PropertyTable(); >+ void* positionedPartsProp = >+ props->Get(this, nsTableFrame::PositionedTablePartArray()); >+ auto positionedParts = static_cast<nsIFrameArray*>(positionedPartsProp); This should be just one line: auto positionedParts = static_cast<FrameTArray*>(Props().Get(PositionedTablePartArray()); >+ if (positionedParts) { Instead of indenting, return early if (!positionedParts). >+ for (size_t i = 0 ; i < positionedParts->Length() ; ++i) { Local style has no space before either semicolon. >+ auto positionedPart = static_cast<nsFrame*>(positionedParts->ElementAt(i)); Why the cast? I'd drop it and just do: nsIFrame* positionedPart = positionedParts->ElementAt(i); If you need a cast somewhere later, do that explicitly; this code makes it look to a casual reader like you're dealing with a container type that holds void*, which you're not. >+ reflowState.mCBReflowState = &aReflowState; It seems odd to set mCBReflowState on a reflow state with a null parent. Is this necessary? >+ // Reflow absolutely-positioned descendants of the positioned part. >+ overflowTracker.AddFrame(positionedPart); It probably makes sense to do this only if the overflow area has changed. Could you add a "FIXME:" comment pointing out that both the NS_UNCONSTRAINEDSIZE for the height and ignoring any change to the reflow status aren't actually correct (and possibly also the reflow status's initial value), and mean we'll never paginate these absolutely-positioned frames. It might not be a bad idea if bz has a quick look at the frame constructor changes. r=dbaron with the above changes, though
Attachment #8376603 -
Flags: review?(dbaron)
Attachment #8376603 -
Flags: review+
Attachment #8376603 -
Flags: feedback?(bzbarsky)
Comment 187•11 years ago
|
||
Comment on attachment 8373780 [details] [diff] [review] (Part 3) - Add tests for positioned internal table objects serving as absolute containing blocks. Maybe rename all the table-* to in-table-*, because you're testing positioning in a table-* rather than of it? I'm a little skeptical of the value of the prose descriptions in the tests; they make it seem like that's a sufficient pass condition -- do you really think the prose conditions are sufficient? If not, maybe remove them... although I know the CSS WG likes them (though they're not otherwise in CSS WG format)? (If there were a spec that said what should happen here, I'd have suggested adding the relevant CSS WG metadata and putting the tests in layout/reftests/w3c-submitted/ instead... but there isn't.) r=dbaron on these, although I was expecting to see more coverage of right and bottom (with either auto width/height or auto top/left, although the former kills two birds with one stone). I think you need some test coverage of that as well. (Or am I missing it?)
Attachment #8373780 -
Flags: review?(dbaron) → review+
![]() |
||
Comment 188•11 years ago
|
||
Comment on attachment 8376603 [details] [diff] [review] (Part 2) - Support table parts as absolute containing blocks. The frame constructor changes look fine to me, modulo the existing comments.
Attachment #8376603 -
Flags: feedback?(bzbarsky) → feedback+
Assignee | ||
Comment 189•11 years ago
|
||
Thanks for the review! I'll upload a new patch shortly with these issues addressed. A few comments below: (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #186) > I don't think we can use auto yet We can! See here: https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code > >+ reflowState.mCBReflowState = &aReflowState; > > It seems odd to set mCBReflowState on a reflow state with a null parent. > > Is this necessary? Sadly (since I agree it'd be preferable to avoid it), it does seem to be necessary. Tons of assertions are generated without it. > >+ // Reflow absolutely-positioned descendants of the positioned part. > >+ overflowTracker.AddFrame(positionedPart); > > It probably makes sense to do this only if the overflow area has changed. I'm surprised that the overflow tracker doesn't do the right thing automatically! Will double check.
Assignee | ||
Comment 190•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #186) > Comment on attachment 8376603 [details] [diff] [review] > In GetTableFramePassingThrough, you can (and I think should) drop > the boolean hitPassThruFrame by swapping the order of the two ifs > inside the loop, and just returning null if you hit the pass-through > frame. Simpler code, and less looping. Heh, this is a good point. Looking back at the code, all of the stuff you mention existed just to support the assert at the bottom of the function. =)
Assignee | ||
Comment 191•11 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #189) > > >+ reflowState.mCBReflowState = &aReflowState; > > > > It seems odd to set mCBReflowState on a reflow state with a null parent. > > > > Is this necessary? > > Sadly (since I agree it'd be preferable to avoid it), it does seem to be > necessary. Tons of assertions are generated without it. ... Or not. I just checked and it isn't needed. This was probably another issue that was solved by switching to NS_UNCONSTRAINEDSIZE for the reflow state available height.
Assignee | ||
Comment 192•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #187) > Comment on attachment 8373780 [details] [diff] [review] > Maybe rename all the table-* to in-table-*, because you're testing > positioning in a table-* rather than of it? Well, the names of the tests are just the names of the display values/frame types that are being tested. > I'm a little skeptical of the value of the prose descriptions in the > tests; they make it seem like that's a sufficient pass condition -- do > you really think the prose conditions are sufficient? If not, maybe > remove them... although I know the CSS WG likes them (though they're > not otherwise in CSS WG format)? Well, I agree that they're not really a sufficient pass condition, but the tests are designed in such a way that the most likely failure mode (the abs-pos child is positioned relative to the wrong absolute containing block) will cause the failure that the text describes. It's good to know for you're not a big fan of this approach, as it took more work to do it this way (particularly while sharing the references between the tests for the different table parts). However, since I've already done that work, I'm inclined to leave the text in. =) > r=dbaron on these, although I was expecting to see more coverage of > right and bottom (with either auto width/height or auto top/left, > although the former kills two birds with one stone). I think you need > some test coverage of that as well. (Or am I missing it?) The reason I didn't write tests that involved right and bottom was because I thought of these tests as testing two issues: (1) Is the abs-pos child positioned relative to the correct absolute containing block? (2) Is the abs-pos child positioned relative to the *final position* of the correct absolutely containing block, after all table reflow passes are complete? It seemed to me that usage of right/bottom wasn't relevant to those issues, and there were already a lot of tests. Do you think that coverage of right and bottom *is* relevant to those issues? Or am I just looking at the scope of these tests too restrictively? I'll needinfo you and if necessary add a part 4 with additional tests.
Flags: needinfo?(dbaron)
Assignee | ||
Comment 193•11 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #192) > Well, I agree that they're not really a sufficient pass condition, but the > tests are designed in such a way that the most likely failure mode (the > abs-pos child is positioned relative to the wrong absolute containing block) > will cause the failure that the text describes. Actually, in terms of the two issues described in the later part of comment #192, it's also true that issue 2 will cause the failure that the text describes in those tests where the problem could occur.
Assignee | ||
Comment 194•11 years ago
|
||
Addressed review comments. The only change I made here that is not discussed in the review comments above is that I pulled the repeated code for registering positioned table parts into a separate static function. After the additional comments were added it started to feel like too big and perhaps too subtle a block of code to be repeated.
Assignee | ||
Updated•11 years ago
|
Attachment #8376603 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8378650 -
Attachment is obsolete: true
Assignee | ||
Comment 196•11 years ago
|
||
Pushed parts 1-3 to inbound. https://hg.mozilla.org/integration/mozilla-inbound/rev/6609365ee797 https://hg.mozilla.org/integration/mozilla-inbound/rev/80bd36a3d4f1 https://hg.mozilla.org/integration/mozilla-inbound/rev/02e057fea56c
Assignee | ||
Updated•11 years ago
|
Attachment #8375992 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #8378656 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #8373780 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #566883 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #538160 -
Flags: checkin+
(In reply to Seth Fowler [:seth] from comment #196) > Pushed parts 1-3 to inbound. > > https://hg.mozilla.org/integration/mozilla-inbound/rev/6609365ee797 > https://hg.mozilla.org/integration/mozilla-inbound/rev/80bd36a3d4f1 > https://hg.mozilla.org/integration/mozilla-inbound/rev/02e057fea56c Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/969a7e228596 for debug crashtest failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=34947236&tree=Mozilla-Inbound
Flags: needinfo?(seth)
Assignee | ||
Comment 198•11 years ago
|
||
Sadface! The problem was that nsCSSFrameConstructor was creating continuation frames that had NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN set (because they snarfed it from their previous continuation, via nsFrame::Init) but *weren't actually registered* with nsTableFrame. Will upload a patch shortly that fixes this. I've verified the fix locally, but will push another try job to make sure everything has shaken out.
Flags: needinfo?(seth)
Assignee | ||
Comment 199•11 years ago
|
||
Boris, sorry to add to your review load, but would you mind giving the frame construction changes another look? In particular, the changes to CreateContinuingTableFrame and CreateContinuingFrame are totally new in this version of the patch.
Attachment #8378801 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•11 years ago
|
Attachment #8378656 -
Attachment is obsolete: true
Assignee | ||
Comment 200•11 years ago
|
||
Try job for the new patch here: https://tbpl.mozilla.org/?tree=Try&rev=b97a4a3d8daa
Updated•11 years ago
|
Attachment #8378801 -
Flags: feedback?(dbaron)
Flags: needinfo?(dbaron)
Comment 201•11 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #192) > > r=dbaron on these, although I was expecting to see more coverage of > > right and bottom (with either auto width/height or auto top/left, > > although the former kills two birds with one stone). I think you need > > some test coverage of that as well. (Or am I missing it?) > > The reason I didn't write tests that involved right and bottom was because I > thought of these tests as testing two issues: > > (1) Is the abs-pos child positioned relative to the correct absolute > containing block? > (2) Is the abs-pos child positioned relative to the *final position* of the > correct absolutely containing block, after all table reflow passes are > complete? > > It seemed to me that usage of right/bottom wasn't relevant to those issues, > and there were already a lot of tests. > > Do you think that coverage of right and bottom *is* relevant to those > issues? Or am I just looking at the scope of these tests too restrictively? I think it is relevant, in that the containing block has four edges, and some of those edges might not move in the later stages of reflow, and it's worth having some test coverage of the right/bottom edges of the containing block.
![]() |
||
Comment 203•11 years ago
|
||
Comment on attachment 8378801 [details] [diff] [review] (Part 2) - Support table parts as absolute containing blocks. r=me. Sorry for the lag. :(
Attachment #8378801 -
Flags: review?(bzbarsky) → review+
Comment 204•11 years ago
|
||
Comment on attachment 8378801 [details] [diff] [review] (Part 2) - Support table parts as absolute containing blocks. >+ // If there are any relatively-positioned table parts, we need to reflow their >+ // Retrieve the table frame, and ensure that we hit aMustPassThrough on the way. watch the 80th column violations. Otherwise looks good.
Attachment #8378801 -
Flags: feedback?(dbaron) → feedback+
Comment 205•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #204) > Comment on attachment 8378801 [details] [diff] [review] > (Part 2) - Support table parts as absolute containing blocks. > > >+ // If there are any relatively-positioned table parts, we need to reflow their > > >+ // Retrieve the table frame, and ensure that we hit aMustPassThrough on the way. > > watch the 80th column violations. > > Otherwise looks good. What do you mean "looks good"? Was this put out in Nightly? It's still broken for me.
Comment 206•11 years ago
|
||
It will be in nightlies generated when the bug is marked fixed, which it is not now.
Assignee | ||
Comment 207•11 years ago
|
||
Thanks for the reviews, folks! All known issues are taken care of now, so I went ahead and pushed to inbound again: https://hg.mozilla.org/integration/mozilla-inbound/rev/ceafe92c7716 https://hg.mozilla.org/integration/mozilla-inbound/rev/0199299ac111 https://hg.mozilla.org/integration/mozilla-inbound/rev/d78110a5cffa
Assignee | ||
Comment 208•11 years ago
|
||
Marking as leave-open until the final set of tests gets in.
Keywords: leave-open
Assignee | ||
Updated•11 years ago
|
Attachment #8378801 -
Flags: checkin+
Comment 209•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ceafe92c7716 https://hg.mozilla.org/mozilla-central/rev/0199299ac111 https://hg.mozilla.org/mozilla-central/rev/d78110a5cffa
![]() |
||
Updated•11 years ago
|
Keywords: dev-doc-needed
![]() |
||
Comment 210•11 years ago
|
||
If we're not sure we'll land the tests for 30, might be good to spin that off to a separate bug.
Keywords: relnote
Assignee | ||
Comment 211•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #210) > If we're not sure we'll land the tests for 30, might be good to spin that > off to a separate bug. I currently don't see any reason why we shouldn't, but I agree that a separate bug is appropriate if it looks like that might happen.
Assignee | ||
Comment 212•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #186) > In GetTableFramePassingThrough, you can (and I think should) drop > the boolean hitPassThruFrame by swapping the order of the two ifs > inside the loop, and just returning null if you hit the pass-through > frame. Simpler code, and less looping. Doh, this is not actually equivalent, though I didn't immediately see the problem. This actually ends up returning null if we hit the pass-through frame, which is exactly the opposite of the intended behavior! I've reverted this change, which will fix bug 980223, in my local version of the patch. Will upload a new version shortly.
Assignee | ||
Comment 213•11 years ago
|
||
Backed out these patches until bug 980075 is resolved. https://hg.mozilla.org/integration/mozilla-inbound/rev/4d2e89e1ea3d
Assignee | ||
Comment 214•11 years ago
|
||
This makes the change mentioned in comment 212, resolving bug 980223. (Though in some sense it's resolved anyway by the backout...)
Assignee | ||
Updated•11 years ago
|
Attachment #8378801 -
Attachment is obsolete: true
Assignee | ||
Comment 215•11 years ago
|
||
(Sorry, I intended to provide this as a separate patch, but it ended up being trickier than I thought due to some Mercurial wonkiness.) In addition to the changes mentioned in comment 212, this version of part 2 adds a warning during frame construction when we see a non-cell table part with relative positioning applied. The warning only appears in DEBUG builds (which, to my knowledge, includes Nightly) so it should be very low-impact. The relevant code is in nsTableFrame::RegisterPositionedTablePart.
Attachment #8387996 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8387244 -
Attachment is obsolete: true
Assignee | ||
Comment 216•11 years ago
|
||
Fixed some issues that came up in offline discussion with dbaron. I no longer wrap the warning code in #if DEBUG since Nightly does not set -DDEBUG. (And it's not clear we want Nightly-only anyway.) Instead, to minimize the performance impact, I added a flag to the nsPresContext to ensure that we only report the error once. I experimented with setting the category argument to ReportToConsole to different values, including "CSS", but it made no difference; the web console always categorizes the message under JS. I tracked down this problem and filed bug 981241, but I don't think it needs to block this bug.
Attachment #8388006 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8387996 -
Attachment is obsolete: true
Attachment #8387996 -
Flags: review?(dbaron)
Comment 217•11 years ago
|
||
Comment on attachment 8388006 [details] [diff] [review] (Part 2) - Support table parts as absolute containing blocks. Maybe change "Your site" to "This site" in the error message? r=dbaron
Attachment #8388006 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 218•11 years ago
|
||
Replaced "Your site" with "This site". Also, moved the error message's location string back to the layout properties file, since it looks like bug 981241 will make this nice very soon.
Assignee | ||
Updated•11 years ago
|
Attachment #8388006 -
Attachment is obsolete: true
Comment 219•11 years ago
|
||
(In reply to Seth Fowler [:seth] from comment #218) > Also, moved the error message's > location string back to the layout properties file, since it looks like bug > 981241 will make this nice very soon. The category and the properties file used aren't related, though. But it seems like a reasonable change anyway.
Assignee | ||
Comment 220•11 years ago
|
||
Bug 980075 is now resolved, so relanding these patches: https://hg.mozilla.org/integration/mozilla-inbound/rev/3a3f3f3dddbc https://hg.mozilla.org/integration/mozilla-inbound/rev/677d07d6cadb https://hg.mozilla.org/integration/mozilla-inbound/rev/84e4b2ecb19b
Assignee | ||
Comment 221•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #219) > The category and the properties file used aren't related, though. But it > seems like a reasonable change anyway. I thought it might be more intuitive for people looking for the string, but yeah, it didn't really matter.
Assignee | ||
Updated•11 years ago
|
Attachment #8388835 -
Flags: checkin+
Comment 222•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3a3f3f3dddbc https://hg.mozilla.org/mozilla-central/rev/677d07d6cadb https://hg.mozilla.org/mozilla-central/rev/84e4b2ecb19b
Assignee | ||
Comment 223•11 years ago
|
||
Writing the tests in part 4 uncovered another bug - OverflowChangedTracker does not propagate overflow all the way to the subtree root (the table frame), but only to its children, contrary to my assumption. This caused us to fail to render absolutely positioned children of relatively positioned table parts when they ended being placed above or to the left of the table frame. This patch fixes this problem by updating the overflow areas of the table frame after we apply the relative positioning. This will need to be uplifted to Aurora once it sticks to m-c.
Attachment #8392651 -
Flags: review?(dbaron)
Assignee | ||
Comment 224•11 years ago
|
||
This patch adds additional tests which cover positioning by 'bottom' and 'right'. This complements the previous set of tests, which cover positioning by 'top' and 'left'. This will need to be uplifted to Aurora once it sticks to m-c.
Attachment #8392652 -
Flags: review?(dbaron)
Assignee | ||
Comment 225•11 years ago
|
||
Here's a try job for parts 4 and 5 against current m-c: https://tbpl.mozilla.org/?tree=Try&rev=ff773525fa1b
Assignee | ||
Comment 226•11 years ago
|
||
Removed leave-open because parts 4 and 5 represent all of the remaining work for this bug.
Keywords: leave-open
Comment 227•11 years ago
|
||
Comment on attachment 8392651 [details] [diff] [review] (Part 4) - Propagate overflow of relatively positioned table parts to the table frame. >+ >+ // Update our own overflow areas. (OverflowChangedTracker doesn't update the >+ // subtree root itself.) >+ nsLayoutUtils::UnionChildOverflow(this, aDesiredSize.mOverflowAreas); Before you do this, you should do aDesiredSize.SetOverflowAreasToDesiredBounds(), because you want to discard the previous unioning of the children. This is testable, for example, with: <td style="position:relative; top: -50px"> <div style="height: 50px"> <div style="height: 100px"> </div> </div> </td> and testing that the table with this td at the bottom doesn't cause a scrollbar to scroll to where the cell would have been if it hadn't been relatively positioned. (I believe that's consistent with the way we treat overflow and relative positioning elsewhere, but it's worth checking before following my advice.) r=dbaron with that
Attachment #8392651 -
Flags: review?(dbaron) → review+
Comment 228•11 years ago
|
||
Comment on attachment 8392652 [details] [diff] [review] (Part 5) - Add more tests for positioned internal table objects serving as absolute containing blocks. Looks like some of the history is recorded as copies and some isn't; is that intentional? Maybe more should be recorded as hg copies? (I think hg cp -A will work.) r=dbaron
Attachment #8392652 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 229•11 years ago
|
||
Looked into dbaron's suggestion in comment 227 and it makes sense to me. Updated the patch.
Assignee | ||
Updated•11 years ago
|
Attachment #8392651 -
Attachment is obsolete: true
Assignee | ||
Comment 230•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC+8, but slow response this week) from comment #228) > Looks like some of the history is recorded as copies and some isn't; is that > intentional? Maybe more should be recorded as hg copies? (I think hg cp -A > will work.) `hg cp -A` doesn't seem to work with patch queues, sadly. Thanks for the reviews! Looks like this is ready to go.
Assignee | ||
Comment 231•11 years ago
|
||
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/0320aabdedc5 https://hg.mozilla.org/integration/mozilla-inbound/rev/d2bc5ad54215 Will request uplift once these hit m-c.
Assignee | ||
Updated•11 years ago
|
Attachment #8393294 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #8392652 -
Flags: checkin+
Comment 232•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0320aabdedc5 https://hg.mozilla.org/mozilla-central/rev/d2bc5ad54215
Status: REOPENED → RESOLVED
Closed: 13 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee | ||
Updated•11 years ago
|
Target Milestone: mozilla31 → mozilla30
Assignee | ||
Comment 233•11 years ago
|
||
Comment on attachment 8393294 [details] [diff] [review] (Part 4) - Propagate overflow of relatively positioned table parts to the table frame. [Approval Request Comment] Bug caused by (feature/regressing bug #): 63895 User impact if declined: Rendering and invalidation errors when relative positioning is used with table parts. Since this is a new feature in FF, it'd be preferable to avoid ever exposing web content to these problems. Testing completed (on m-c, etc.): On m-c for a day with no reported issues. Risk to taking this patch (and alternatives if risky): Low risk. This is a very small patch that is unlikely to introduce new issues. String or IDL/UUID changes made by this patch: None. (Note that I understand I don't need to request approval for test-only patches. I'll be uplifting part 5, which contains the tests for this issue, along with this patch.)
Attachment #8393294 -
Flags: approval-mozilla-aurora?
Comment 234•11 years ago
|
||
TM tracks trunk landing. Status flags track branch uplifts.
status-firefox30:
--- → affected
status-firefox31:
--- → fixed
Target Milestone: mozilla30 → mozilla31
Assignee | ||
Comment 235•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #234) > TM tracks trunk landing. Status flags track branch uplifts. Fair enough, although 3/5 of this bug landed on 30. =)
![]() |
||
Comment 236•11 years ago
|
||
Yeah, hence comment 210... It was a suggestion to prevent that sort of confusion.
Updated•11 years ago
|
Attachment #8393294 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 237•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/eef4866eba7b https://hg.mozilla.org/releases/mozilla-aurora/rev/d4248a320f3e
Comment 238•11 years ago
|
||
Reproduced the issues on Nightly 2009-10-15. Verified fixed using the testcases on 31.0a1 (2014-03-25), win 7 x64.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 239•11 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #236) > Yeah, hence comment 210... It was a suggestion to prevent that sort of > confusion. Yeah, in retrospect I should've done that. I didn't expect the tests to reveal new issues to be debugged, but that was naive. =\
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•