364079 - Right margin too wide in certain case of over-constrained block in RTL context

Status: RESOLVED FIXED

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

Description

[Uri Bernstein]

When a block inside an RTL block has a width and both left and right margins specified, and the sum of the specified margins and width is less than the available width, the specified value for the left margin is effectively added to the right margin.

The page in the URL field is a real-life example (the content should be approximately centered on the white background, but appears adjacent to its left side instead).

This could also be seen in the "Normal, margins" and "Relative, positioned & margins" sections of attachment 232963 [details] (testcase for bug 328181) - the right margin on the in-RTL blocks is 20px larger than it should be.

Prior to the reflow branch landing, this apparently only affected cases where the contained block and containing block were of opposite directionality (which are relatively rare). For the (more common) cases where both blocks are RTL, this is technically a regression from bug 300030.

Patch coming up.

Comment 1

[Uri Bernstein]

Attachment: patch

availMarginSpace does not include the margin that was specified, but now treated as auto (because |sum| does include this margin). Therefore the specified margin should be added to availMarginSpace in order for the sum to reach the avilable width.

I fixed here both the LTR and RTL cases, although in practice, in the LTR case calculating mComputedMargin.right incorrectly seems to have no effect.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 248861 [details] [diff] [review]
patch

r+sr=dbaron

Comment 3

[Uri Bernstein]

Checked in:

Checking in mozilla/layout/generic/nsHTMLReflowState.cpp;
/cvsroot/mozilla/layout/generic/nsHTMLReflowState.cpp,v  <--  nsHTMLReflowState.cpp
new revision: 1.243; previous revision: 1.242

Comment 4

[Boris Zbarsky [:bzbarsky]]

*** Bug 364182 has been marked as a duplicate of this bug. ***

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: reftest

Comment 6

[Boris Zbarsky [:bzbarsky]]

I checked in that regression test.  In general, please do land tests together with patches, or at the very least mark the bug as "in-testsuite?".

Comment 7

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).