Remove the blocking touch handlers for the input[type=range] and add touch-action instead

Remove the blocking touch handlers for the input[type=range] element and add touch-action instead.

In current implementation, the input[type=range] blocks browser's scrolling in all directions.

After adding "touch-action: pan-y" with a passive event listener to the horizontal input[type=range], or "pan-x" to the vertical input[type=range], the slider will only consume pan in its direction and does not block scrolling on the other direction.

This touch-action will be overridden if the user has already defined the touch-action. It will update if the slider changes to horizontal/vertical.

BUG=584438
Committed: https://crrev.com/cb18694aff180e913277a346a37e74835935b37d
Cr-Commit-Position: refs/heads/master@{#413616}

[sunyunjia, 2016-08-03 17:04:03]

sunyunjia@chromium.org changed reviewers:
+ majidvp@chromium.org

[sunyunjia, 2016-08-03 17:04:03]

Not sure if this is the right way to do it, but please take a look.

[majidvp, 2016-08-03 18:01:06]

We need a layout test to ensure the right type of event
handler is added for the range input type.

Also need tests to verify the right touch-action is 
used.

https://codereview.chromium.org/2209773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/2209773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLInputElement.h:57: enum TouchHandlerType
{
If this is only used in InputTypeView then it should move there.

Also, it appears that no HTLM Input ever uses 'Blocking' so we might as well
just use None, Passive.

https://codereview.chromium.org/2209773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLInputElement.h:405: TouchHandlerType
m_hasTouchEventHandler : Blocking;
Why do you need to keep this value as a member variable?

Also ": Blocking" does not make much sense here. The value after ':' 
defines the bit size for that variable. In this case you would have
used '2'.

see: http://en.cppreference.com/w/cpp/language/bit_field

https://codereview.chromium.org/2209773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/forms/RangeInputType.cpp (right):

https://codereview.chromium.org/2209773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/forms/RangeInputType.cpp:262:
track->setAttribute(styleAttr, "pan-y pinch-zoom");
Using the style attribute seems reasonable.

However I have a few notes about the attributes used: 
1- pinch-zoom: This one is still is not in the spec. I am not against using it
but chrome
currently does not support it fully [1], [2].

2- what about double-tap-zoom, similar to (1) it is not in spec but I think we
should allow it.

3- What happens if the slider reaches its extent? should we in that case allow
the user agent to
scroll in that direction? There is an interesting example of doing this in the
pointer-events spec
which I think applies here. [3]


[1] http://crbug.com/626086 
[2] https://github.com/w3c/pointerevents/issues/29 
[3] https://w3c.github.io/pointerevents/#the-touch-action-css-property

https://codereview.chromium.org/2209773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/forms/RangeInputType.h (right):

https://codereview.chromium.org/2209773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/forms/RangeInputType.h:63: TouchHandlerType
hasTouchEventHandler() const override;
This is no longer a boolean so 'hasTouchEventHandler' is not an 
appropriate method name. Perhaps 'touchEventHandlerType'.

[sunyunjia, 2016-08-03 23:55:39]

Changed the function name and dealt with the out-of-range situation.

This is just an attempt at this moment. I'm not sure if any if-branches in
HTMLInputElement.cpp needs to be updated.

For the out-of-range situation, I'm considering this situation:
1. Drag the thumb to the right extend -> pan-right enabled.
2. Then, drag the thumb to the middle, and without releasing the pointer, drag
the slider towards right again.
Considering the second note from [3], at this time, will pan-right be disabled?

[majidvp, 2016-08-04 15:20:26]

Taking a step back. I think it will be more appropriate to handle the
new extend behaviour (i.e., pan-left, pan-right) in a separate follow up
patch. This keeps the current patch simple and more importantly keeps
the behaviour consistent with the existing implementation.

Once this lands then you can follow up with a patch that *improves*
the UX by allowing panning one slider has reached its extent. Some
may even argue to keep the current behaviour.

On 2016/08/03 23:55:39, sunyunjia wrote:
> Changed the function name and dealt with the out-of-range situation.
> 
> This is just an attempt at this moment. I'm not sure if any if-branches in
> HTMLInputElement.cpp needs to be updated.
> 
> For the out-of-range situation, I'm considering this situation:
> 1. Drag the thumb to the right extend -> pan-right enabled.
> 2. Then, drag the thumb to the middle, and without releasing the pointer, drag
> the slider towards right again.

Sounds reasonable. Same thing for left extent with pan-left.

> Considering the second note from [3], at this time, will pan-right be
disabled?

Not sure what this refers too? we have pan-left and pan-right in Chrome.

[majidvp, 2016-08-04 15:20:36]

https://codereview.chromium.org/2209773002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/2209773002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLInputElement.cpp:416:
registry.didAddEventHandler(*this,
EventHandlerRegistry::TouchStartOrMoveEventBlocking);
The idea is to use TouchStartOrMoveEventPassive here.

https://codereview.chromium.org/2209773002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp (right):

https://codereview.chromium.org/2209773002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:146:
HTMLInputElement* host = hostInput();
I don't think this component should be involved with the details of touch-action
behaviour on its host. This should either become part of LayoutSliderContainer 
or RangeInputType.

https://codereview.chromium.org/2209773002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:147: if
(!isVertical) {
Good catch handling the vertical case. I think the initial value for
touch-action
should also take into account vertical mode.

[sunyunjia, 2016-08-04 17:41:36]

Temporarily remove the drag-to-extend case.

Handle the initial value for touch-action with vertical mode.

Change the registry to TouchStartOrMoveEventPassive.

[majidvp, 2016-08-04 21:21:58]

Please add a test to verify we are adding the right touch-action and event
handler. 

Please add a bit more details in the CL description. In particular add the
following information:
 - explanation on why are you making the change
 - why we need "touch-action: pan-y" with a passive event listener
 - how would the behaviour be different for the existing behaviour (i.e, slider
will only consume pan in its direction and does not block scrolling on the other
direction)

https://codereview.chromium.org/2209773002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/2209773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLInputElement.cpp:414: // TODO(dtapuska):
Make this passive touch listener see crbug.com/584438
I think this TODO is no longer applicable once you land this. Please remove.

https://codereview.chromium.org/2209773002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/RangeInputType.cpp (right):

https://codereview.chromium.org/2209773002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/RangeInputType.cpp:271: }
This logic looks reasonable but I think this does not handle style updates 
correctly. For example if I open devtools and change -webkit-appearance
value we don't recreate the shadow tree but only re-layout and re-paint it.

So I think this should actually move inside |LayotuSliderContainer::layout()|
which is invoked anytime style change invalidates paint, layout.

[sunyunjia, 2016-08-05 14:20:06]

Description was changed from

==========
Remove the blocking touch handlers for the InputRange element and add
touch-action instead

Change range's scroll-blocking touch handlers to passive

BUG=584438
==========

to

==========
In current implementation, the InputRange element blocks scrolling in all
directions.

After adding "touch-action: pan-y" with a passive event listener to the
InputRange element, so that slider will only consume pan in its direction and
does not block scrolling on the other direction

BUG=584438
==========

[sunyunjia, 2016-08-08 20:11:22]

Description was changed from

==========
In current implementation, the InputRange element blocks scrolling in all
directions.

After adding "touch-action: pan-y" with a passive event listener to the
InputRange element, so that slider will only consume pan in its direction and
does not block scrolling on the other direction

BUG=584438
==========

to

==========
In current implementation, the InputRange element blocks browser's scrolling in
all directions.

After adding "touch-action: pan-y" with a passive event listener to the
horizontal InputRange element, or "pan-x" to the vertical InputRange element,
the slider will only consume pan in its direction and does not block scrolling
on the other direction.

This touch-action will be overridden if the user has already defined the
touch-action. It will update if the slider changes to horizontal/vertical. 

BUG=584438
==========

[sunyunjia, 2016-08-08 20:26:33]

PTAL tkent@

[sunyunjia, 2016-08-08 20:31:16]

sunyunjia@chromium.org changed reviewers:
+ tkent@chromium.org

[sunyunjia, 2016-08-08 20:32:14]

PTAL tkent@

[sunyunjia, 2016-08-08 20:58:27]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 20:58:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 21:24:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 21:24:56]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sunyunjia, 2016-08-08 21:59:59]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 22:00:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 22:29:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 22:29:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tkent, 2016-08-09 00:29:46]

