Closed Bug 1348596 Opened 3 years ago Closed 3 years ago

Incorrectly landed change to BuildTextRunsScanner::ContinueTextRunAcrossFrames

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

Details

Attachments

(1 file)

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
In light of the recent thread re tightening up reviews, rebasing, etc., this was an interesting (though highly embarrassing) discovery!
Attachment #8848817 - Flags: review?(dbaron)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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.
Attachment #8848817 - Flags: review?(dbaron) → review+
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 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.
Attachment #8848817 - Flags: approval-mozilla-beta?
Attachment #8848817 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/40a5fe3da661
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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+.
Attachment #8848817 - Flags: approval-mozilla-beta?
Attachment #8848817 - Flags: approval-mozilla-beta+
Attachment #8848817 - Flags: approval-mozilla-aurora?
Attachment #8848817 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.