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

RESOLVED FIXED

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Uri Bernstein (Google), Assigned: Uri Bernstein (Google))

Tracking

({regression, rtl, testcase})

Trunk
regression, rtl, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 248861 [details] [diff] [review]
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.
Assignee: nobody → uriber
Status: NEW → ASSIGNED
Attachment #248861 - Flags: superreview?(dbaron)
Attachment #248861 - Flags: review?(dbaron)
Comment on attachment 248861 [details] [diff] [review]
patch

r+sr=dbaron
Attachment #248861 - Flags: superreview?(dbaron)
Attachment #248861 - Flags: superreview+
Attachment #248861 - Flags: review?(dbaron)
Attachment #248861 - Flags: review+
(Assignee)

Comment 3

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
*** Bug 364182 has been marked as a duplicate of this bug. ***
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?".
Flags: in-testsuite+

Comment 7

10 years ago
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
You need to log in before you can comment on or make changes to this bug.