> In current implementation, the InputRange element blocks browser's scrolling
in all directions.


The first line of the CL description should be a summary of the CL.  Maybe
"Remove the blocking touch handlers for the InputRange element and add
touch-action instead" should be there.

"InputRange" looks like there is such a proper noun.  We usually call it "range
input type" or "input[type=range]" or "<input type=range>".

https://codereview.chromium.org/2209773002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html
(right):

https://codereview.chromium.org/2209773002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html:15:
assert_equals(internals.touchActionType(document.getElementById('slider1')),
'pan-y');
Doesn't |getComputedStyle(document.getElementById('slider1'), '').touchAction|
work?

https://codereview.chromium.org/2209773002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/2209773002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1486: if (property
== CSSPropertyWebkitAppearance && !hasTouchAction) {
This code looks like the state.style() will have touch-action:pan-* even if the
element is not <input type=range>.

We have appearance-specific style adjustment code in LayoutTheme::adjustStyle().
We can check <input type=range> inside it.
Also, we have some other slider appearances; MediaFullScreenVolumeSliderPart,
MediaSliderPart, and MediaVolumeSliderPart.

https://codereview.chromium.org/2209773002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLInputElement.h (right):

https://codereview.chromium.org/2209773002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLInputElement.h:38: };
Do you have a plan to add more values to the enum?
If not, how about renaming InputTypeView::hasTouchEventHandler to
hasPassiveTouchEventHandler?

[sunyunjia, 2016-08-09 01:48:34]

Description was changed from

==========
In current implementation, the InputRange element blocks browser's scrolling in
all directions.

After adding "touch-action: pan-y" with a passive event listener to the
horizontal InputRange element, or "pan-x" to the vertical InputRange element,
the slider will only consume pan in its direction and does not block scrolling
on the other direction.

This touch-action will be overridden if the user has already defined the
touch-action. It will update if the slider changes to horizontal/vertical. 

BUG=584438
==========

to

==========
Remove the blocking touch handlers for the input[type=range] element and add
touch-action instead.

In current implementation, the InputRange element blocks browser's scrolling in
all directions.

After adding "touch-action: pan-y" with a passive event listener to the
horizontal InputRange element, or "pan-x" to the vertical InputRange element,
the slider will only consume pan in its direction and does not block scrolling
on the other direction.

This touch-action will be overridden if the user has already defined the
touch-action. It will update if the slider changes to horizontal/vertical. 

BUG=584438
==========

[sunyunjia, 2016-08-09 01:49:15]

Description was changed from

==========
Remove the blocking touch handlers for the input[type=range] element and add
touch-action instead.

In current implementation, the InputRange element blocks browser's scrolling in
all directions.

After adding "touch-action: pan-y" with a passive event listener to the
horizontal InputRange element, or "pan-x" to the vertical InputRange element,
the slider will only consume pan in its direction and does not block scrolling
on the other direction.

This touch-action will be overridden if the user has already defined the
touch-action. It will update if the slider changes to horizontal/vertical. 

BUG=584438
==========

to

==========
Remove the blocking touch handlers for the input[type=range] element and add
touch-action instead.

In current implementation, the input[type=range] blocks browser's scrolling in
all directions.

After adding "touch-action: pan-y" with a passive event listener to the
horizontal input[type=range], or "pan-x" to the vertical input[type=range], the
slider will only consume pan in its direction and does not block scrolling on
the other direction.

This touch-action will be overridden if the user has already defined the
touch-action. It will update if the slider changes to horizontal/vertical. 

BUG=584438
==========

[sunyunjia, 2016-08-09 17:55:39]

PTAL, Thanks!

[majidvp, 2016-08-09 18:30:09]

https://codereview.chromium.org/2209773002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html
(right):

https://codereview.chromium.org/2209773002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html:9:
assert_equals(internals.blockingTouchStartOrMoveEventHandlerCount(document), 2);
Why are we expecting two blocking touch handlers? I expected this to be 0.

https://codereview.chromium.org/2209773002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html:12:
'Tests that each range input type has one passive touch handler and a pan-x or
pan-y blocking touch handler.');
This test does not check pan-x, pan-y. Please update the description to be more
accurate.
Also it does not really test that each one has one handler for all we know one
can 
have 2 and one 0.

https://codereview.chromium.org/2209773002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html:19:
document.body.offsetTop;
How about wrapping document.body.offsetTop; in a |forceLayoutUpdate| function to
help
with readability?

https://codereview.chromium.org/2209773002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html
(right):

https://codereview.chromium.org/2209773002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html:28:
assert_equals(getComputedStyle(document.getElementById('slider1')).touchAction,
'none');
Hmmm, I don't think this verifies any logic that you have added.
AFAIK this just setting a CSS property and reading it back which
is not relevant here.

https://codereview.chromium.org/2209773002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTheme.cpp (right):

https://codereview.chromium.org/2209773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTheme.cpp:219: return;
Better to move this inside adjustSliderStyle.
Also better tests are: 

isHTMLInputElement()
and
element->type() != InputTypeNames::range

https://codereview.chromium.org/2209773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTheme.cpp:640: if
(style.getTouchAction() != TouchActionAuto) {
why do you need this? 
Is this to prevent overriding author's styling?

It is odd that you need this. Some points:

1. Should authors actually be able to modify the style for this user agent 
   created in shadow dom? I would say no in general. They can always
   do it in devtools but if they do we can just rely on CSS resolution to pick
   the effective value.

2. touch-action walks up the chain and combines all ancestor touch-actions
    so if there is a need, authors can actually update the shadow host style
    to control the touch action and don't need to reach inside the shadow
    dom tree.

https://codereview.chromium.org/2209773002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTheme.cpp:648: return;
This return is unnecessary.

[sunyunjia, 2016-08-09 23:01:45]

PTAL

Besides, we are thinking about moving all the touch handler logic of <input
type="range"> into an element of its shadow dom, e.g. SliderContainerElement. 

This will enable authors have their own touch-action that does not conflict with
user-agent's one. Doing so would give us a more clear logic that handles the
conflict than my current implementation. What's your opinion, tkent@ ?

[tkent, 2016-08-10 02:20:53]

On 2016/08/09 at 23:01:45, sunyunjia wrote:
> PTAL
> 
> Besides, we are thinking about moving all the touch handler logic of <input
type="range"> into an element of its shadow dom, e.g. SliderContainerElement. 
> 
> This will enable authors have their own touch-action that does not conflict
with user-agent's one. Doing so would give us a more clear logic that handles
the conflict than my current implementation. What's your opinion, tkent@ ?

It sounds better.  However I'm not sure if it really won't prevent web authors
from overriding touch-action of input[type=range]. Does <input type=range
style="touch-action:none"> will continue to work as expected?

[majidvp, 2016-08-10 14:31:24]

On 2016/08/10 02:20:53, tkent wrote:
> On 2016/08/09 at 23:01:45, sunyunjia wrote:
> > PTAL
> > 
> > Besides, we are thinking about moving all the touch handler logic of <input
> type="range"> into an element of its shadow dom, e.g. SliderContainerElement. 
> > 
> > This will enable authors have their own touch-action that does not conflict
> with user-agent's one. Doing so would give us a more clear logic that handles
> the conflict than my current implementation. What's your opinion, tkent@ ?
> 
> It sounds better.  However I'm not sure if it really won't prevent web authors
> from overriding touch-action of input[type=range]. Does <input type=range
> style="touch-action:none"> will continue to work as expected?

It will work as expected. With proposed change, the touch-action on the input 
is considered an ancestor touch-action which can only make allowed touch actions
more restrictive. So it is safe and compose well with UA value. :)

The problem was that if defined on the same element, web authors could
redefine UA provided touch-action with a more permissive touch action and 
break things.

[sunyunjia, 2016-08-12 02:03:47]

Moved the touch event handlers of <input type="range"> to the container in the
shadow dom. PTAL, Thanks!

[sunyunjia, 2016-08-12 02:05:19]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 02:05:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 03:22:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 03:22:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[majidvp, 2016-08-12 03:35:44]

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html
(right):

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html:24:
var ctner1 = internals.shadowRoot(slider1).getElementById('container');
ctner does not read that well. how about just using container?

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:428: 
I think tkent@ suggestion was to move this to LayoutTheme::adjustStyle.

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:429: // This block
is for initialzation stage, when container's layoubtObject() hasn't been
initialized.
s/initialzation/initialization/

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:432: if (host &&
isHTMLInputElement(host) && static_cast<HTMLInputElement*>(host)->type() ==
InputTypeNames::range) {
blink has helper function that should be used instead of such static casts. 
Here: toHTMLInputElement

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:433:
adjustSliderStyle(style, element, host->layoutObject()->styleRef());
Instead of passing the whole style reference as last parameter why not 
just pass in the "appearance". Although see my following comment on
how we could get rid of this.

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:438: //
Container's layoutObject() has been initialized this time.
I am not an style expert but I think a better approach would be
to cascade the appearance style from input host to the slider
container appearance and just have the above logic.

This way the touch-action depends only on slider container appearance
and slider container appearance depends on input element appearance.


The cascade will be something similar to:
  LayoutSliderThumb::updateAppearance

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLInputElement.cpp:417:
*(this->userAgentShadowRoot()->getElementById(ShadowElementNames::sliderContainer())),

This leaks lots of internal details of the RangeInputType to HTMLInputElement
and should be avoided.

I think, we really should get rid of this whole registration here and move the
registration inside SliderContainerElement itself.

HTMLInputElement should not be aware of existence of a touch handler and it
shouldn't need to update it. The touch handler gets registered when internal
shadow dom is created and de-registered when it is removed.


This involves a bit of work and may even be better suited as a follow up patch.
I suggest the following:

This patch 
------------ 
keep the logic here intact, i.e. register touch handler on input rather than
the slider container.

Follow up patch 
-------------- 
Register touch handler when constructing the SliderContainer and 
de-register when it is detached. This ensures the touch- action and touch 
handler are on the same element which is ideal. 

Also removes
InputTypeView::hasTouchEventHandler/m_hasPassiveTouchEventHandler/updateTouchEvent
HandlerRegister


Just my suggestion but please follow kent@ advice. He is the expert.

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp (right):

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:321:
HTMLInputElement* input = static_cast<HTMLInputElement*>(shadowHost());
s/static_cast<HTMLInputElement*>/toHTMLInputElement/

[tkent, 2016-08-12 06:15:18]

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:438: //
Container's layoutObject() has been initialized this time.
On 2016/08/12 at 03:35:43, majidvp wrote:
> I am not an style expert but I think a better approach would be
> to cascade the appearance style from input host to the slider
> container appearance and just have the above logic.
> 
> This way the touch-action depends only on slider container appearance
> and slider container appearance depends on input element appearance.
> 
> 
> The cascade will be something similar to:
>   LayoutSliderThumb::updateAppearance

Yes, adding -webkit-appearance:inherit to ::-webkit-slider-container section of
html.css should work.  Then, we can move the adjustment code to LayoutTheme
again.

Also, html.css should have touch-action CSS property for a case of
-webkit-appearance:none.

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLInputElement.cpp:417:
*(this->userAgentShadowRoot()->getElementById(ShadowElementNames::sliderContainer())),
On 2016/08/12 at 03:35:43, majidvp wrote:
> This leaks lots of internal details of the RangeInputType to HTMLInputElement
> and should be avoided.
> 
> I think, we really should get rid of this whole registration here and move the
> registration inside SliderContainerElement itself.
> 
> HTMLInputElement should not be aware of existence of a touch handler and it
> shouldn't need to update it. The touch handler gets registered when internal
> shadow dom is created and de-registered when it is removed.
> 
> 
> This involves a bit of work and may even be better suited as a follow up
patch.
> I suggest the following:
> 
> This patch 
> ------------ 
> keep the logic here intact, i.e. register touch handler on input rather than
> the slider container.
> 
> Follow up patch 
> -------------- 
> Register touch handler when constructing the SliderContainer and 
> de-register when it is detached. This ensures the touch- action and touch 
> handler are on the same element which is ideal. 
> 
> Also removes
InputTypeView::hasTouchEventHandler/m_hasPassiveTouchEventHandler/updateTouchEvent
> HandlerRegister
> 
> 
> Just my suggestion but please follow kent@ advice. He is the expert.

Yes, we shouldn't have inputtype-specific logic in HTMLInputElement.  At least,
we should introduce InputType::elementWithPassiveTouchEvent().

[sunyunjia, 2016-08-15 15:48:03]

sunyunjia@chromium.org changed reviewers:
+ dtapuska@chromium.org

[sunyunjia, 2016-08-15 15:52:04]

Moved the touch-action logic to LayoutTheme.cpp using -webkit-appearance:
inherit

The touch event handler is still registered through HTMLInputElement for now.
The following patch will register the touch handler directly through
SliderContainer element.

PTAL, Thanks!

[sunyunjia, 2016-08-15 15:52:22]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 15:52:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 17:03:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-15 17:04:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2016-08-15 22:11:16]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 22:11:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 23:25:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-15 23:25:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2016-08-16 00:09:26]

