offsetLeft broken within CSS3 columns

Created attachment 48615 [details]
testcase for bug

I'm finding that the offsetLeft property behaves differently in WebKit than Firefox for paragraphs nested within a div that uses CSS3 columns.  See the attached testcase.  With the latest WebKit nightly on Mac, the alert shows 8, but with Firefox 3.6, it shows 413.  The numbers may vary a bit depending on font settings, but the key point is that 8 seems like a wrong value since the corresponding paragraph appears in the second column; the Firefox behavior seems more reasonable.  This bug is blocking my attempt to port my multi-column articles user script to WebKit-based browsers:

http://userscripts.org/scripts/show/9022

If a workaround is known, I would love to hear about it.  Thanks!

Cc'ing Dave Hyatt, as it was suggested to me he would be the appropriate person to take a quick look at this bug.

[Jianying Ji]

I have just met the same bug. I am using the offsetLeft of the last child to figure out the right edge of a number of p elements in a multi-columnated div. My case is at http://eclecticself.com/scrollmark.html  Clicking the first column should move the columns one over. This behavior is broken in webkit browsers however. Including chromium-browser and safari.

[Shezan Baig]

Created attachment 139014 [details]
Patch

Fix for offsetLeft and offsetTop within multicolumns

[Levi Weintraub]

Comment on attachment 139014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139014&action=review

This looks like a good change to me.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:595
> -    return yPos;
> +    LayoutPoint topLeft = isBox() ? toRenderBox(this)->topLeftLocation() : LayoutPoint();
> +    return offsetTopLeft(topLeft).y();

Great to see this simplification.

[Julien Chaffraix]

Comment on attachment 139014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139014&action=review

> Source/WebCore/rendering/RenderBoxModelObject.cpp:557
>      RenderBoxModelObject* offsetPar = offsetParent();

While touching this function, this should be moved into the if (offsetPar) below and renamed offsetParent (no need for bad abbreviation).

> Source/WebCore/rendering/RenderBoxModelObject.cpp:588
> +    LayoutPoint topLeft = isBox() ? toRenderBox(this)->topLeftLocation() : LayoutPoint();

This should be moved inside offsetTopLeft().

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:595
>> +    return offsetTopLeft(topLeft).y();
> 
> Great to see this simplification.

I feel a bit ambivalent about that: the code sharing is superb but now you always get both offsets and discard half of the result. That seem wasteful and will slow down our implementation of offset{Top|Left} (AFAICT it's not used elsewhere in rendering/).

To give us some perspective, what are the numbers for a run of Bindings/dom-attributes.html and Dromaeo before / after your patch? (just use Tools/Scripts/run-perf-tests [testName])

> Source/WebCore/rendering/RenderBoxModelObject.h:66
> +    LayoutPoint offsetTopLeft(const LayoutPoint&) const;

It should be protected, we don't want code outside rendering/ to start relying on it.

> Source/WebCore/rendering/RenderObject.h:749
> +    inline LayoutSize offsetForColumns(const LayoutPoint&) const;

No need to make it 'inline' any function defined in the header is inlined by the compiler.

> Source/WebCore/rendering/RenderObject.h:1140
> +inline LayoutSize RenderObject::offsetForColumns(const LayoutPoint& point) const
> +{
> +    LayoutSize offset;
> +    adjustForColumns(offset, point);
> +    return offset;
> +}

This could be moved along the declaration above.

> LayoutTests/fast/block/positioning/offsetLeft-offsetTop-multicolumn-expected.html:32
> +    .size1 {
> +        display: inline-block;
> +        width: 10px;
> +        height: 20px;
> +        padding: 2px;
> +        background: green;
> +    }
> +    .size2 {
> +        display: inline-block;
> +        width: 20px;
> +        height: 20px;
> +        padding: 2px;
> +        background: green;
> +    }
> +    .size3 {
> +        display: inline-block;
> +        width: 40px;
> +        height: 20px;
> +        padding: 2px;
> +        background: green;
> +    }

For the record, this could be simplified a lot:
span {
    display: inline-block;
    height: 20px;
    padding: 2px;
    background-color: green;
}

span .size1
{
   width: 10px;
}

...
(this would underline what is actually specific to the different class as size1 / 2 / 3 (FIXME!) don't really speak to me).

> LayoutTests/fast/block/positioning/offsetLeft-offsetTop-multicolumn.html:131
> +        cover.setAttribute("style", "background: green; " +
> +                                    "z-index: 10; " +
> +                                    "position: absolute; " +
> +                                    "left: " + item.offsetLeft + "px; " +
> +                                    "top: " + item.offsetTop + "px;");
> +        body.appendChild(cover);

I don't understand why this needs to be a ref-tests. It looks like it could be a very simple dumpAsText() test: you are getting offsetLeft / offsetTop here so you can just dump them and compare them to some preset values.

[Shezan Baig]

Created attachment 140295 [details]
Patch (with review changes)

Thanks for reviewing!

(In reply to comment #5)
> (From update of attachment 139014 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139014&action=review
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:557
> >      RenderBoxModelObject* offsetPar = offsetParent();
> 
> While touching this function, this should be moved into the if (offsetPar) 
> below and renamed offsetParent (no need for bad abbreviation).

I've made this change in the latest patch.


> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:588
> > +    LayoutPoint topLeft = isBox() ? toRenderBox(this)->topLeftLocation() : LayoutPoint();
> 
> This should be moved inside offsetTopLeft().


The reason this is outside offsetTopLeft is because the way 'topLeft' is calculated is different for RenderInline.  To make this more obvious, my latest patch does this slightly differently: instead of switching on 'isBox', I modified RenderBox to override offsetLeft/offsetTop (like how we already do for RenderInline) and pass the box's topLeftLocation.



> 
> >> Source/WebCore/rendering/RenderBoxModelObject.cpp:595
> >> +    return offsetTopLeft(topLeft).y();
> > 
> > Great to see this simplification.
> 
> I feel a bit ambivalent about that: the code sharing is superb but now you 
> always get both offsets and discard half of the result. That seem wasteful 
> and will slow down our implementation of offset{Top|Left} (AFAICT it's not 
> used elsewhere in rendering/).


Yes, I sort of hinted this in passing in the changelog.  The problem is that in order to get correct offsetLeft for multicolumns, we *need* to keep track of both x&y coordinates as we move up each parent (i.e. we cannot get the correct offsetLeft if we don't have the correct y-coordinate).  The code sharing is just a nice bonus side-effect of this, it isn't the primary goal of this patch.

I thought of a couple of alternative approaches:

* Cache the result of offsetTopLeft in the renderer so that calling offsetTop 
  right after offsetLeft will not recalculate the result.  However, I think 
  the space cost makes this undesirable.

* Only use offsetTopLeft if we detect that one of the parents has multiple 
  columns, otherwise we can fallback to the previous code.  This means we only 
  do the extra work when we need to.  However, I'm not sure if the perf 
  numbers (see below) justify having 3 copies of nearly identical code.



> 
> To give us some perspective, what are the numbers for a run of Bindings/dom-
> attributes.html and Dromaeo before / after your patch? (just use 
> Tools/Scripts/run-perf-tests [testName])


I ran dom-attributes.html twice after each build.  I should point out that dom-attributes.html only tries offsetLeft, it doesn't try offsetTop (not sure if this is intentional or not).  The results are pasted below:

Without Patch
=============
shezan@shezwk-ubuntu:~/WebKit$ run-perf-tests --platform=gtk PerformanceTests/Bindings/dom-attributes.html 
Running Bindings/dom-attributes.html (1 of 1)
RESULT Bindings: dom-attributes= 2834.8 ms
median= 2822.0 ms, stdev= 58.4992307642 ms, min= 2746.0 ms, max= 2921.0 ms

shezan@shezwk-ubuntu:~/WebKit$ run-perf-tests --platform=gtk PerformanceTests/Bindings/dom-attributes.html 
Running Bindings/dom-attributes.html (1 of 1)
RESULT Bindings: dom-attributes= 2736.8 ms
median= 2739.0 ms, stdev= 15.2367975638 ms, min= 2711.0 ms, max= 2755.0 ms


With Patch
==========
shezan@shezwk-ubuntu:~/WebKit$ run-perf-tests --platform=gtk PerformanceTests/Bindings/dom-attributes.html 
Running Bindings/dom-attributes.html (1 of 1)
RESULT Bindings: dom-attributes= 2813.2 ms
median= 2830.0 ms, stdev= 44.1062353868 ms, min= 2729.0 ms, max= 2856.0 ms

shezan@shezwk-ubuntu:~/WebKit$ run-perf-tests --platform=gtk PerformanceTests/Bindings/dom-attributes.html 
Running Bindings/dom-attributes.html (1 of 1)
RESULT Bindings: dom-attributes= 2709.6 ms
median= 2716.0 ms, stdev= 11.4995652092 ms, min= 2692.0 ms, max= 2721.0 ms



I also did Dromaeo, but it's not immediately obvious whether this tests offsetLeft/offsetTop either.  Results are:

Without patch: http://dromaeo.com/?id=170533
With patch: http://dromaeo.com/?id=170534



> 
> > Source/WebCore/rendering/RenderBoxModelObject.h:66
> > +    LayoutPoint offsetTopLeft(const LayoutPoint&) const;
> 
> It should be protected, we don't want code outside rendering/ to start 
> relying on it.

I've made this change in the latest patch.


> 
> > Source/WebCore/rendering/RenderObject.h:749
> > +    inline LayoutSize offsetForColumns(const LayoutPoint&) const;
> 
> No need to make it 'inline' any function defined in the header is inlined by 
> the compiler.

Ditto.


> 
> > Source/WebCore/rendering/RenderObject.h:1140
> > +inline LayoutSize RenderObject::offsetForColumns(const LayoutPoint& point) const
> > +{
> > +    LayoutSize offset;
> > +    adjustForColumns(offset, point);
> > +    return offset;
> > +}
> 
> This could be moved along the declaration above.
>

Ditto.


> > LayoutTests/fast/block/positioning/offsetLeft-offsetTop-multicolumn-expected.html:32
> > +    .size1 {
> > +        display: inline-block;
> > +        width: 10px;
> > +        height: 20px;
> > +        padding: 2px;
> > +        background: green;
> > +    }
> > +    .size2 {
> > +        display: inline-block;
> > +        width: 20px;
> > +        height: 20px;
> > +        padding: 2px;
> > +        background: green;
> > +    }
> > +    .size3 {
> > +        display: inline-block;
> > +        width: 40px;
> > +        height: 20px;
> > +        padding: 2px;
> > +        background: green;
> > +    }
> 
> For the record, this could be simplified a lot:
> span {
>     display: inline-block;
>     height: 20px;
>     padding: 2px;
>     background-color: green;
> }
> 
> span .size1
> {
>    width: 10px;
> }
> 
> ...
> (this would underline what is actually specific to the different class as 
> size1 / 2 / 3 (FIXME!) don't really speak to me).


Ditto.


> 
> > LayoutTests/fast/block/positioning/offsetLeft-offsetTop-multicolumn.html:131
> > +        cover.setAttribute("style", "background: green; " +
> > +                                    "z-index: 10; " +
> > +                                    "position: absolute; " +
> > +                                    "left: " + item.offsetLeft + "px; " +
> > +                                    "top: " + item.offsetTop + "px;");
> > +        body.appendChild(cover);
> 
> I don't understand why this needs to be a ref-tests. It looks like it could 
> be a very simple dumpAsText() test: you are getting offsetLeft / offsetTop 
> here so you can just dump them and compare them to some preset values.


I originally made it a ref-test because that was the only way I could figure out how to test just the offsetLeft/offsetTop calculation, and not the multicolumn layout algorithm (which I assume is covered by other tests, and not the focus of this bug).  Including the preset values as part of the test would make this also a test of our multicolumn layout algorithm, which has the following drawbacks:

* The test will not work on other browsers, because it turns out that different
  browsers implement the multicolumn layout algorithm differently.

* If any changes are made to the webkit multicolumn layout algorithm, it will
  cause this test to fail, even though this test is only supposed to check the
  implementation of offsetLeft and offsetTop.

Nevertheless, my latest patch uses a dumpAsText test as requested, with the preset values that I gathered from my test run, but it will obviously fail in other browsers.

[WebKit Review Bot]

Comment on attachment 140295 [details]
Patch (with review changes)

Attachment 140295 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12626224

New failing tests:
fast/block/positioning/offsetLeft-offsetTop-multicolumn.html

[WebKit Review Bot]

Created attachment 140305 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Shezan Baig]

Hmm, from the chromium test results:

-PASS spans[4].offsetTop is 23
+FAIL spans[4].offsetTop should be 23. Was 25.
-PASS spans[5].offsetTop is 23
+FAIL spans[5].offsetTop should be 23. Was 25.
-PASS spans[6].offsetTop is 46
+FAIL spans[6].offsetTop should be 46. Was 50.
-PASS spans[7].offsetTop is 46
+FAIL spans[7].offsetTop should be 46. Was 50.
-PASS spans[8].offsetTop is 46
+FAIL spans[8].offsetTop should be 46. Was 50.
-PASS spans[9].offsetTop is 69
+FAIL spans[9].offsetTop should be 69. Was 75.
-PASS spans[10].offsetTop is 69
+FAIL spans[10].offsetTop should be 69. Was 75.
-PASS spans[11].offsetTop is 92
+FAIL spans[11].offsetTop should be 92. Was 100.
-PASS spans[12].offsetTop is 92
+FAIL spans[12].offsetTop should be 92. Was 100.


Something in the Chromium port seems to be inserting an extra 2 pixels per row.  Doesn't ever port use exactly the same layout algorithm?

[Shezan Baig]

Created attachment 140542 [details]
Patch (with review changes, use Ahem font for test case)

This fixes patch 140295 by using the Ahem font so that the pre-calculated test values are consistent for all platforms

[Julien Chaffraix]

Comment on attachment 140542 [details]
Patch (with review changes, use Ahem font for test case)

View in context: https://bugs.webkit.org/attachment.cgi?id=140542&action=review

> Source/WebCore/rendering/RenderBox.h:437
> +    virtual LayoutUnit offsetLeft() const;
> +    virtual LayoutUnit offsetTop() const;

That's much better thanks. Please annotate this with OVERRIDE.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:551
> +    if (RenderBoxModelObject* offsetParent = this->offsetParent()) {

Maybe we could make |offsetParent| (and the subsequent |curr| const?

> Source/WebCore/rendering/RenderInline.cpp:683
> +        topLeft = LayoutPoint(firstBox->x(), firstBox->y());

I think this should just be topLeft = firstBox->topLeft(); (not repeated)

> LayoutTests/fast/block/positioning/offsetLeft-offsetTop-multicolumn.html:20
> +    @font-face {
> +        font-family: Ahem;
> +        src: url(../../../resources/Ahem.ttf);
> +    }

You don't need that AFAICT as Ahem is an already known font-family. See http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree for our tricks to avoid platform-dependent text. If that doesn't cut it, we can go with a ref-test as it has the advantage of working on other browsers too.

> LayoutTests/fast/block/positioning/offsetLeft-offsetTop-multicolumn.html:157
> +    /*
> +     * Note: the following expected results were generated by running the
> +     *       following function:
> +    function prepareExpectedResults()
> +    {
> +        console.log("    var expectedResults = [");
> +        var line = "        ";
> +        for (var i = 0; i < spans.length; ++i) {
> +            var item = spans[i];
> +            line += "[" + item.offsetLeft + "," + item.offsetTop + "],";
> +            if (i != 0 && i % 8 == 0) {
> +                console.log(line);
> +                line = "        ";
> +            }
> +        }
> +        console.log(line);
> +        console.log("    ];");
> +    }
> +    prepareExpectedResults();
> +    */

This is dead code, it shouldn't be checked in. The way you generated the expected output doesn't really matter.

[WebKit Review Bot]

Comment on attachment 140542 [details]
Patch (with review changes, use Ahem font for test case)

Attachment 140542 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12652084

New failing tests:
fast/block/positioning/offsetLeft-offsetTop-multicolumn.html

[WebKit Review Bot]

Created attachment 140555 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Shezan Baig]

Created attachment 140565 [details]
Patch (with changes from comment 11)

[Shezan Baig]

Comment on attachment 140565 [details]
Patch (with changes from comment 11)

for review

[Julien Chaffraix]

Comment on attachment 140565 [details]
Patch (with changes from comment 11)

View in context: https://bugs.webkit.org/attachment.cgi?id=140565&action=review

Looks good, I have one last comment but the change is neat.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:558
> +            while (curr && curr != offsetParent) {

I think the |curr| NULL-check is unneeded here. You know |curr| and curr->parent() cannot be NULL as |curr| is a descendant of |offsetParent| (which is non-NULL).

[Shezan Baig]

Created attachment 140733 [details]
Patch (with change from comment 16)

[Julien Chaffraix]

Comment on attachment 140733 [details]
Patch (with change from comment 16)

r=me

[WebKit Review Bot]

Comment on attachment 140733 [details]
Patch (with change from comment 16)

Clearing flags on attachment: 140733

Committed r116446: <http://trac.webkit.org/changeset/116446>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

This naming with an “offset” prefix is forced on us for public DOM functions named by Microsoft. But for our own internal functions we could give them clearer names!

[Ojan Vafai]

This test asserts pixel positions for elements that depend on text-rendering. The mac ports (apple and chromium) render the Ahem font a different size than the other ports. A better solution would be to put fixed-size display:inline-block elements inside the spans (or instead of the spans?).

[Eric Seidel (no email)]

Really??  Ahem is rendered at a different size on Mac?

[Shezan Baig]

entered bug 85913 to fix the font-size

[Shezan Baig]

entered bug 85915 to change the name (comment 21)