385234 - 336736-1.html reftest is failing with new textframe (involves RTL marquee)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Adam Guthrie]

The test for 336736-1.html is failing with the new textframe. Need to mark it as KNOWN FAIL for now and investigate.

Comment 1

[Daniel Holbert [:dholbert]]

The problem arises when clamping scrollLeft values, in nsScrollPortView.cpp:ClampScrollValues.  
link: http://mxr.mozilla.org/seamonkey/source/view/src/nsScrollPortView.cpp#221

(This isn't the location of the bug, but it's an easy diagnosing spot)

In the regressed build, for a marquee in a RTL region, maxX == 0 and scrolledRect.x (which serves the role of minimum X-value) is also 0.  So, scrollLeft is always clamped to be equal to 0.

In the working code before the regression build, for the same sort of page, maxX was 0, but scrolledRect.x would be a negative number.  Hence, scrollLeft was clamped to be between a negative number and 0. (which gives the marquee freedom to move)

Comment 2

[Daniel Holbert [:dholbert]]

Here's a comparison of a frame-dump of a marquee in a RTL region before and after the regression:

http://pastebin.mozilla.org/104354

The testcase used for the frame-dump is at http://people.mozilla.com/~dholbert/tests/336736/rtl.html

Overview of the interesting changes, from working build to regressed build (listed in order of appearance in frame dump)
 - |Area(div)|               -- x pos changed from  6240 to 0
 - |line|                    -- x pos changed from -6600 to 0
 - |Box(div)|                -- x pos changed from -600 to 6000
 - |line|                    -- x pos changed from 0 to -600
 - |Text(0)| containing "\n" -- x pos changed from 600 to 0
 - |Text(0)| containing "a"  -- x pos changed from 0 to -600
 - |Text(0)| containing "\n" -- x pos changed from 0 to -600

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Ok, this fixes it. The problem was with nsTextFrame::AddInlinePref/MinWidth. The string "\n a\n" was being broken into three runs: RTL, LTR, and RTL and we were only including pref width for the first run, because the code to loop over the runs was quite wrong.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in.

Comment 5

[(no longer active)]

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