With the newest patch, the touch event handler is registered directly through
SliderContainer element and the code related to touch event handler in
HTMLInputElement has been removed.

"-webkit-appearance: inherit" solves the touch-action problem elegantly and I
can move my code to LayoutTheme. However, this breaks many tests, most of which
are related to painting. Seems adding the appearance causes the painter to paint
some extra stuff for the SliderContainer element. I'm wondering if you have any
good suggestions on that? Maybe we can try to move all the touch
handlers/actions to the SliderThumb element? The thumb has to be painted anyway.

Thanks!

[sunyunjia, 2016-08-16 00:09:32]

With the newest patch, the touch event handler is registered directly through
SliderContainer element and the code related to touch event handler in
HTMLInputElement has been removed.

"-webkit-appearance: inherit" solves the touch-action problem elegantly and I
can move my code to LayoutTheme. However, this breaks many tests, most of which
are related to painting. Seems adding the appearance causes the painter to paint
some extra stuff for the SliderContainer element. I'm wondering if you have any
good suggestions on that? Maybe we can try to move all the touch
handlers/actions to the SliderThumb element? The thumb has to be painted anyway.

Thanks!

[sunyunjia, 2016-08-16 02:18:25]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 02:18:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 04:05:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 04:05:17]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sunyunjia, 2016-08-16 14:08:56]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 14:09:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 16:36:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 16:36:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dtapuska, 2016-08-16 20:37:36]

On 2016/08/16 00:09:32, sunyunjia wrote:
> With the newest patch, the touch event handler is registered directly through
> SliderContainer element and the code related to touch event handler in
> HTMLInputElement has been removed.
> 
> "-webkit-appearance: inherit" solves the touch-action problem elegantly and I
> can move my code to LayoutTheme. However, this breaks many tests, most of
which
> are related to painting. Seems adding the appearance causes the painter to
paint
> some extra stuff for the SliderContainer element. I'm wondering if you have
any
> good suggestions on that? Maybe we can try to move all the touch
> handlers/actions to the SliderThumb element? The thumb has to be painted
anyway.
> 
> Thanks!

tkent@ can you respond to #48 when you are back tomorrow?

[sunyunjia, 2016-08-17 00:08:13]

On 2016/08/16 20:37:36, dtapuska wrote:
> On 2016/08/16 00:09:32, sunyunjia wrote:

> tkent@ can you respond to #48 when you are back tomorrow?

#48 has been solved. The only test I'm failing is
virtual/android/fullscreen/compositor-touch-hit-rects-fullscreen-video-controls.html.
On the second output, instead of "handler: DIV (42, 3, 637, 24)", it outputs
"handler: no rect". This seems to be an issue caused by removing the
touch-event-handler from <input type="range"> element. I'm wondering how
important the test is, or whether it's possible to modify the test or output?
I'm still investigating the cause.

Thanks!

[tkent, 2016-08-17 01:32:00]

https://codereview.chromium.org/2209773002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html
(right):

https://codereview.chromium.org/2209773002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html:18:
'Tests that there are 4 blocking touch handlers in total(each slider has its own
blocking touch-action, plus one blocking action defined by the user), and that
there are 3 passive touch handlers(each slider has its own handler to deal with
the dragging event)');
We should test user-facing behavior instead of internal event handler counts.

https://codereview.chromium.org/2209773002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/2209773002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLInputElement.cpp:68: #include
"core/html/shadow/ShadowElementNames.h"
This change isn't necessary.

https://codereview.chromium.org/2209773002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp (right):

https://codereview.chromium.org/2209773002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:380: void
SliderContainerElement::parserDidSetAttributes()
Why is it necessary though parseAttribute() calls updateTouchEventRegistry()?

https://codereview.chromium.org/2209773002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:388:
updateTouchEventHandlerRegistry();
Why do we need to call updateTouchEventHandlerRegistry() on attribute changes?

https://codereview.chromium.org/2209773002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTheme.cpp (right):

https://codereview.chromium.org/2209773002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTheme.cpp:66: inline static bool
isUnderInputRangeShadow(Element* e)
e -> element

https://codereview.chromium.org/2209773002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTheme.cpp:69: return host &&
isHTMLInputElement(host) && toHTMLInputElement(host)->type() ==
InputTypeNames::range;
Can you check e->shadowPseudoId() instead?  We'd like to avoid to check
HTMLInputElement::type() in Blink core.

