Closed Bug 1089388 Opened 10 years ago Closed 10 years ago

nsBlockFrame::SlideLine incorrectly treats logical coordinates as if they were physical

Categories

(Core :: Layout: Block and Inline, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

The code in nsBlockFrame::ReflowDirtyLines has been logicalized, but when it passes a block-direction delta to SlideLine, that method treats it as a physical y-delta. As a result, it'll do the wrong thing in vertical writing modes.

Testcase attached: select a vertical writing mode, then enter more text at the beginning of the contentEditable <div>, so that additional lines are created; watch what happens to the lines after the <br>, which don't need to be reflowed but are simply moved using SlideLine.
David, do you have time to review this while Simon is away? If not, then redirect to him and it can wait for his return, I guess. Thanks!
Attachment #8511661 - Flags: review?(dbaron)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Blocks: 1088025
Comment on attachment 8511661 [details] [diff] [review]
Convert nsBlockFrame::SlideLine to use logical coordinates.

Redirecting r? to Simon...
Attachment #8511661 - Flags: review?(dbaron) → review?(smontagu)
Attachment #8511671 - Flags: review?(dbaron) → review?(smontagu)
Comment on attachment 8511661 [details] [diff] [review]
Convert nsBlockFrame::SlideLine to use logical coordinates.

Review of attachment 8511661 [details] [diff] [review]:
-----------------------------------------------------------------

I would like this more if it added a version of MovePositionBy that took a LogicalPoint, but that can be done separately.

There is also a similar usage of MovePositionBy(nsPoint(0, aDeltaBCoord)) in nsBlockReflowState::RecoverFloats which needs patching.
Attachment #8511661 - Flags: review?(smontagu) → review+
Attachment #8511671 - Flags: review?(smontagu) → review+
Something like this? Currently just converts and calls to the physical MovePositionBy, but we could change that someday if we change the storage of nsIFrame::mRect to be logical.
Attachment #8520709 - Flags: review?(smontagu)
Comment on attachment 8520709 [details] [diff] [review]
Followup to add a LogicalPoint version of nsIFrame::MovePositionBy, and use this in SlideLine.

Review of attachment 8520709 [details] [diff] [review]:
-----------------------------------------------------------------

Yes!
Attachment #8520709 - Flags: review?(smontagu) → review+
You need to log in before you can comment on or make changes to this bug.