-moz-columns reacts poorly to lists with text-shadow in :hover pseudo-class

NEW
Unassigned

Status

()

Core
Layout
6 years ago
2 years ago

People

(Reporter: Andre Ryan, Unassigned)

Tracking

({regression, testcase})

17 Branch
x86_64
Windows 7
regression, testcase
Points:
---

Firefox Tracking Flags

(firefox17 affected, firefox18+ wontfix, firefox19+ wontfix, firefox20- affected, firefox21 affected, firefox22 affected)

Details

Attachments

(9 attachments, 2 obsolete attachments)

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
mats
: review+
Details | Diff | Splinter Review
2.28 KB, patch
mats
: review+
Details | Diff | Splinter Review
11.81 KB, patch
mats
: review-
Details | Diff | Splinter Review
8.86 KB, patch
mats
: review+
Details | Diff | Splinter Review
735 bytes, text/html
Details
(Reporter)

Description

6 years ago
Created attachment 684823 [details]
Test case with weird column reflows during hover

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.

Updated

6 years ago
Attachment #684823 - Attachment mime type: text/plain → text/html

Updated

6 years ago
Component: Untriaged → Layout
Product: Firefox → Core

Comment 1

6 years ago
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: --- → ?
Keywords: regression, regressionwindow-wanted, testcase

Updated

6 years ago
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
Keywords: regressionwindow-wanted
Blocks: 616722
No longer depends on: 616722

Comment 3

6 years ago
New web regression in Firefox 17. Let's try to knock this out for Firefox 18.
Assignee: nobody → matspal
tracking-firefox18: ? → +
tracking-firefox19: ? → +
tracking-firefox20: ? → +

Comment 4

5 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

5 years ago
Spoke with Mats about column bugs this morning. Over to him for the diagnosis...
Assignee: bugs → matspal

Comment 6

5 years ago
Mats: please update with your findings, and/or a recommendation on backing out bug 616722.

Updated

5 years ago
Duplicate of this bug: 823451
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.
Created attachment 705390 [details] [diff] [review]
backout bug 616722

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)
Created attachment 705392 [details] [diff] [review]
Disable part of column-balancing-nested-001.html

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.
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.
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox22: --- → affected
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.
tracking-firefox20: + → -
Created attachment 722575 [details]
Simplified testcase
Assignee: matspal → roc
Created attachment 723284 [details]
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.
Created attachment 723285 [details]
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.
Created attachment 723407 [details] [diff] [review]
Part 1: Change DEBUG_roc to DEBUG_COLUMNS
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)
Created attachment 723408 [details] [diff] [review]
Part 2: Don't try subpixel balance heights for no good reason

Not really needed for this bug, but I noticed it while debugging.
Attachment #723408 - Flags: review?(matspal)
Created 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

This is the real fix. I hate adding to nsHTMLReflowState but it's the simplest way...
Attachment #723410 - 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.
Created attachment 723955 [details]
Testcase #5

This test still flickers when hovering the links at certain window widths.

Updated

5 years ago
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+
You need to log in before you can comment on or make changes to this bug.