[sunyunjia, 2016-08-17 02:03:55]

>
https://codereview.chromium.org/2209773002/diff/260001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:380: void
> SliderContainerElement::parserDidSetAttributes()
> Why is it necessary though parseAttribute() calls updateTouchEventRegistry()?

I'm not so sure about this. I added this code because this is how it was
implemented in HTMLInputElement. I haven't checked the justification for
HTMLInputElement to do it this way. Or does anyone know the justification and
the difference between HTMLInputElement and SliderContainerElement(DIV) such
that we don't need to do it this way? Thanks!

>
https://codereview.chromium.org/2209773002/diff/260001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:388:
> updateTouchEventHandlerRegistry();
> Why do we need to call updateTouchEventHandlerRegistry() on attribute changes?

Same as above

I've checked the reason for the failure of
virtual/android/fullscreen/compositor-touch-hit-rects-fullscreen-video-controls.html,
tracing the call hierarchy, I found this code:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/scro...
So the cause of the failure is the change of touch-handler registry's to
passive. Should we somehow change the test output to "no rect"?

[tkent, 2016-08-17 03:56:44]

On 2016/08/17 at 02:03:55, sunyunjia wrote:
> >
https://codereview.chromium.org/2209773002/diff/260001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:380: void
> > SliderContainerElement::parserDidSetAttributes()
> > Why is it necessary though parseAttribute() calls
updateTouchEventRegistry()?
> 
> I'm not so sure about this. I added this code because this is how it was
implemented in HTMLInputElement. I haven't checked the justification for
HTMLInputElement to do it this way. Or does anyone know the justification and
the difference between HTMLInputElement and SliderContainerElement(DIV) such
that we don't need to do it this way? Thanks!

I looked at the current code, and understood the current
updateTouchEventHandlerRegistry() calls are for updating 'type' attribute.  So,
we don't need to call it in SliderContainerElement::parseDidSetAttributes() and
parseAttribute(), and probably need to call
EventHandlerRegistry::didRemoveEventHandler() in
SliderContainerElement::removedFrom() hook instead?

[sunyunjia, 2016-08-17 23:19:08]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[sunyunjia, 2016-08-17 23:19:26]

The CQ bit was unchecked by sunyunjia@chromium.org

[sunyunjia, 2016-08-17 23:48:03]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 23:48:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2016-08-17 23:50:09]

Added a test for user-facing behavior and removed unnecessary code in
SliderContainerElement.

PTAL. Thanks!

[sunyunjia, 2016-08-18 01:26:37]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 01:27:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 02:12:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 02:12:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tkent, 2016-08-18 05:13:49]

https://codereview.chromium.org/2209773002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html
(right):

https://codereview.chromium.org/2209773002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html:26:
<script>
Indentation and coding style in this element look inconsistent.

Also, please use consistent quote characters.  Many strings are wrapped with
single quotes, but some are wrapped with double quotes.

https://codereview.chromium.org/2209773002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp (right):

https://codereview.chromium.org/2209773002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:369:
registry.didAddEventHandler(*this,
EventHandlerRegistry::TouchStartOrMoveEventPassive);
didRemoveEventHandler() isn't called for *this.  Is it ok?

[sunyunjia, 2016-08-18 13:34:26]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 13:34:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 15:34:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 15:34:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2016-08-18 20:35:15]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 20:35:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-18 22:29:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 22:29:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2016-08-19 02:29:57]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 02:30:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 04:54:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 04:54:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2016-08-19 13:23:20]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 13:23:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 13:27:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 13:27:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2016-08-21 18:58:38]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-21 18:58:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-21 19:01:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-21 19:01:08]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[sunyunjia, 2016-08-21 19:06:22]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-21 19:06:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-21 19:07:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-21 19:07:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sunyunjia, 2016-08-21 19:15:39]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-21 19:15:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-21 19:18:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-21 19:18:06]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2016-08-21 21:08:00]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-21 21:08:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-21 23:02:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-21 23:02:03]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2016-08-21 23:07:44]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-21 23:07:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 01:06:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 01:06:20]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2016-08-22 02:02:56]

>
https://codereview.chromium.org/2209773002/diff/320001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:369:
> registry.didAddEventHandler(*this,
> EventHandlerRegistry::TouchStartOrMoveEventPassive);
> didRemoveEventHandler() isn't called for *this.  Is it ok?
Yes, event handlers will be removed from removeAllEventListeners(). We also need
to updateEventHandlerRegistry() in parseAttribute(), which is called from
RangeInputType::createShadowSubtree->Element::setAttribute(), so that
SliderContainer can inherit Input's webkit-appearance.
------------

In previous commit, when a touch event happens, the thumb will move horizontally
and the page will scroll vertically at the same time, since users can hardly
perform a perfect horizontal/vertical drag input. In recent updates, we lock one
of the directions for each dragging. If the angle of the dragging and the slider
is smaller than 45 degree, only the slider thumb will move. Otherwise, if the
angle is larger than 45 degree, only the page will scroll and the slider's value
won't be changed.

PTAL, thanks!

[tkent, 2016-08-22 06:00:48]

https://codereview.chromium.org/2209773002/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html
(right):

https://codereview.chromium.org/2209773002/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html:28:
function forceLayoutUpdate() {
Wrong indentation.  'function' should be started at the first column.

https://codereview.chromium.org/2209773002/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html:32:
function buildPage()
Ditto.

https://codereview.chromium.org/2209773002/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/touch/touch-action-range-input.html:222:

Unnecessary blank lines.

https://codereview.chromium.org/2209773002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp (right):

https://codereview.chromium.org/2209773002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:332:
HTMLInputElement* input = toHTMLInputElement(shadowHost());
HTMLInputElement* input = hostInput();

https://codereview.chromium.org/2209773002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:349:
SliderThumbElement* thumb =
toSliderThumbElement(input->userAgentShadowRoot()->getElementById(ShadowElementNames::sliderThumb()));
toSliderThumbElement(treeScope()->getElementById(ShadowElementNames::sliderThumb()))

is simpler.

https://codereview.chromium.org/2209773002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:361: if
(m_slidingDirection != NoMove && canSlide()) {
m_slidingDirection==NoMove was already check.  So this line should be |} else if
...|

    } else if (canSlide()) {

Also, canSlide() was already checked at line 344.

https://codereview.chromium.org/2209773002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:442:
updateTouchEventHandlerRegistry();
I think we can move this to RangeInputType::createShadowSubtree(), and remove
the parseAttribute() override.

[sunyunjia, 2016-08-22 14:16:28]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 14:17:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 16:18:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 16:18:51]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2016-08-22 16:55:14]

