Bug 70898 – Range sliders and spin buttons don't work with multi-columns

• Component: Forms
• Status: RESOLVED FIXED

This bug is imported from Chromium http://crbug.com/90128
== Steps ==
1. Create a form with number, range, date, etc input types by using http://jsfiddle.net/
2. Use CSS3 columns to layout the form
3. Try to use the up and down arrows to change the value

== Expected result ==
Form elements should work with the new UI

== Current result ==
Up and down arrows are not clickable

Created attachment 112644 [details]
Patch 1

[WebKit Review Bot]

Comment on attachment 112644 [details]
Patch 1

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

New failing tests:
fast/forms/number/spin-in-multi-column.html
fast/events/document-elementFromPoint.html
fast/events/offsetX-offsetY.html

Created attachment 112818 [details]
Patch 2

[WebKit Review Bot]

Comment on attachment 112818 [details]
Patch 2

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

New failing tests:
fast/events/offsetX-offsetY.html

Created attachment 112842 [details]
Patch 3

Comment on attachment 112842 [details]
Patch 3

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2596
> +    LayoutSize containerOffset = offsetFromContainer(o, LayoutPoint());

offsetFromContainer() already calls adjustForColumns(). You should just pass the actual point instead of the zero point so that it does the right thing, instead of replicating adjustForColumns() below (your replicated code is also missing the check for the progression axis). All you probably need to do is move the offsetFromContainer() override from RenderBox to RenderBoxModelObject.

adjustColumns expects logical column coordinate rather than physical column coordinate, terminology may not be common, see below for logical/physical column coordinate:

Physical column Coordinate
+-------+-------+-------+
|       |       |   x   |
+-------+-------+-------+

Logical Column Coordinate
+-------+
|       |
+-------+
|       |
+-------+
|   x   |
+-------+

So, we can't pass actual point to offsetFromContainer and this patch doesn't replicate code of adjustColumns.
// I guess this was reason why original code pass LayoutPoint() instead of passing actual point in mapLocalToContainer.

Pass (0, 0) to offsetFromContainer calculates offset with considering progressive axis.

So, I think this patch does right thing.

I am sorry, my suggestion was wrong! I will take another look at the patch. At the very least, it is missing support for inline-axis column progression, but I will review it again anyway.

Comment on attachment 112842 [details]
Patch 3

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

It might be better to make this change in two steps, first moving the functions up to RenderBoxModelObject, and then actually fixing the bug.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2616
> +        ColumnInfo* colInfo = block->columnInfo();
> +        LayoutPoint point(roundedLayoutPoint(transformState.mappedPoint()));
> +        point -= containerOffset;
> +        size_t colCount = block->columnCount(colInfo);
> +        for (size_t i = 0; i < colCount; i++) {
> +            LayoutRect colRect = block->columnRectAt(colInfo, i);
> +            if (colRect.contains(point)) {
> +                if (i) {
> +                    LayoutRect colRect0 = block->columnRectAt(colInfo, 0);
> +                    if (block->isHorizontalWritingMode())
> +                        containerOffset.expand(colRect.location().x() - colRect0.location().x(), -colInfo->columnHeight() * i);
> +                    else
> +                        containerOffset.expand(-colInfo->desiredColumnWidth() * i, colRect.location().y() - colRect0.location().y());
> +                }
> +                break;
> +            }
> +        }

This logic belongs in a new function in RenderBlock, where hitTestColumns() can share it. In fact, you should factor out the version in hitTestColumns(), because that version correctly accounts for flipped blocks writing modes and for block-axis column progression.

Created attachment 113642 [details]
Patch 4

Comment on attachment 113642 [details]
Patch 4

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:29
> +#include "ColumnInfo.h"

Is this needed?

Created attachment 116904 [details]
Patch 5

(In reply to comment #11)
> (From update of attachment 113642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113642&action=review
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:29
> > +#include "ColumnInfo.h"
> 
> Is this needed?

No, we don't need to have ColumnInfo.h. I forgot to remove it, old change required it.

Thanks for catching this.

Created attachment 117557 [details]
Patch 6 - Update GTK test

[WebKit Review Bot]

Comment on attachment 117557 [details]
Patch 6 - Update GTK test

Clearing flags on attachment: 117557

Committed r101755: <http://trac.webkit.org/changeset/101755>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

fast/events/offsetX-offsetY.html has been failing ever since it was added in this patch:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r102205%20(35254)/fast/events/offsetX-offsetY-pretty-diff.html