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

Status: RESOLVED FIXED

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

Description

[carson]

Attachment: Screen Shot 2015-10-02 at 1.11.31 PM.png

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.

Comment 1

[Gingerbread Man]

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:44.0) Gecko/20100101 Firefox/44.0
20151002030204

Comment 2

[Mats Palmgren (inactive)]

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.

Comment 3

[Mats Palmgren (inactive)]

Attachment: Make nsBidiPresUtils::RepositionFrame work also for box-decoration-break:clone.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49a6d6dacc84

Comment 4

[Mats Palmgren (inactive)]

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.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eca3009f91f6

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eca3009f91f6

Comment 7

[Mats Palmgren (inactive)]

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

Comment 8

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

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.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c63b04aa6e9a