413542 - percentage margins on inline elements are ignored in bidi pages

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P2)

Description

[Bruno Fassino]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012104 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012104 Minefield/3.0b3pre

The presence of certain characters in the source of an utf-8 html file (even if simply inside comments, or in scripts string variables) causes the disappearing of % margins on inline elements. The characters triggering the problem seem to be most Hebrew and Arabic ones (maybe the ones belonging to rtl languages?)

Reproducible: Always

Comment 1

[Bruno Fassino]

Attachment: minimal test case

A wrapper contains two spans with 20% horizontal margins. An html comment contains the alef (א) character. As a result the margin between the two spans is not rendered.  Removing the alef from the comment solves the problem.

Comment 2

[Bruno Fassino]

A somewhat similar problem occurs even in absence of any triggering character, if dir=rtl is declared on the wrapper. I've no idea if the two problems are really related.

Comment 3

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

There's a bunch of bidi-related stuff that we optimize away if there's nothing bidi-related in the document.  Presumably either of those things trigger the "this document has bidi" flag, which is all that's needed to trigger the bug.

Comment 4

[Simon Montagu :smontagu]

When there is a percentage margin, the calls to GetMargin() in nsBidiPresUtils::RepositionFrame() and RepositionInlineFrames() return an empty margin. Maybe we should be using GetUsedMargin() instead?

Comment 5

[Simon Montagu :smontagu]

This is a regression since 1.8

Comment 6

[Simon Montagu :smontagu]

Attachment: Patch

Comment 7

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

Comment on attachment 298694 [details] [diff] [review]
Patch

r+sr=dbaron

Comment 8

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

(And I'd note that these are both called, from nsBlockFrame::PlaceLine (via the call to bidiUtils->ReorderFrames), which means this is guaranteed to be after the inlines have been reflowed, which is good (and required).)

Comment 9

[Simon Montagu :smontagu]

Attachment: Testcase for padding

Padding has the same bug, of course...

Comment 10

[Simon Montagu :smontagu]

Attachment: Patch including padding

...and the same fix

Comment 11

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

Comment on attachment 298922 [details] [diff] [review]
Patch including padding

r+sr=dbaron

Comment 12

[Simon Montagu :smontagu]

Checked in with the testcases as reftests