1093316 - Back out flexbox "flex-basis:main-size" rename, since the CSSWG removed it from the spec

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Bug 1032922 (a "flex-basis" keyword rename, introducing the "main-size" value) is scheduled to ship as part of Firefox 34 in 3 weeks.

Since I implemented bug 1032922, an alternate proposal[1] had been made. If that proposal is accepted by the CSSWG, then we'd want to rip out our "main-size" support.

This is under discussion on www-style right now[2], and it's unclear what's going to happen. Microsoft may have some data showing that "main-size" causes too many backwards-compatibility issues (though we haven't seen too many issues so far [3]).

In the meantime, if it's not too risky / breaking of a change, it might be worth backing out bug 1032922 on the beta channel before it ships (i.e. delaying the shipping of "flex-basis:main-size"), so that:
 * Firefox release users don't hit site-breakage while this is sorted out (since currently we're the only browser with the "main-size" behavior implemented, AFAIK, and sites coded to other browsers (or to older Firefox) may misrender as a result.)

 * If the CSSWG does resolve to change the behavior & drop "main-size", it'd be better for us to have not shipped "main-size" already, to reduce the likelihood that sites depend on it.  (Any stuff that breaks would presumably not work in other browsers anyway, so this is more of a risk for Firefox/Gecko-targeted content like add-ons, Gaia apps, & Firefox's own UI code.)

[1] http://lists.w3.org/Archives/Public/www-style/2014Sep/0381.html
[2] http://lists.w3.org/Archives/Public/www-style/2014Nov/0023.html
[3] (Bug 1057162 tracks sites that were broken by the 'main-size' rename)

Comment 1

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

Seems reasonable.

(I think I lean a little more towards getting rid of main-size than you do, although I haven't looked at the alternative proposal.)

Comment 2

[Daniel Holbert [:dholbert] (PTO until 2/24)]

FWIW, there are two bugs that landed after bug 1032922 which make the backout require a bit of hand-tweaking:

 - bug 1050654 replaced a bunch of "flex: 0 0 [auto or main-size]" lines with "flex:none". Those lines don't have to be changed, as "flex:none" is backwards-and-forwards-compatible, so this is actually great. (It makes the backout patch smaller.)

 - bug 1046950 renamed some variables in code that's being touched here, in nsLayoutUtils.cpp & nsFrame.cpp, to replace "horizontal" with "inline", and "vertical" with "block". (This just requires some hand-editing of the chunks to be reverted / restored, to use the updated variable-names.)

Other than that, this is a pretty clean backout.

Comment 3

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=89c470b4f002

Comment 4

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: PART 1: backout aece7f9f944c (i.e. backout bug 1032922 part 2)

Comment 5

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: PART 2: backout af2a4fb980ad (i.e. backout bug 1032922 part 1)

Comment 6

[Daniel Holbert [:dholbert] (PTO until 2/24)]

If the Try run goes well, I'll request beta approval.

Comment 7

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 8516337 [details] [diff] [review]
PART 1: backout aece7f9f944c (i.e. backout bug 1032922 part 2)

Try run has some oranges, but they don't look at all related, so I think they're intermittent or artifacts of pushing-Beta-to-Try. Requesting approval to land.

Approval Request Comment
========================

[Feature/regressing bug #]: bug 1032922

[User impact if declined]: Potentially broken sites. (Without these backout-patches, we'll end up shipping the implementation of a spec-change while the CSSWG is still unsure whether this spec-change will stick or not.) See end of comment 0 for more.

[Describe test coverage new/current, TBPL]: We have good reftest/crashtest/mochitest coverage of flexbox & "flex-basis".  (and the first patch here changes our tests to adjust their expectations.)

[Risks and why]:
 - Broken content, basically.
   * On the web, this shouldn't be an issue; any content that works in current Firefox releases will continue to work.
   * In Gaia content for B2G builds that are based off of Gecko 34 & would presumably get this change (i.e. in Firefox OS 2.1), this could potentially break content, if we didn't fix Gaia to account for it (and I intend to do so, if this is approved).  The fix is easy -- just s/main-size/auto/.

[String/UUID change made/needed]:
None.

Comment 8

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> [Risks and why]:
>  - Broken content, basically.
[...]
>    * In Gaia content for B2G builds that are based off of Gecko 34 & would
> presumably get this change (i.e. in Firefox OS 2.1), this could potentially
> break content, if we didn't fix Gaia to account for it (and I intend to do
> so, if this is approved).  The fix is easy -- just s/main-size/auto/.

Following up on this -- it actually looks like Gaia shouldn't be affected by this change, which is great news.

In particular, in our gaia 2.1 branch (https://github.com/mozilla-b2g/gaia/tree/v2.1):
 - There are no mentions of "main-size" -- so, we don't need any search-and-replace.
 - There are a few cases of styles like "flex-basis:auto" & the shorthand "flex: N [N] auto", whose semantics are subtly changing [since after this backout, 'auto' will mean 'look at my main-size property', instead of 'auto-size me'].  But, I audited those cases & none of them have the main-size property (width/height) explicitly set (not in the same CSS rule at least), which means that property almost certainly is "auto", which means the behavior shouldn't actually change.

SO: tl;dr, I think this is low-risk & unlikely to break any content, including gaia's content.

Comment 9

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Try run has some oranges, but they don't look at all related, so I think
> they're intermittent or artifacts of pushing-Beta-to-Try. Requesting
> approval to land.

(Confirmed that these oranges are just noise from pusing-beta-to-try, via a beta-tree-with-no-patches Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e255299d3cba . So, that's comforting, for some definition of comforting. :))

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8516338 [details] [diff] [review]
PART 2: backout af2a4fb980ad (i.e. backout bug 1032922 part 1)

Ok. Let's process the backout in desktop beta7 and mobile beta8. Beta+

Comment 11

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Landed the patches here:
 https://hg.mozilla.org/releases/mozilla-beta/rev/b4e9b4dab577
 https://hg.mozilla.org/releases/mozilla-beta/rev/d04d205b6c12

After landing, I realized I'd used the wrong bug number in the commit message, so (in a second push) I reverted those ^ changesets, and pushed new commits with the correct bug number.

The revert:
 https://hg.mozilla.org/releases/mozilla-beta/rev/0430d2b93ed3
Re-landing:
 https://hg.mozilla.org/releases/mozilla-beta/rev/af442befe914
 https://hg.mozilla.org/releases/mozilla-beta/rev/6f460d9ed80d

(As a sanity-check, I verified that the second push made no net changes [since it was a revert + re-land]. I tagged the second push as DONTBUILD to avoid wasting build resources.)

(Sorry for the mistake.)

Comment 12

[Daniel Holbert [:dholbert] (PTO until 2/24)]

I guess that means we can resolve this as FIXED, and mark bug 1032922 as disabled on firefox34.

Comment 13

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(Note: I'll leave the status-firefox35/status-firefox36 flags as they are for now; but, if the CSSWG ends up opting for an alternative proposal, it's possible we'll end up needing this backout for 35 and 36, too.)

Comment 14

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> if the CSSWG ends up opting for an alternative proposal, it's
> possible we'll end up needing this backout for 35 and 36, too.

Looks like the CSSWG ended up opting for the alternative proposal, so we'll need these backouts on all channels (and then, separately, I can implement the alternative proposal on trunk, after the backout).

Source: http://logs.csswg.org/irc.w3.org/css/2014-11-19/#e495029

Comment 15

[Daniel Holbert [:dholbert] (PTO until 2/24)]

I rebased the patches to apply on trunk. Try run looks good:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e1cb9a6c62cb

I landed the backout patches on inbound:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/6625f6d27396
  https://hg.mozilla.org/integration/mozilla-inbound/rev/4f830e8fb84b

I'll request Aurora approval after the builds/tests have had some time to complete.

Comment 16

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Comment on attachment 8516337 [details] [diff] [review]
PART 1: backout aece7f9f944c (i.e. backout bug 1032922 part 2)

Approval Request Comment:
See comment 7 for answers to approval questions. New facts since then:
 - The "main-size" keyword has been officially removed from the spec (whereas before it was simply at-risk-of-being-removed). Hence, landing the backout on all branches instead of just beta.
 - This backout has landed on inbound (per previous comment) and has been on beta for ~20 days; so, it's gotten test coverage in both of those places.

(In the unlikely event that this doesn't make it to aurora before the merge, then we can just fix it in "beta". Better to fix it earlier & not ship a beta that reintroduces this removed-from-the-spec keyword, though.)

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8516338 [details] [diff] [review]
PART 2: backout af2a4fb980ad (i.e. backout bug 1032922 part 1)


Approving for Aurora since this has already landed on Beta, and it's a holiday so this is the last aurora approval round before the weekend (and merge-day).

Comment 18

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Landed on aurora:
 https://hg.mozilla.org/releases/mozilla-aurora/rev/b0e8b0cf3aac
 https://hg.mozilla.org/releases/mozilla-aurora/rev/cfd757c77c7b

(Also landed a whitespace-only helper-patch to fix 3 DOS-style line-endings in browser.css, which mercurial kept insisting on qref'ing into my first patch here -- so I just split it into its own whitespace-only patch:
  https://hg.mozilla.org/releases/mozilla-aurora/rev/39931dc54451
)