1111517 - selection in vertical-rl content with nested blocks is flaky or broken

Status: RESOLVED FIXED

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

Description

[Jonathan Kew [:jfkthame]]

Attachment: testcase for selection behavior

STR: in vertical-enabled build, load attached testcase. Click within the first (rightmost) of the text, and drag gradually leftwards.

Note how the selection is correct when the cursor is within one of the lines, but whenever it is in the inter-line gaps, everything to the end of the text gets selected. This produces an ugly, distracting flicker if the cursor is dragged back and forth.

For a more extreme example, load attachment 8525944 [details] (testcase from bug 1102175) and set writing mode to vertical-rl; then try to drag-select some of the text. Drag-select works within the labels of the radio buttons, but does not work at all within the main <div>. In other writing modes, however, it is fine.

Comment 1

[Jonathan Kew [:jfkthame]]

This occurs because of an error in the patch from bug 1088025. That used nsBlockFrame::SlideLine to move the lines of a vertical-rl block into their final positions once the block size (which becomes their container width) is known. This correctly updates the rects of the lines and their child frames, so everything displays in the proper place; however, it is wrong because it also updates the *logical* rect of the nsLineBox records.

At this point, where the lines have been reflowed logically, but rects were set based on an incorrect container width, what we need to do is to maintain their logical rects *unchanged*, but update physical positions based on the new container width. So although the effect on physical rects and hence visual appearance is the same as SlideLine, this is a different operation in terms of logical coordinates.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Use new method UpdateLineContainerWidth instead of SlideLine when finalizing the width of a vertical-rl block

With this, drag-selection behaves much better.

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 8536446 [details] [diff] [review]
Use new method UpdateLineContainerWidth instead of SlideLine when finalizing the width of a vertical-rl block

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

::: layout/generic/nsBlockFrame.cpp
@@ +2813,5 @@
> +    return;
> +  }
> +
> +  // Adjust line state
> +  nscoord widthDelta = aLine->UpdateContainerWidth(aNewContainerWidth);

Move this inside the if().

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8536446 [details] [diff] [review]
Use new method UpdateLineContainerWidth instead of SlideLine when finalizing the width of a vertical-rl block

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

::: layout/generic/nsBlockFrame.cpp
@@ +2813,5 @@
> +    return;
> +  }
> +
> +  // Adjust line state
> +  nscoord widthDelta = aLine->UpdateContainerWidth(aNewContainerWidth);

Forget that; we should update it for consistency regardless of the writing mode.

Comment 5

[Simon Montagu :smontagu]

Comment on attachment 8536446 [details] [diff] [review]
Use new method UpdateLineContainerWidth instead of SlideLine when finalizing the width of a vertical-rl block

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

LGTM

Comment 6

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4859bf133ffa

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4859bf133ffa