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)

17 Branch
x86_64
Windows 7
defect

Tracking

()

Tracking Status
firefox17 --- affected
firefox18 + wontfix
firefox19 + wontfix
firefox20 - affected
firefox21 --- affected
firefox22 --- affected

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
Component: Untriaged → Layout
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Blocks: 616722
No longer depends on: 616722
New web regression in Firefox 17. Let's try to knock this out for Firefox 18.
Assignee: nobody → matspal
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
Spoke with Mats about column bugs this morning. Over to him for the diagnosis...
Assignee: bugs → matspal
Mats: please update with your findings, and/or a recommendation on backing out bug 616722.
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.
Attached patch backout bug 616722 (obsolete) — Splinter Review
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)
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)
Why is text-shadow having any effect on layout, though?
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.
Given the fact that FF19b4 will come and go today and this has been a longstanding issue, wontfixing for FF19.
(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.
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.
Attached file even simpler testcase
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.
Attached file another testcase
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)
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.
Attached file Testcase #5
This test still flickers when hovering the links at certain window widths.
Attachment #723407 - Flags: review?(matspal) → review+
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 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 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+
Component: Layout → Layout: Columns
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: