Bug 70898

Summary: Range sliders and spin buttons don't work with multi-columns
Product: WebKit Reporter: yosin
Component: FormsAssignee: yosin
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, hyatt, mitz, rniwa, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://jsfiddle.net/estelle/5wNxR/1/
Bug Depends on: 73660    
Bug Blocks:    
Attachments:
Description Flags
Patch 1
none
Patch 2
none
Patch 3
none
Patch 4
none
Patch 5
none
Patch 6 - Update GTK test none

Description yosin 2011-10-26 02:53:27 PDT
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
Comment 1 yosin 2011-10-27 00:18:15 PDT
Created attachment 112644 [details]
Patch 1
Comment 2 WebKit Review Bot 2011-10-27 08:59:37 PDT
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
Comment 3 yosin 2011-10-27 23:16:20 PDT
Created attachment 112818 [details]
Patch 2
Comment 4 WebKit Review Bot 2011-10-27 23:54:37 PDT
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
Comment 5 yosin 2011-10-28 03:17:38 PDT
Created attachment 112842 [details]
Patch 3
Comment 6 mitz 2011-10-28 07:56:40 PDT
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.
Comment 7 yosin 2011-10-31 03:52:55 PDT
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.
Comment 8 mitz 2011-11-03 15:38:34 PDT
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 9 mitz 2011-11-03 16:46:07 PDT
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.
Comment 10 yosin 2011-11-04 03:55:55 PDT
Created attachment 113642 [details]
Patch 4
Comment 11 mitz 2011-11-28 10:31:03 PST
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?
Comment 12 yosin 2011-11-28 23:29:15 PST
Created attachment 116904 [details]
Patch 5
Comment 13 yosin 2011-11-28 23:30:25 PST
(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.
Comment 14 yosin 2011-12-01 20:38:17 PST
Created attachment 117557 [details]
Patch 6 - Update GTK test
Comment 15 WebKit Review Bot 2011-12-02 00:26:21 PST
Comment on attachment 117557 [details]
Patch 6 - Update GTK test

Clearing flags on attachment: 117557

Committed r101755: <http://trac.webkit.org/changeset/101755>
Comment 16 WebKit Review Bot 2011-12-02 00:26:27 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Ryosuke Niwa 2011-12-06 19:55:48 PST
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