Closed Bug 1287122 Opened 8 years ago Closed 8 years ago

Opacity transitions are affected by the presence of a filter.

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
50 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox49 --- unaffected
firefox50 + verified
firefox51 --- verified
firefox52 --- verified

People

(Reporter: johngraciliano, Assigned: mattwoodrow)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

Testcase with filter + opacity
1.64 KB, text/html
Details
Make sure we build the opacity item if we need it.
1.77 KB, patch
dbaron
: review+
ritu
: approval-mozilla-aurora+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0 Build ID: 20160714030208 Steps to reproduce: Start Firefox Nightly 50.0a1 (2016-07-13). (or later, some earlier versions maybe) Start the 'Developer Tools'. Go to the Options and select the 'Light' theme. Switch to the 'Inspector' tool. Move the 'mouse pointer' from one side to the other of the toggle pane button. Actual results: The button opacity increases and remains high after the pointer has passed the button. Expected results: The the button opacity should go from 0.8 (normal) to 1.0 (while hover) and return to the original 0.8 value. I found similar behavior problems with all inverted buttons in the Light theme and also in the Firebug theme. The problem affects the icons that have a filter on the default state. At times, some buttons may have only the opacity shifted in only part of the image! The problem can also be tested with the following code: <html> <style> #center { filter: contrast(1); opacity: .5; } #center:hover { opacity: 1.0; } </style> <body> Left - <span id=center>Center</span> - Right </body> </html> If the pointer is moved quickly (taking less than .3s) from 'Left' to 'Right' the 'Center' text will turn and remain dark. The problem does not happen if the movement is slow (taking more than 1.5s). The weird behavior will not happen if the opacity was high for the normal state but low on the hover state. The problem will not happen if there is no filter on the normal state, even if the hover state had a filter.
Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=067521ccd17907843e1db644ca0f02a1f5a6520a&tochange=eac0c056235ee1174fc092fe159d2d5710331fbd apparently a regression from bug 1283827.
Blocks: 1283827
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Testcase used while bisecting. A. filter + opacity: 0.5 => opacity: 1.0 B. opacity: 0.5 => opacity: 1.0 C. opacity: 0.5 => filter + opacity: 1.0 D. filter + opacity: 0.5 => filter + opacity: 1.0 E. filter + opacity: 1.0 => opacity: 0.5 F. opacity: 1.0 => opacity: 0.5 G. opacity: 1.0 => filter + opacity: 0.5 H. filter + opacity: 1.0 => filter + opacity: 0.5 opacity remains 1.0 when mouse moves out in A and D, opacity remains 1.0 when mouse hovers 2nd time in E, G, and H.
Matt, can you look into this? (Or should we find someone else to?) Nothing jumped out at me looking at the patch...
Flags: needinfo?(matt.woodrow)
status-firefox50: --- → affected
Assignee: nobody → matt.woodrow
Depends on: 1287654
Flags: needinfo?(matt.woodrow)
The useOpacity variable already takes SVG effects (like filters into account), and it can still be true even with SVG effects (like when we want to use async animations). I don't think we can easily test this (since triggering it requires us to change style in the middle of the transition), but hopefully bug 1287654 will help with that.
Attachment #8774197 - Flags: review?(dbaron)
[Tracking Requested - why for this release]: This fix is needed to actually fix bug 1281428, which is tracking-firefox50+, and causes user-visible issues on the WebRTC device sharing indicator (implemented in bug 1275262 for Firefox 50).
Blocks: 1281428
tracking-firefox50: --- → ?
Comment on attachment 8774197 [details] [diff] [review] Make sure we build the opacity item if we need it. r=dbaron, although I don't really follow at a detailed level what's going on here (although I can see that this being an else combined with the way useOpacity is set doesn't quite make sense). (Perhaps we need to build up new reviewers other than you who do understand this code thoroughly? Or maybe we have some and you're just asking the wrong person? :-)
Attachment #8774197 - Flags: review?(dbaron) → review+
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #6) > Comment on attachment 8774197 [details] [diff] [review] > Make sure we build the opacity item if we need it. > > r=dbaron, although I don't really follow at a detailed level what's going on > here (although I can see that this being an else combined with the way > useOpacity is set doesn't quite make sense). > > (Perhaps we need to build up new reviewers other than you who do understand > this code thoroughly? Or maybe we have some and you're just asking the > wrong person? :-) mstange or tnikkel might have been a better choice in hindsight, I chose you as it was a regression from a bug that you reviewed.
Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d2701591470 Make sure we build nsDisplayOpacity if we need it, since useOpacity already takes svg effects into account. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/4d2701591470
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Matt, could you please request aurora approval here, as this was a regression in Firefox 50? Thanks for fixing this :-).
Flags: needinfo?(matt.woodrow)
Thank you!
Comment on attachment 8774197 [details] [diff] [review] Make sure we build the opacity item if we need it. Approval Request Comment [Feature/regressing bug #]: bug 1283827 [User impact if declined]: Broken opacity animations when combined with SVG filters. [Describe test coverage new/current, TreeHerder]: Tested manually. [Risks and why]: Very low risk, just reverts a bit of the regressing bug to the old behaviour. [String/UUID change made/needed]: None.
Flags: needinfo?(matt.woodrow)
Attachment #8774197 - Flags: approval-mozilla-aurora?
Tracked as it's a recent regression in Fx50.
tracking-firefox50: ? → +
Hello John, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(johngraciliano)
Comment on attachment 8774197 [details] [diff] [review] Make sure we build the opacity item if we need it. Fixes a recent regression, Aurora50+
Attachment #8774197 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The problem is fixed in the latest nightly. It is not fixed in Aurora yet. It will be tested easily if there is any timed transition. It happens right away in such case.
Afterword: I did more experiments about this problem and it was: "When a filter is present, if there are two consecutive changes in opacity with little time between them, the opacity will become 1 on the second change." In particular, when there is a filter, if a (timed) transition is specified for a (single) change in the opacity, the value will become and stay as 1 immediatelly (very soon) after the transition is triggered (because it calls for a fast succession of changes). Note the values used for the opacity in the code have nothing to do with the final result, 1. Due to changes in the Developer Tools' themes, the problem later affected the 'dark' theme of Firefox 50+ and not the 'light' or the 'firebug' themes as I stated when I posted the bug. It is good this is just history now.
Verified as fixed on Nightly 52.0a1 -20160920030429, Aurora 51.0a2 - 20160920004004 and Beta 50.0b1-20160920155715.
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
status-firefox51: fixed → verified
status-firefox52: --- → verified
Flags: needinfo?(johngraciliano)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: