1224251 - Changing row opacity on hover if table has border-collapse: collapse caused background to bleed through if cursor moves fast enough

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Christian Sonne [:cers]]

Attachment: testcase

This bug has a few factors that seem to influence it.

1) table has border-collapse: collapse
2) row has a background
3) row opacity changes on :hover
4) row background does not change on :hover

If all of the above are met, and the cursor is moves fast enough (if it lands on a row with the non :hover opacity, the bug stays visible after movement has stopped - I will attach video of this), the background bleeds through.

I've tested this as far back as I could get Firefox to launch, which is around Firefox 4, and the bug has been there in all tested versions (including latest Nightly)

I've tested and found the bug on two machines running OS X (El Capitan), but not found it on any of three machines running linux. I have not tested on a Windows machine.

STR:
1) load attached testcase
2) move cursor over table

Expected result:
Row opacity changes for currently hovered row

Actual results:
Background bleeds through (and sometimes permanently)

Comment 1

[Christian Sonne [:cers]]

Attachment: Video showing the bug in effect

Another bug can be seen in this video too, but this will be filed separately.

Comment 2

[Christian Sonne [:cers]]

The bug also appears in Windows 7 (Firefox 42)

Comment 3

[Christian Sonne [:cers]]

Turns out this bug is also present in the "No bug" section of the test in Firefox versions up until 18. That's the version where the related bug 1224253 was introduced.

Comment 4

[Christian Sonne [:cers]]

For what it's worth, the appears to have started after Firefox 3.6.28 and is present in Firefox 4

Comment 5

[Christian Sonne [:cers]]

The bug appears between 4.0b1 and 4.0b2 which narrows the window to
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2010-06-30&enddate=2010-07-21

That's still a pretty long list, but my best guess would be that it's fallout from bug 564991

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I think the problem here is that we send an nsChangeHint_UpdateOpacityLayer change hint for the change between 0.8 and 1.0.  However, since opacity on a row has changed between 1.0 and non-1.0, two things have changed:

 (a) what gets painted by the nsDisplayTableBorderBackground display item for the table (since the row and its cells are no longer part of it)

 (b) whether the row gets an nsDisplayTableRowBackground display item

(Note that the bug does not occur changing between 0.8 and 0.95.)

