1348596 - Incorrectly landed change to BuildTextRunsScanner::ContinueTextRunAcrossFrames

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jonathan Kew (:jfkthame)]

CSS Text 3 says that shaping should be interrupted at inline element boundaries if 'vertical-align' (on either side of the boundary) is not 'baseline'.[1]

We used not to implement that. (And we still don't implement other aspects of that section of the spec, see bug 1064172.)

However, I just discovered that in landing the patch for bug 1331683, I accidentally included an nsTextFrame.cpp chunk that did not belong there (and is not present in the patch that was posted to the bug). This can be seen by checking what actually landed in https://hg.mozilla.org/mozilla-central/rev/de99933ae324. Major oops. :( Clearly this was a bad case of mercurial user error on my part.

That chunk from my local tree doesn't quite implement the spec correctly, AFAICS, as it allows shaping across the boundary if 'vertical-align' is specified as a zero offset, in addition to the keyword value 'baseline', whereas the spec wants it to be interrupted in *all* cases except 'baseline'.

Given that the incorrectly-landed change does not quite conform to the spec (and was not reviewed prior to landing), I think we should simply back out that change, and then re-land a corrected version of it after proper review as part of bug 1064172.

I'll post a backout patch (for the bad part of the landing only) here for review and uplift.

[1] https://drafts.csswg.org/css-text/#boundary-shaping

Comment 1

[Jonathan Kew (:jfkthame)]

Attachment: Back out incorrectly-landed (and unreviewed) change to ContinueTextRunAcrossFrames that was mistakenly pushed

In light of the recent thread re tightening up reviews, rebasing, etc., this was an interesting (though highly embarrassing) discovery!

Comment 2

[David Baron :dbaron: 🇯🇵 ⌚UTC+9]

Comment on attachment 8848817 [details] [diff] [review]
Back out incorrectly-landed (and unreviewed) change to ContinueTextRunAcrossFrames that was mistakenly pushed

I don't think backing out an erroneously-landed patch actually needs review, but r=dbaron.

Comment 3

[Jonathan Kew (:jfkthame)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40a5fe3da66157a997e6a5f5490e3e0b6baa6cb7
Bug 1348596 - Back out incorrectly-landed (and unreviewed) change to ContinueTextRunAcrossFrames that was mistakenly pushed. r=dbaron

Comment 4

[Jonathan Kew (:jfkthame)]

Comment on attachment 8848817 [details] [diff] [review]
Back out incorrectly-landed (and unreviewed) change to ContinueTextRunAcrossFrames that was mistakenly pushed

Not sure if explicit approval is necessarily required for backout of an erroneously-landed patch, but given that it has reached as far as beta, I thought it better to flag it so it doesn't catch release managers by surprise.

The patch being backed-out here was accidentally pushed (without review) as part of an unrelated bug. Fortunately, AFAICS it doesn't actually affect behavior -- it was checking the wrong style contexts for its intended effect, such that it ends up being a no-op -- but on principle I think we should remove the stray code from all the branches it has reached.

Comment 5

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/40a5fe3da661

Comment 6

[Gerry Chang [:gchang]]

Comment on attachment 8848817 [details] [diff] [review]
Back out incorrectly-landed (and unreviewed) change to ContinueTextRunAcrossFrames that was mistakenly pushed

Backout incorrectly landed codes. Aurora54+ & Beta53+.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fc0cc9ad4ef8

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c826f1b2bc24