Closed Bug 1210877 Opened 4 years ago Closed 4 years ago

box-decoration-break: clone and direction: rtl bug

Categories

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

41 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox43 --- fixed
firefox44 --- fixed

People

(Reporter: carson, Assigned: mats)

References

()

Details

(Keywords: css3, rtl, testcase)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/45.0.2454.101 Safari/537.36

Steps to reproduce:

1. Create an inline element with a background color, some padding, and box-decoration-break: clone.
2. Add direction: rtl on any other element on the page - it can be anywhere and not related at all
3. The padding on the box-decoration-break element is not set properly. Borders, box shadows, and border radius is.

Demo with examples (open in Firefox) - http://jsfiddle.net/cshold/hp1jh7q1/


Actual results:

Padding on the box-decoration-break element is not applied, as if box-decoration-break: slice is set but just for padding.


Expected results:

Padding on the element should be the same as if there were not direction: rtl element on the page.
Keywords: css3
OS: Unspecified → Mac OS X
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0
20151002030204
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout: Block and Inline
Ever confirmed: true
Keywords: rtl, testcase
OS: Mac OS X → All
Product: Firefox → Core
Hardware: Unspecified → All
Looks like frame size is correct after reflowing the inline but then
nsBidiPresUtils::RepositionFrame mess it up.

I guess we optimize away the BidiPresUtils stuff when there are
no RTL elements so that explains why the extra direction:rtl
element is needed to trigger this path.
Assignee: nobody → mats
Flags: needinfo?(mats)
BTW, the code here looks rather weird to me; the comment says
"we need to remove any border/padding first", but on line
1595 we're actually *adding* border+padding.  And |frameISize|
came from aFrame->ISize() on line 1575, which is a *border-box*
size.  I don't understand how this can work, but apparently
it does so I guess the later code compensates for it somehow.
Or maybe I'm just misreading it.
http://hg.mozilla.org/mozilla-central/annotate/0010c0cb259e/layout/base/nsBidiPresUtils.cpp#l1575

The SetRect() near the end use |rect| which is calculated
independently, so perhaps |frameISize| isn't really used in
any way that matters.

Anyway, my patch is just putting the code that assumes 'slice'
under an if-condition that it is.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eca3009f91f6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8669319 [details] [diff] [review]
Make nsBidiPresUtils::RepositionFrame work also for box-decoration-break:clone.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: 'box-decoration-break:clone' doesn't work as expected in some RTL contexts
[Describe test coverage new/current, TreeHerder]:we have lot's of 'box-decoration-break' reftests
[Risks and why]: low risk, seems worth fixing
[String/UUID change made/needed]:none
Attachment #8669319 - Flags: approval-mozilla-aurora?
Flags: in-testsuite+
Comment on attachment 8669319 [details] [diff] [review]
Make nsBidiPresUtils::RepositionFrame work also for box-decoration-break:clone.

Layout fix, has reftests. OK to uplift to aurora.
Attachment #8669319 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.