1524431 - Teach nsBidiPresUtils to only manipulate continuations created by bidi resolution

Status: RESOLVED FIXED

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

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

File per dbaron's comment in https://phabricator.services.mozilla.com/D17551#466846,

Though in the longer term, I think it would probably make more sense for this code to know which continuations were created as a result of bidi resolution and only mess with those; that gives us more flexibility to use fixed continuations for things other than bidi. This code is essentially assuming that bidi is the only user of fixed continuations, and I don't think that's fair. You're fixing it by keeping things separated, but not really fixing what I see as the underlying problem.

The background story is that column-span:all splits nsColumnSetFrames into several fragments, and they're linked together by non-fluid (fixed) continuations, which previously are only created by the results of bidi resolutions. So bidi utils aren't aware of that, and mess with them.

My patches in Bug 1520722 are regards as short term workaround. Depend on the out come of the solution in this bug, we may be able to revert bug 1520722 without any regression.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1524431 Part 1 - Revert Bug 1520722 Part 2.

This reverts the modification to nsBidiPresUtils.cpp in Bug 1520722 Part
2, but keeps the test added. Next part will fix the problem in a proper
way.

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1524431 Part 2 - Check IsBidiSplittable() before processing the ancestor frames.

The idea is to check IsBidiSplittable() in more places to prevent fixed
continuations created by column-span from becoming fluid ones.

Depends on D34092

Comment 3

[Jonathan Kew [:jfkthame]]

Does this also fix the testcase from bug 1554824, by any chance?

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Jonathan Kew (:jfkthame) from comment #3)

Does this also fix the testcase from bug 1554824, by any chance?

Sadly, no. It's a different issue. I'm still seeking a solution to the bug.

Comment 5

[Jonathan Kew [:jfkthame]]

OK, thanks for checking.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79d5b3103bff
Part 1 - Revert Bug 1520722 Part 2. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/6f0a648c3d83
Part 2 - Check IsBidiSplittable() before processing the ancestor frames. r=jfkthame

Comment 7

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

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

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (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/17275
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/VD5Nobz7RYKAtbJ-9sOBRA)

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/79d5b3103bff
https://hg.mozilla.org/mozilla-central/rev/6f0a648c3d83

Comment 10

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

Upstream PR was closed without merging

Comment 11

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

Upstream PR merged