Open
Bug 814811
Opened 12 years ago
Updated 2 years ago
-moz-columns reacts poorly to lists with text-shadow in :hover pseudo-class
Categories
(Core :: Layout: Columns, defect)
Tracking
()
People
(Reporter: AndreRyan_ptb, Unassigned)
References
Details
(Keywords: regression, testcase)
Attachments
(9 files, 2 obsolete files)
1.88 KB,
text/html
|
Details | |
342 bytes,
text/html
|
Details | |
327 bytes,
text/html
|
Details | |
360 bytes,
text/html
|
Details | |
8.19 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
2.28 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
11.81 KB,
patch
|
MatsPalmgren_bugz
:
review-
|
Details | Diff | Splinter Review |
8.86 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
735 bytes,
text/html
|
Details |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.64 Safari/537.11 Steps to reproduce: Created a list inside a DIV. DIV has "-moz-columns: 2". List is full of links with "a:hover { text-shadow: 2px 2px 1px #666 }" style. Tried hovering over links in the right column. Columns "dance" as the first item in column 2 alternates to the end of column 1 then back to the start of column 2 over and over. Hovering over item 9 causes an infinite bounce cycle without having to move the mouse at all. Actual results: The text-shadow seems to trigger a recalculation of the columns. This causes the items to be repositioned and moved between columns which is extremely unhelpful for link hovering. The item with the shadow also usually is not the one that is actually under the mouse. This seems to imply that text shadows are affecting layout somehow, which they are explicitly not supposed to do. Expected results: The application of text-shadow should not cause the column items to be rearranged.
Attachment #684823 -
Attachment mime type: text/plain → text/html
It's a regression in Firefox 17. m-c good=2012-07-20 bad=2012-07-21 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3a05d298599e&tochange=446b788ab99d
status-firefox17:
--- → affected
tracking-firefox18:
--- → ?
tracking-firefox19:
--- → ?
tracking-firefox20:
--- → ?
Comment 2•12 years ago
|
||
The first bad revision is: changeset: 99958:8ad2c23374bc user: Mats Palmgren <matspal@gmail.com> date: Fri Jul 20 14:27:04 2012 -0400 summary: Bug 616722 - Make ColumnSetFrame a block margin root. Make the fir st column a margin root at the top edge, and the last column at the bottom. r=r oc
Depends on: 616722
Keywords: regressionwindow-wanted
Updated•12 years ago
|
Comment 3•12 years ago
|
||
New web regression in Firefox 17. Let's try to knock this out for Firefox 18.
Assignee: nobody → matspal
Comment 4•11 years ago
|
||
Jet - can you help find an assignee to resolve in the FF19 timeframe? Or would you mind making a recommendation for untracking this web regression? Thanks!
Assignee: matspal → bugs
status-firefox18:
--- → wontfix
Comment 5•11 years ago
|
||
Spoke with Mats about column bugs this morning. Over to him for the diagnosis...
Assignee: bugs → matspal
Comment 6•11 years ago
|
||
Mats: please update with your findings, and/or a recommendation on backing out bug 616722.
Comment 8•11 years ago
|
||
The root of the problem is making the last column a bottom margin root. http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#7001 This makes column-balancing confused. I tried to fiddle with it but I don't see a simple solution. I think we should back out bug 616722 for now... I'm preparing a backout patch.
Comment 9•11 years ago
|
||
Fortunately this code hasn't changed much, the only (minor) conflicts were in NS_NewColumnSetFrame where a null-check on 'it' had been removed and in the tests were "-moz-column-fill: auto;" had been added. Reverting nsBlockFrame/nsBlockReflowContext had no conflicts at all.
Attachment #705390 -
Flags: review?(roc)
Comment 10•11 years ago
|
||
The backout above caused a hang in column-balancing-nested-001.html (a test that is newer than 616722). I'm not sure exactly why, but we have quite a few bugs filed on hangs in column layout so I'm assuming it's one of those (e.g. bug 730559).
Attachment #705392 -
Flags: review?(roc)
Comment 11•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=76c2528a4c0e
Why is text-shadow having any effect on layout, though?
Comment 13•11 years ago
|
||
A text-shadow style change triggers reflow: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#2818 http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#2730 for the <a>, the nearest ancestor reflow root is the ViewportFrame. Before bug 616722 the ColumnSetFrame would rebalance resulting in the same feasible height. After bug 616722 it toggles between H and H-1 twip.
We should be able to fix the text-shadow problem with UpdateOverflow and not reflowing. I guess we have outstanding issues with UpdateOverflow that prevent that? I think we should try to fix this bug for real rather than just backing out.
Comment 15•11 years ago
|
||
Given the fact that FF19b4 will come and go today and this has been a longstanding issue, wontfixing for FF19.
status-firefox19:
--- → wontfix
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14) > We should be able to fix the text-shadow problem with UpdateOverflow and not > reflowing. I guess we have outstanding issues with UpdateOverflow that > prevent that? If we want to do this, we should add an additional hint. The current UpdateOverflow hint (which could be renamed, perhaps to UpdateParentOverflow) means that an element's pre-transform overflow is unchanged, but its effect on its parent's overflow has changed (possibly though not necessarily by changing its post-transform overflow). So if we want this, we should add a new hint that says that an element's *own* overflow has changed (maybe UpdateSelfOverflow). > I think we should try to fix this bug for real rather than just backing out. I'm hoping that by this, you mean that we should ensure that columns don't change their layout as a result of a no-op reflow.
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #16) > I'm hoping that by this, you mean that we should ensure that columns don't > change their layout as a result of a no-op reflow. Yes.
Updated•11 years ago
|
Comment 18•11 years ago
|
||
After checking with Roc in email, this is not a serious enough issue to deserve tracking, it can be uplifted when fixed if low risk.
Assignee: matspal → roc
This testcase illustrates the core problem better. When we get to placing "line 3", nsBlockReflowContext::PlaceBlock doesn't know about the parent block's bottom-margin so it thinks "line 3" fits just below "line 2" in the second column. In my previous testcase, that makes us decide everything fits with a balance height for 40px, but other code thinks that's not true (because it isn't) and the resulting confusion causes the behavior seen in this bug.
It's not just from ancestors: PlaceBlock doesn't take into account the margin being carried out of the placed block, either. So basically we assume that a margin below a block can always be thrown away if it exceeds the available height. That's not true for content at the end of the block formatting context.
Actually the problem is more general. When placing a block (or any line I guess) that's the last child of its parent, we may have to place the padding, borders or (collapsed) margin of the parent (or other ancestors) immediately after the block. So we should be taking that into account when we decide whether there's enough room to place the block. But we don't.
Attachment #705390 -
Attachment is obsolete: true
Attachment #705392 -
Attachment is obsolete: true
Attachment #705390 -
Flags: review?(roc)
Attachment #705392 -
Flags: review?(roc)
Attachment #723407 -
Flags: review?(matspal)
Not really needed for this bug, but I noticed it while debugging.
Attachment #723408 -
Flags: review?(matspal)
This is the real fix. I hate adding to nsHTMLReflowState but it's the simplest way...
Attachment #723410 -
Flags: review?(matspal)
Attachment #723411 -
Flags: review?(matspal)
After these patches are in, I want to go back and rip out the "carried out bottom margin" stuff in nsColumnSetFrame, since we never carry out bottom margins from content in a column set. Also I'm pretty sure DescendIntoBlockLevelFrame in nsBlockReflowContext.cpp can be removed.
Comment 28•11 years ago
|
||
This test still flickers when hovering the links at certain window widths.
Updated•11 years ago
|
Attachment #723407 -
Flags: review?(matspal) → review+
Comment 29•11 years ago
|
||
Comment on attachment 723408 [details] [diff] [review] Part 2: Don't try subpixel balance heights for no good reason >+ // XXX NS_BLOCK_MARGIN_ROOT seems to be pointless here since this isn't a >+ // block? > it->AddStateBits(aStateFlags | NS_BLOCK_MARGIN_ROOT); Yeah, please remove it. >+ // Add 1 to knownInfeasibleHeight so we're averaging the minimum >+ // and maximum feasible height. If the content has >+ // a pixel-aligned height, knownInfeasibleHeight will typically be >... The comment lines seems a bit unbalanced, please rewrap this comment.
Attachment #723408 -
Flags: review?(matspal) → review+
Comment 30•11 years ago
|
||
Comment on attachment 723410 [details] [diff] [review] Part 3: Allow nsHTMLReflowState to propgate information about extra height that will need to be added if the current frame is complete r- because it appears to regress test #5. > nsBlockFrame::ReflowBlockFrame >+ if (aState.GetFlag(BRS_ISBOTTOMMARGINROOT)) { >+ blockHtmlRS.mExtraBottomIfFrameIsComplete += >+ aState.mReflowState.mComputedBorderPadding.bottom + >+ bottomMargin.get(); >+ } else { Since you "apply" the bottom margin here, shouldn't you Zero() some mExtraBottomMarginIfFrameIsComplete? or is that implicit by not propagating the aState.mReflowState margin to blockHtmlRS, and aState.mReflowState.mExtraBottomMarginIfFrameIsComplete isn't used again anywhere? > nsBlockFrame::PlaceLine >+ // If this is the last line of the block, account for extra space >+ // needed by the margins/border/padding on this block and ancestors nit: Please end the sentence with a full stop.
Attachment #723410 -
Flags: review?(matspal) → review-
Comment 31•11 years ago
|
||
Comment on attachment 723411 [details] [diff] [review] Part 4: tests Is there a reason not to submit these tests to the CSS-WG test suite?
Attachment #723411 -
Flags: review?(matspal) → review+
Assignee: roc → nobody
Updated•6 years ago
|
Component: Layout → Layout: Columns
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•