1492567 - Back out bug 1481866.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

See bug 1481866 Comment 14 as for why.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1492567 - Back out bug 1481866. r=dbaron

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.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Really sorry to have to revert the documentation changes and such :(

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(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 6

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

Comment on attachment 9010418 [details]
Bug 1492567 - Back out bug 1481866. r=dbaron

David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 has approved the revision.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf9cd0662126
Back out bug 1481866. r=dbaron

Comment 8

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/13088 for changes under testing/web-platform/tests

Comment 9

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/cf9cd0662126

Comment 11

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 12

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13088
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/PxeSFhjxRzCNzEVhLVDIhg)

Comment 13

[fantasai]

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 14

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9010418 [details]
Bug 1492567 - Back out bug 1481866. r=dbaron

Uplift approved for 63 beta 8, thanks.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/862f7fc8be9d

Comment 17

[Chris Mills [:cmills]]

Setting to DDC — we already reverted this in the docs.

Comment 18

[Chris Mills [:cmills]]

Note added to Fx 64 rel notes to make this clear:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#CSS