Closed Bug 1492567 Opened 2 years ago Closed 2 years ago

Back out bug 1481866.

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file)

No description provided.
See bug 1481866 Comment 14 as for why.
The behavior the WG proposed is way more subtle than what that bug implements,
including:

 * Implementing two logical overflow longhands.
 * Expanding the overflow shorthand to different longhands depending on the
   syntax of that.

Meanwhile, Blink hasn't done the swap and will ship the same behavior that we
shipped in Firefox 61 (bug 1453148), that is, overflow-x, then overflow-y.

So I think lacking a clear way forward we should revert this change and preserve
our shipped behavior.
Really sorry to have to revert the documentation changes and such :(
Comment on attachment 9010418 [details]
Bug 1492567 - Back out bug 1481866. r=dbaron

Preemptively setting the approval flag before I forget.

Approval Request Comment
[Feature/Bug causing the regression]: Not really a regression, but bug 1481866.
[User impact if declined]: Behavior change that we probably don't want to ship as-is.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no, but it's a clean revert.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: This patch reverts a behavior change which I think we shouldn't change, per the  bug 1481866 comment 14. This reverts the behavior that we had since firefox 61, which is when this feature was implemented.
[String changes made/needed]: none
Attachment #9010418 - Flags: approval-mozilla-beta?
(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
> [Why is the change risky/not risky?]: This patch reverts a behavior change
> which I think we shouldn't change, per the  bug 1481866 comment 14. This
> reverts the behavior that we had since firefox 61, which is when this
> feature was implemented.

per the *reasons described in* that comment, and reverts *to* the behavior we had since 61. Sorry, can't type.
Comment on attachment 9010418 [details]
Bug 1492567 - Back out bug 1481866. r=dbaron

David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 has approved the revision.
Attachment #9010418 - Flags: review+
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13088 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/cf9cd0662126
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
I would understand if reverting to only accepting one value, but reverting to the opposite of the spec says means that we will end up with Web content depending on what you are shipping. I don't think this is a good plan. Either ship something compatible with the spec, or tell the CSSWG that you disagree with the spec and insist on it matching your implementation. Having the implementation intentionally do the opposite of what the spec says is not helpful to the Web.
Comment on attachment 9010418 [details]
Bug 1492567 - Back out bug 1481866. r=dbaron

Uplift approved for 63 beta 8, thanks.
Attachment #9010418 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to fantasai from comment #13)
> I would understand if reverting to only accepting one value, but reverting
> to the opposite of the spec says means that we will end up with Web content
> depending on what you are shipping. I don't think this is a good plan.
> Either ship something compatible with the spec, or tell the CSSWG that you
> disagree with the spec and insist on it matching your implementation. Having
> the implementation intentionally do the opposite of what the spec says is
> not helpful to the Web.

I'm backing out to what we and other engines are shipping, to avoid shipping something that contradicts both the spec and our previous behavior. I wouldn't be confident to back this out to pre-61 behavior and getting that uplifted.

If you want to put the shorthand back to one value I can do that, but I don't think I should get that uplifted for 63. That being said if we go that rout we should ask the Blink folks to unship their shorthand as well.
Setting to DDC — we already reverted this in the docs.
You need to log in before you can comment on or make changes to this bug.