The processing of the nsChangeHint_UpdateOpacityLayer doesn't call InvalidateFrameSubtree or InvalidateFrame.  I don't believe DLBI is intended to pick up these changes (although I'm really not sure what we do about geometry changes within tables -- probably something slow!), so I think (given the messy table background painting setup) that a special case for internal table parts (including cells) in handling of UpdateOpacityLayer might fix this.  (Or even a new hint for changes between opacity 1.0 and non-1.0, since I think that's the only time it's needed.)

Then again, I don't think anything that fancy is needed, and we can just convert UpdateOpacityLayer on internal table parts into RepaintFrame.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I have patches that fix both this and bug 1224253.  I'm having trouble getting an automated test that works, though; I have a test that shows the bug when I load it in the browser, but doesn't in the reftest harness (either watching it or according to the snapshot).

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I was thinking last night that that might have been because I was testing in a configuration without hardware acceleration (remote X), but that isn't the case; the test also fails to fail without the patch on try.

I also realize that the test I have currently is more a test for bug 1224253 than the symptoms here.  Though I'm not convinced we need automated tests for both.


(Fundamentally, the problem here is that tables are an unusual case.  Roughly, the backgrounds on all the parts of a table (down to the cells) is associated with the table in our code; unless the part or something between it and the table has opacity != 1.  So switching between 1 and non-1 opacity was failing to repaint one of the halves that it should, in fact, have repainted -- either the layer associated with the table or the layer associated with the row -- and depending on which direction the change was (1 to 0.8 or 0.8 to 1) you could end up with the row background in neither layer or in both layers rather than just in the one it was supposed to be in.)

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 - Add reftests

Locally, these tests fail reliably for me without the patch.  I'm not,
however, confident that they would continue to do so, across future
mutations in the code, were the bug to be reintroduced, given the amount
of fiddling I had to go through to get them to fail without the patch.

That is, without the patch:

  table-row-opacity-dynamic-1.html shows the area of the row that's not
  behind the text as a much more opaque blue than it should be (while
  the area of the row that is behind the text is correct)

  table-row-opacity-dynamic-2.html shows the area of the row that's not
  behind the text as the white background showing through (while the
  area of the row that is behind the text is correct)

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 - Add nsChangeHint_UpdateUsesOpacity to say when opacity changes between 1 and non-1

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3 - Return nsChangeHint_UpdateUsesOpacity when opacity changes between 1 and non-1

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4 - Convert UpdateOpacityLayer to RepaintFrame when changing opacity between 1 and non-1 on table parts

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, thanks for the clear bug report on this problem.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

It seems to me this issue happens because changing opacity between 1 and non-1 changes whether the element creates a stacking context, which consequently affects whether the background is painted as part of the nsDisplayTableBorderBackground or their own display item.

But changing whether it should create a stacking context looks like a bigger thing, which generally should always triggers a nsChangeHint_RepaintFrame, shouldn't it?

It seems opacity currently works fine otherwise in rendering order, so I suppose UpdateOpacityLayer somehow triggers RepaintFrame as well when the switch happens. Could you explain why that doesn't work with table parts here? (Probably that only invalidate the subtree, but not its ancestor table frame? But it seems to me RepaintFrame only invalidates the subtree as well?)

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

If RepaintFrame is always desirable in this case, we probably don't need to introduce a new change hint bit with the complexity to the RestyleManager?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #14)
> It seems to me this issue happens because changing opacity between 1 and
> non-1 changes whether the element creates a stacking context, which
> consequently affects whether the background is painted as part of the
> nsDisplayTableBorderBackground or their own display item.
> 
> But changing whether it should create a stacking context looks like a bigger
> thing, which generally should always triggers a nsChangeHint_RepaintFrame,
> shouldn't it?
> 
> It seems opacity currently works fine otherwise in rendering order, so I
> suppose UpdateOpacityLayer somehow triggers RepaintFrame as well when the
> switch happens. Could you explain why that doesn't work with table parts
> here? (Probably that only invalidate the subtree, but not its ancestor table
> frame? But it seems to me RepaintFrame only invalidates the subtree as well?)

So if you look at DoApplyRenderingChangeToTree, currently the only difference between UpdateOpacityLayer and RepaintFrame is whether we call aFrame->InvalidateFrameSubtree().  We call aFrame->SchedulePaint(nsIFrame::PAINT_DEFAULT) for both.

SchedulePaint(PAINT_DEFAULT) will, I believe (though Matt can confirm), rerun display list building and do any invalidation needed based on display list differences that DLBI detects, which I believe includes anything where we're constructing different display items or putting them in a different stacking order.  This is sufficient for normal opacity changes.

But tables are special as I described in comment 6.  In particular, there's no separate display item for the row's background, and no display item at all associated with the row.  We could add something complicated to handle that, but our existing RepaintFrame handling appears to be sufficient, although I'm not familiar enough with the details to explain the whole process.  But I think part of it is that by calling aFrame->InvalidateFrameSubtree(), we (via InvalidateFrame -> InvalidateFrameInternal) set NS_FRAME_NEEDS_PAINT, which makes nsIFrame::IsInvalid and in turn nsDisplayItem::IsInvalid return true, which causes what we need to be repainted get repainted.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #15)
> If RepaintFrame is always desirable in this case, we probably don't need to
> introduce a new change hint bit with the complexity to the RestyleManager?

I'm not sure what complexity you're talking about.  But the new hint is really very little code; it's some changes to constants, a comparison in nsStyleDisplay::CalcDifference, and the test to handle it (by swapping 2 other bits) in RestyleManager.cpp.  I think it is worth not doing this extra work for animations of opacity, but only doing it when we swap between 1 and non-1.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

I meant, why shouldn't we just set RepaintFrame when we switch between 1 and non-1, so that we don't need an additional flag and code to handle that flag? It doesn't seem to me doing so could cause significant performance issue. (It actually seems funny to me that we are doing more things for switching between 0.99 and 1 than between 0.8 and 1.)

It also seems to me we have some plan to optimize opacity like transform that we do not always schedule paint for its change (in bug 796697, not active though). With that optimization, we may eventually still need to add RepaintFrame flag where you are currently adding your new flag.

Given these reasons, I suggest we probably should just use RepaintFrame in that case, which looks simpler and safer.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #17)
> Given these reasons, I suggest we probably should just use RepaintFrame in
> that case, which looks simpler and safer.

In what sense is it safer?  Changing opacity on things that aren't parts of tables is quite well tested and is widely used, and I'd rather not make it slower right now without any benefit.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #18)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #17)
> > Given these reasons, I suggest we probably should just use RepaintFrame in
> > that case, which looks simpler and safer.
> 
> In what sense is it safer?

In sense that we may apply the same optimization of transform layer to opacity in the future, so that UpdateOpacityLayer does not guarantee scheduling a paint. Actually it seems to me the name "Update*Layer" should not be assumed to guarantee a paint.

> Changing opacity on things that aren't parts of
> tables is quite well tested and is widely used, and I'd rather not make it
> slower right now without any benefit.

Well, the benefit is that we do not need to add additional hint bit and code to do something which can be done in an even simpler way without significant performance penalty. I don't really believe picking that way could strongly slow it down. (Especially given the code for switching between 0.99 and 1.) And simpler logic in CalcDifference and ProcessRestyledFrames might even benefit general cases.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #19)
> (In reply to David Baron [:dbaron] ⌚UTC-8 from comment #18)
> > (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #17)
> > > Given these reasons, I suggest we probably should just use RepaintFrame in
> > > that case, which looks simpler and safer.
> > 
> > In what sense is it safer?
> 
> In sense that we may apply the same optimization of transform layer to
> opacity in the future, so that UpdateOpacityLayer does not guarantee
> scheduling a paint. Actually it seems to me the name "Update*Layer" should
> not be assumed to guarantee a paint.

If we want to go in that direction, I think we'd still want the additional hint.  A change on opacity between 1 and non-1 shouldn't cause us to repaint things inside of descendant elements' layers.  For table rows, I'm not worried about that, but if we wanted to do that in the general case I would be.

The code for switching between 0.99 and 1 is unfortunate, and hopefully can be improve on in the future.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8692694 [details] [diff] [review]
patch 2 - Add nsChangeHint_UpdateUsesOpacity to say when opacity changes between 1 and non-1

Review of attachment 8692694 [details] [diff] [review]:
-----------------------------------------------------------------

OK. I'm not totally convinced this flag is worthwhile, but if you insist that we want to go in this direction even if we may no longer ensure a paint for UpdateOpacityLayer in the future, I guess it's fine. The style part itself looks fine anyway.

::: layout/base/nsChangeHint.h
@@ +188,5 @@
> +   * Indicates that the 'opacity' property changed between 1 and non-1.
> +   *
> +   * Used as extra data for handling UpdateOpacityLayer hints.
> +   */
> +  nsChangeHint_UpdateUsesOpacity = (1 << 25),

Probably better follow the convention above and use 0x2000000 instead for now. I guess it may be worth a separate patch to convert all the flags to use the "1 << X" form.

Also it may worth a comment that changes between [0.99, 1) and 1 would not have this flag because we don't handle them with the UpdateOpacityLayer hint.

Comment 22

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8692693 [details] [diff] [review]
patch 1 - Add reftests

Review of attachment 8692693 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/table-background/table-row-opacity-dynamic-2.html
@@ +31,5 @@
> +
> +var tr = document.getElementsByTagName("tr")[0];
> +
> +// Not sure why, but to make this test fail without the patch, this has to
> +// be a setTimeout(0) rather than waiting for MozReftestInvalidate.

Is this because the test is changing the opacity from 1 -> 0.8 -> 1, and the reftest harness only updates the snapshot within the invalidated area? If we don't correctly invalidate the broken area, then it won't get updated in the snapshot?

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #22)
> Is this because the test is changing the opacity from 1 -> 0.8 -> 1, and the
> reftest harness only updates the snapshot within the invalidated area? If we
> don't correctly invalidate the broken area, then it won't get updated in the
> snapshot?

I don't think that explains it, because then it would still appear broken, as it needs to change in order to get a pass result.  (The test is changing it 0.8 -> 0.8 -> 0.8 -> 1, I think since it needs to poke the activity timers.)  Or maybe I'm misunderstanding.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70ed5feb6d47&group_state=expanded

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52f9c007281d973138d8b91a1a5fe6a3af717cd0
Bug 1224251 patch 1 - Add reftests.  r=mattwoodrow

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c4a581885e47d0d5a0a864525298ffd3c4c29e0
Bug 1224251 patch 2 - Add nsChangeHint_UpdateUsesOpacity to say when opacity changes between 1 and non-1.  r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/91898a35b414661dc596abec59714b1085a80dd0
Bug 1224251 patch 3 - Return nsChangeHint_UpdateUsesOpacity when opacity changes between 1 and non-1.  r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a2d8e926a66b22ac7f0e132248cc5c635335351
Bug 1224251 patch 4 - Convert UpdateOpacityLayer to RepaintFrame when changing opacity between 1 and non-1 on table parts.  r=mattwoodrow

Comment 26

[Nigel Babu [:nigelb]]

The reftest you added is perma-orange on Windows 8.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36a37b7451a56bee00cce0c318a8c5a485fd370e
Bug 1224251 followup - Annotate one of the new reftests as fuzzy on Windows and Mac.

Comment 28

[Carsten Book [:Tomcat]]

sorry had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/196f2f131c70 since something in this push caused https://treeherder.mozilla.org/logviewer.html#?job_id=17983411&repo=mozilla-inbound

Comment 29

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee86c86bb46
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce162173fe84
https://hg.mozilla.org/integration/mozilla-inbound/rev/5906a7d9254d
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9c91b65209

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So the bit I'm currently stumped about is this:
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/svg/svg-integration/clipPath-html-06.xhtml | image comparison (==), max difference: 255, number of differing pixels: 30
REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/svg/svg-integration/clipPath-html-06-extref.xhtml | image comparison (==), max difference: 255, number of differing pixels: 30 
which I've confirmed on try is triggered by patch 4 in this bug.

See these two try runs for comparison (although they are on different base revisions):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2451d42aa37&group_state=expanded
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e68c460bf13e&group_state=expanded

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... though they seem to have gone away in another more-recent try push that does include patch 4.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I also pushed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b73eabe9e2f4&group_state=expanded&selectedJob=14288977
and it seems like the failure has just gone away.

(I now suspect it may have been related to the additional test causing changes in test chunking.)

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fee42e9c11b9cdd85bb6bf3e1ed0927eca83404
Bug 1224251 patch 1 - Add reftests.  r=mattwoodrow

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d9165f61d39008e1e73c4dbf0f5933dbc34c59b
Bug 1224251 patch 2 - Add nsChangeHint_UpdateUsesOpacity to say when opacity changes between 1 and non-1.  r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/7093bae2bb25d84bbdd12301eefaa6247e75048e
Bug 1224251 patch 3 - Return nsChangeHint_UpdateUsesOpacity when opacity changes between 1 and non-1.  r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dde6025527fbf1c44b2ef6cdcfdd7062c51e24a
Bug 1224251 patch 4 - Convert UpdateOpacityLayer to RepaintFrame when changing opacity between 1 and non-1 on table parts.  r=mattwoodrow

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I was thinking test chunking might be a good explanation for the failure, but it doesn't seem like it.  The first test did shift slightly, but the failing test was about 10% of the way in to the chunk in the opt R14, and 33% in in the debug R40.

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8fee42e9c11b
https://hg.mozilla.org/mozilla-central/rev/6d9165f61d39
https://hg.mozilla.org/mozilla-central/rev/7093bae2bb25
https://hg.mozilla.org/mozilla-central/rev/9dde6025527f