Closed
Bug 1348596
Opened 4 years ago
Closed 4 years ago
Incorrectly landed change to BuildTextRunsScanner::ContinueTextRunAcrossFrames
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: jfkthame, Assigned: jfkthame)
Details
Attachments
(1 file)
3.64 KB,
patch
|
dbaron
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•4 years ago
|
||
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 | ||
Updated•4 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment 2•4 years ago
|
||
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+
Assignee | ||
Comment 3•4 years ago
|
||
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
Assignee | ||
Comment 4•4 years ago
|
||
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?
Comment 5•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/40a5fe3da661
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•4 years ago
|
status-firefox53:
--- → affected
status-firefox54:
--- → affected
Comment 6•4 years ago
|
||
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+
Comment 7•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/fc0cc9ad4ef8
Comment 8•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/c826f1b2bc24
You need to log in
before you can comment on or make changes to this bug.
Description
•