Closed Bug 1650189 Opened 3 months ago Closed 3 months ago

Transitions not working when specifying 'all' and an exception property with 0s duration

Categories

(Core :: CSS Transitions and Animations, defect)

77 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla80
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- verified
firefox77 --- wontfix
firefox78 --- wontfix
firefox79 --- verified
firefox80 --- verified

People

(Reporter: till, Assigned: emilio)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Attached file Transition bug

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

See the attached test case. Hover the box.

Actual results:

When hovering the box, both width and height jump, even though transitions are set for all properties to 1s, and only height is set to 0s. Specifying a very small duration for the height instead (e.g. 1ms) makes the test case basically work as expected.

Expected results:

When hovering the box, the width should transition smoothly while the height should jump.

This seems to be broken since Firefox 77, it worked before and it works in other browsers.

Severity: -- → S3
Status: UNCONFIRMED → NEW
Ever confirmed: true
QA Whiteboard: [qa-regression-triage]

Emilio, are you interested in looking into this?

Flags: needinfo?(emilio)

Sure thing. CC'ing mrobinson who authored most of those patches too, just in case he can get to take a look sooner than me.

I can confirm reverting https://hg.mozilla.org/integration/autoland/rev/89d28e030ba6eff042c8a572393a1455b295a853 fixes it, which is a bit odd, but not all that surprising from that set of changes I guess.

Digging a bit on why...

Ouch... silly mistake in that patch, surprised no test caught it before, really :(

Assignee: nobody → emilio

Till, how do you want to be credited for the test-case? Usually we do stuff like <link rel="author" href="mailto:emilio@crisal.io" title="Emilio Cobos Álvarez">, but I'd understand if you didn't want to have a mailto: link in a public test suite, so I could just use the http://mellthas.de link, or just don't mention you altogether, your call.

Thanks for filing and for the excellent test-case btw :)

Flags: needinfo?(till)

By the time we get to iterate over the longhands of a shorthand, we've
already advanced the range iterator, so we look at the next duration and
such, which causes this bug.

I'm seriously baffled that no existing test caught this when it
landed, neither in our internal test suite nor wpt... :/

I went for the http:// link for now but I'll update before or after landing if appropriate.

Flags: needinfo?(emilio)

Comment on attachment 9161641 [details]
Bug 1650189 - Fix an off-by-one in the transition property iterator. r=mrobinson,birtles

Beta/Release Uplift Approval Request

  • User impact if declined: Some transitions will use the wrong duration.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: comment 0
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Fixes a silly mistake that apparently is hard to hit in practice.
  • String changes made/needed: none
Attachment #9161641 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9161641 [details]
Bug 1650189 - Fix an off-by-one in the transition property iterator. r=mrobinson,birtles

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Simple fix for CSS transitions regression in 77.
  • User impact if declined: See comment 0, some CSS transitions will use the wrong duration.
  • Fix Landed on Version:
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Simple fix + test.
  • String or UUID changes made by this patch: none
Attachment #9161641 - Flags: approval-mozilla-esr78?

(In reply to Emilio Cobos Álvarez (:emilio) (PTO until Jul 10) from comment #6)

Till, how do you want to be credited for the test-case? Usually we do stuff like <link rel="author" href="mailto:emilio@crisal.io" title="Emilio Cobos Álvarez">, but I'd understand if you didn't want to have a mailto: link in a public test suite, so I could just use the http://mellthas.de link, or just don't mention you altogether, your call.

Thanks for filing and for the excellent test-case btw :)

The credit with the link is fine, thanks!

Flags: needinfo?(till)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a98d6188d72
Fix an off-by-one in the transition property iterator. r=mrobinson
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/24462 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80

Comment on attachment 9161641 [details]
Bug 1650189 - Fix an off-by-one in the transition property iterator. r=mrobinson,birtles

Approved for 79.0b5. Thanks for including a test.

Attachment #9161641 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

(In reply to Emilio Cobos Álvarez (:emilio) (PTO until Jul 10) from comment #7)

I'm seriously baffled that no existing test caught this when it
landed, neither in our internal test suite nor wpt... :/

Yes, me too!

Thanks for fixing this so quickly!

QA Whiteboard: [qa-regression-triage] → [qa-regression-triage][qa-triaged]
Upstream PR merged by moz-wptsync-bot

Reproduced the initial issue on Firefox 78.0.1.
Verified as fixed using the latest Nightly 80.0a1 (Build ID: 20200707094747) on Ubuntu 18.04, Mac OS X 10.15, and Windows 10 x64.

Comment on attachment 9161641 [details]
Bug 1650189 - Fix an off-by-one in the transition property iterator. r=mrobinson,birtles

Approved for 78.1esr.

Attachment #9161641 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+

Verified as fixed also on the latest Firefox 79 beta 5 and on Firefox 78.1.0 esr (on a Treeherder build, Build ID: 20200707210843), on Ubuntu 18.04, Mac OS X 10.15 and Windows 10 x64.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-regression-triage][qa-triaged] → [qa-regression-triage]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.