Bug 7604 – calcAbsoluteHorizontalValues() is being getting passed arguments in the wrong order in calcAbsoluteHorizontal()

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Sam Weinig]

In the function calcAbsoluteHorizontal() in render_box.cpp, the function call to calculate the horizontal values for the MaxWidth case:
calcAbsoluteHorizontalValues(MaxWidth, cb, cw, static_distance, pab, l, r, maxW, maxML, maxMR, maxX);

should read:
calcAbsoluteHorizontalValues(MaxWidth, cb, cw, pab, static_distance, l, r, maxW, maxML, maxMR, maxX);

(ie. flip the variables pab and static_distance)

[Sam Weinig]

Now this is fun.  Upon closer inspection of the code, it seems to me that the static_distance variable is not even needed in calcAbsoluteHorizontalValues() as the case when it might be used (when left == AUTO && width != AUTO && right == AUTO ) is never used.  This is because in calcAbsoluteHorizontal(), if both left and right == AUTO, one is always set to the static distance before calcAbsoluteHorizontalValues() is called.

This, however, does not solve the problem of passing the wrong value for pab (which is the sum of the padding and borders), unless it can somehow be shown that it is always equal to the static distance, which I can't imagine is true.

The is no way around it, this code is in need of a good cleaning.

[Sam Weinig]

Created attachment 7094 [details]
first attempt at cleanup

I am attaching a first go at a clean up of the absolute positioning code in RenderBox.  Among other things, it splits out the replaced element code-path to correspond with the separation in the css 2.1 spec, it gets rid of the use of a constant for AUTO, instead using the Length class, and generally cleans up the code by making variable names more clear, adding comments, and getting rid of dead code.  

As it is right now, the code has a lot of FIXME's left in it in places where I was unsure what the correct change should be.  I would appreciate any feedback on any of these.  If your not sure what I am talking about in the FIXME, please let me know and I will elaborate. 

The comments are also quite verbose as I have included the text of the spec completely.  I found having the text in the code very useful while coding it, but if it is too much, will remove it.  Additionally, I went a little 'const' crazy, but found it useful in that the compiler would tell me if I was changing something I really wanted to be const.

Finally, although I said that this patch is just a first attempt, it does work.  By that I mean it compiles fine and causes no regressions.  I can't attest to preformance, but my assumption is that it should be a little faster, due to some straight forward optimizations made and the exclusion of deadcode paths.

[Sam Weinig]

Created attachment 7194 [details]
update patch

This updates the previous patch to work after darin's big rename.

[Sam Weinig]

Created attachment 7197 [details]
test case for bad 'auto' value

Adding a test case which demonstrates what happens if some decides to use a length of '-666666px' on an absolutely positioned element.

[Maciej Stachowiak]

I'd like to review this but I think it really needs hyatt to look at it.

[Dave Hyatt]

Comment on attachment 7194 [details]
update patch

This looks like a really nice cleanup to me.  I can't answer all of your FIXMEs actually, since many of them are questions I had as well that warrant more research.

My inclination would be for a first cut to attempt to keep the behavior as close as possible to the original, calling out the problems just as you have.  Then after landing this we can iterate.

I assume you made sure this passes the layout tests?

[Sam Weinig]

Created attachment 7532 [details]
patch 3

This is a slightly cleaner version of the patch which removes most of the repeated FIXME's with 3 big FIXME's at the top and makes a few more slight optimizations such as not calculating for 'right' and 'bottom' when they are not needed.  It also includes some changes that were made to the current code regarding the static distance for 'top' when inside a table row.

This patch compiles fine and passes all the layout tests, but is quite a big patch and therefore I'm woried that some behavior might have changed from the origanal as many of the corner cases are not tested in the suite.  I agree with hyatt in that we should try to stick as closely to the origanal behavior as possible, but I don't really understand what he is proposing in his comment (if you could elaborate that would be great),

[Maciej Stachowiak]

I think hyatt means to try to avoid deliberately changing the behavior for any corner cases, or at least add tests for anything that was changed. I'm assuming you didn't do anything of that sort on purpose.

[Sam Weinig]

Created attachment 7605 [details]
Test case show new margin behavior

This is a test case which shows the incorrect current behavior of using the direction property of itself and not it's containing block when choosing which value (left or right) to ignore when overconstrained.

[Dave Hyatt]

Ok, let's get a new patch that includes the new test cases and layout test results as part of the patch.  Then I can + it.

[Dave Hyatt]

Comment on attachment 7532 [details]
patch 3

Will minus for now while waiting for test cases and layout tests results to be included in the patch.

[Dave Hyatt]

Any update here?  This is great cleanup.  Just need some tests.

[Sam Weinig]

Created attachment 8177 [details]
Patch (now with testcases)

This beast of a patch now includes two new test cases and two updated ones.

[Dave Hyatt]

Comment on attachment 8177 [details]
Patch (now with testcases)

r=me.  Love this patch.

[Timothy Hatcher]

Landed in r14351, r14355, r14356 and r14357

[Timothy Hatcher]

Layout tests updated in r14360.