[webkit-dev] some feedback on the calcAbsoluteHorizontalValues patch

[Darin Adler]

Begin forwarded message:

> From: Allan Sandfeld Jensen <kde at carewolf.com>
> Date: May 31, 2006 7:56:38 AM PDT
> To: ap at opensource.apple.com
> Cc: khtml-devel at kde.org
> Subject: Re: [webkit-changes] [14351] trunk/WebCore
> Reply-To: khtml-devel at kde.org
>
> On Saturday 13 May 2006 16:58, ap at opensource.apple.com wrote:
>
>> 2006-05-13  Sam Weinig  <sam.weinig at gmail.com>
>>
>>         Reviewed by Hyatt, landed by ap.
>>
>> WebCore:
>>         Patch for http://bugzilla.opendarwin.org/show_bug.cgi?id=7604
>>         calcAbsoluteHorizontalValues() is being getting passed  
>> arguments
>>         in the wrong order in calcAbsoluteHorizontal()
>>
>>         Cleans up the RenderBox code for absolutely positioned  
>> elements
>>         and adds new functions for replaced absolutely positioned
>>         elements. Now uses Length so that magic number -666666 for
>>         auto lengths is no longer used.
>>
>>         * rendering/RenderBox.cpp:
>>         (WebCore::RenderBox::calcAbsoluteHorizontal):
>>         (WebCore::RenderBox::calcAbsoluteHorizontalValues):
>>         (WebCore::RenderBox::calcAbsoluteVertical):
>>         (WebCore::RenderBox::calcAbsoluteVerticalValues):
>>         (WebCore::RenderBox::calcAbsoluteHorizontalReplaced): Handle
>> replaced case separately.
>>         (WebCore::RenderBox::calcAbsoluteVerticalReplaced): ditto.
>>         * rendering/RenderBox.h:
>
> I just merged this patch to KHTML, and noticed a regression:
>
> In the overconstrained case in calcAbsoluteVerticalReplaced() (step  
> 6) you
> discard information about top, margin-top and margin-bottom.
>
> It should look like this:
>     / 
> *--------------------------------------------------------------------- 
> --*\
>      * 6. If at this point the values are over-constrained, ignore  
> the value
>      *    for 'bottom' and solve for that value.
>      
> \*-------------------------------------------------------------------- 
> ---*/
>     else {
>         m_marginTop = marginTop.width(containerHeight);
>         m_marginBottom = marginBottom.width(containerHeight);
>         topValue = top.width(containerHeight);
>
>         // Solve for 'bottom'
>         // NOTE: It is not necessary to solve for 'bottom' because  
> we don't ever
>         // use the value.
>     }
>
> There is of course the same problem in calcAbsoluteVerticalReplaced 
> () where it
> should look like:
>     / 
> *--------------------------------------------------------------------- 
> --*\
>      * 6. If at this point the values are over-constrained, ignore  
> the value
>      *    for either 'left' (in case the 'direction' property of the
>      *    containing block is 'rtl') or 'right' (in case  
> 'direction' is
>      *    'ltr') and solve for that value.
>      
> \*-------------------------------------------------------------------- 
> ---*/
>     else  {
>         m_marginLeft = marginLeft.width(containerWidth);
>         m_marginRight = marginRight.width(containerWidth);
>         if (containerDirection  == LTR) {
>             leftValue = left.width(containerWidth);
>             rightValue = availableSpace - (leftValue + m_marginLeft  
> + m_marginRight);
>         }
>         else {
>             rightValue = right.width(containerWidth);
>             leftValue = availableSpace - (rightValue + m_marginLeft  
> + m_marginRight);
>         }
>     }
>
> Regards
> `Allan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.macosforge.org/pipermail/webkit-dev/attachments/20060531/1c8e3b32/attachment.html