>
https://codereview.chromium.org/2209773002/diff/440001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:361: if
> (m_slidingDirection != NoMove && canSlide()) {
> m_slidingDirection==NoMove was already check.  So this line should be |} else
if
> ...|
> 
>     } else if (canSlide()) {
> 
> Also, canSlide() was already checked at line 344.

That's true. However, m_slidingDirection might have been changed in the above
if-statement, so it's necessary to check whether we should to update the current
slider position.

>
https://codereview.chromium.org/2209773002/diff/440001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/shadow/SliderThumbElement.cpp:442:
> updateTouchEventHandlerRegistry();
> I think we can move this to RangeInputType::createShadowSubtree(), and remove
> the parseAttribute() override.

I figured out that the registry should happen during initialization. I moved
updateTouchEventHandlerRegistry() into the constructor to make
RangeInputType::createShadowSubtree() less complicated.

PTAL, thanks!

[tkent, 2016-08-23 00:53:48]

lgtm

[sunyunjia, 2016-08-23 01:05:03]

The CQ bit was checked by sunyunjia@chromium.org

[commit-bot: I haz the power, 2016-08-23 01:05:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-23 01:11:47]

Description was changed from

==========
Remove the blocking touch handlers for the input[type=range] element and add
touch-action instead.

In current implementation, the input[type=range] blocks browser's scrolling in
all directions.

After adding "touch-action: pan-y" with a passive event listener to the
horizontal input[type=range], or "pan-x" to the vertical input[type=range], the
slider will only consume pan in its direction and does not block scrolling on
the other direction.

This touch-action will be overridden if the user has already defined the
touch-action. It will update if the slider changes to horizontal/vertical. 

BUG=584438
==========

to

==========
Remove the blocking touch handlers for the input[type=range] element and add
touch-action instead.

In current implementation, the input[type=range] blocks browser's scrolling in
all directions.

After adding "touch-action: pan-y" with a passive event listener to the
horizontal input[type=range], or "pan-x" to the vertical input[type=range], the
slider will only consume pan in its direction and does not block scrolling on
the other direction.

This touch-action will be overridden if the user has already defined the
touch-action. It will update if the slider changes to horizontal/vertical. 

BUG=584438
==========

[commit-bot: I haz the power, 2016-08-23 01:11:49]

Committed patchset #24 (id:460001)

[commit-bot: I haz the power, 2016-08-23 01:14:50]

Description was changed from

==========
Remove the blocking touch handlers for the input[type=range] element and add
touch-action instead.

In current implementation, the input[type=range] blocks browser's scrolling in
all directions.

After adding "touch-action: pan-y" with a passive event listener to the
horizontal input[type=range], or "pan-x" to the vertical input[type=range], the
slider will only consume pan in its direction and does not block scrolling on
the other direction.

This touch-action will be overridden if the user has already defined the
touch-action. It will update if the slider changes to horizontal/vertical. 

BUG=584438
==========

to

==========
Remove the blocking touch handlers for the input[type=range] element and add
touch-action instead.

In current implementation, the input[type=range] blocks browser's scrolling in
all directions.

After adding "touch-action: pan-y" with a passive event listener to the
horizontal input[type=range], or "pan-x" to the vertical input[type=range], the
slider will only consume pan in its direction and does not block scrolling on
the other direction.

This touch-action will be overridden if the user has already defined the
touch-action. It will update if the slider changes to horizontal/vertical.

BUG=584438

Committed: https://crrev.com/cb18694aff180e913277a346a37e74835935b37d
Cr-Commit-Position: refs/heads/master@{#413616}
==========

[commit-bot: I haz the power, 2016-08-23 01:14:51]

Patchset 24 (id:??) landed as
https://crrev.com/cb18694aff180e913277a346a37e74835935b37d
Cr-Commit-Position: refs/heads/master@{#413616}

[sunyunjia, 2016-08-23 13:36:46]

A revert of this CL (patchset #25 id:480001) has been created in
https://codereview.chromium.org/2266423003/ by sunyunjia@chromium.org.

The reason for reverting is: Broke windows test. See
https://bugs.chromium.org/p/chromium/issues/detail?id=640150.

[sunyunjia, 2016-09-20 21:39:07]

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/2209773002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:438: //
Container's layoutObject() has been initialized this time.
On 2016/08/12 06:15:18, tkent wrote:
> On 2016/08/12 at 03:35:43, majidvp wrote:
> > I am not an style expert but I think a better approach would be
> > to cascade the appearance style from input host to the slider
> > container appearance and just have the above logic.
> > 
> > This way the touch-action depends only on slider container appearance
> > and slider container appearance depends on input element appearance.
> > 
> > 
> > The cascade will be something similar to:
> >   LayoutSliderThumb::updateAppearance
> 
> Yes, adding -webkit-appearance:inherit to ::-webkit-slider-container section
of
> html.css should work.  Then, we can move the adjustment code to LayoutTheme
> again.
> 
> Also, html.css should have touch-action CSS property for a case of
> -webkit-appearance:none.

According to https://bugs.chromium.org/p/chromium/issues/detail?id=648589, we
can't change the inline style of sliderContainer under this circumstance. Are we
going back to this logic?

[tkent, 2016-09-20 23:01:55]

> According to https://bugs.chromium.org/p/chromium/issues/detail?id=648589, we
can't change the inline style of sliderContainer under this circumstance. Are we
going back to this logic?

IMO, CSP shouldn't reject inline style in User-Agent shadow trees.