Bug 193314 – Support parsing of additional values for the touch-action property

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antoine Quint]

Support additional values for the touch-action property

[Antoine Quint]

Created attachment 358788 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/47176519>

[EWS Watchlist]

Comment on attachment 358788 [details]
Patch

Attachment 358788 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10697353

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall.https.html

[EWS Watchlist]

Created attachment 358795 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Dean Jackson]

Comment on attachment 358788 [details]
Patch

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

> Source/WebCore/css/StyleBuilderConverter.h:1359
> +            auto& primitiveValue = downcast<CSSPrimitiveValue>(currentValue.get());

Is this always safe?

> Source/WebCore/css/StyleBuilderConverter.h:1361
> +            if (primitiveValueID != CSSValuePanX && primitiveValueID != CSSValuePanY && primitiveValueID != CSSValuePinchZoom)

So "none pan-x" evaluates to be "none" (initial)?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:1349
> +    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();

auto?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:1351
> +        RefPtr<CSSPrimitiveValue> ident = consumeIdent<CSSValuePanX, CSSValuePanY, CSSValuePinchZoom>(range);

auto?

> Source/WebCore/dom/Element.cpp:106
> +#include "TouchAction.h"

Is it used in this file?

> Source/WebCore/platform/TouchAction.h:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

2019.

> Source/WebCore/platform/TouchAction.h:17
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.

Why are you using the GPL? Use the normal license.

> Source/WebCore/rendering/style/RenderStyle.h:1227
> +    void setTouchAction(OptionSet<TouchAction> ta) { SET_VAR(m_rareNonInheritedData, touchAction, ta.toRaw()); }

ugh. why not use "touchAction" rather than "ta"?

(or "touchActions")

[Antoine Quint]

(In reply to Dean Jackson from comment #5)
> Comment on attachment 358788 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=358788&action=review
> 
> > Source/WebCore/css/StyleBuilderConverter.h:1359
> > +            auto& primitiveValue = downcast<CSSPrimitiveValue>(currentValue.get());
> 
> Is this always safe?

Antti (on IRC) thinks so since we create the value list in consumeTouchAction(). 

> > Source/WebCore/css/StyleBuilderConverter.h:1361
> > +            if (primitiveValueID != CSSValuePanX && primitiveValueID != CSSValuePanY && primitiveValueID != CSSValuePinchZoom)
> 
> So "none pan-x" evaluates to be "none" (initial)?

It should evaluate to "auto", the initial value.

> > Source/WebCore/css/parser/CSSPropertyParser.cpp:1349
> > +    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
> 
> auto?

Will update.

> > Source/WebCore/css/parser/CSSPropertyParser.cpp:1351
> > +        RefPtr<CSSPrimitiveValue> ident = consumeIdent<CSSValuePanX, CSSValuePanY, CSSValuePinchZoom>(range);
> 
> auto?

Will update.

> > Source/WebCore/dom/Element.cpp:106
> > +#include "TouchAction.h"
> 
> Is it used in this file?

Yes, in Element::allowsDoubleTapGesture(). Since TouchAction now is specified in a separate file, I figured it would be better to explicitly include it.

> > Source/WebCore/platform/TouchAction.h:2
> > + * Copyright (C) 2018 Apple Inc. All rights reserved.
> 
> 2019.

Will update.

> > Source/WebCore/platform/TouchAction.h:17
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Library General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Library General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Library General Public License
> > + * along with this library; see the file COPYING.LIB.  If not, write to
> > + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> > + * Boston, MA 02110-1301, USA.
> 
> Why are you using the GPL? Use the normal license.

Copy/pasted from the wrong file without thinking twice.

> > Source/WebCore/rendering/style/RenderStyle.h:1227
> > +    void setTouchAction(OptionSet<TouchAction> ta) { SET_VAR(m_rareNonInheritedData, touchAction, ta.toRaw()); }
> 
> ugh. why not use "touchAction" rather than "ta"?
> 
> (or "touchActions")

Will update.

[Antti Koivisto]

You might also consider renaming all OptionSet<TouchAction> functions/fields/variables 'touchAction' -> 'touchActions' (unless you really want to match the property name exactly).

[Antoine Quint]

(In reply to Antti Koivisto from comment #7)
> You might also consider renaming all OptionSet<TouchAction>
> functions/fields/variables 'touchAction' -> 'touchActions' (unless you
> really want to match the property name exactly).

I will do that, I think that's the right thing to do.

[Antoine Quint]

Created attachment 358935 [details]
Patch

[Antoine Quint]

Created attachment 358937 [details]
Patch

[Dean Jackson]

Comment on attachment 358937 [details]
Patch

rs=me

[EWS Watchlist]

Comment on attachment 358937 [details]
Patch

Attachment 358937 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10718323

New failing tests:
css3/touch-action/touch-action-parsing.html
imported/w3c/web-platform-tests/pointerevents/pointerevent_touch-action-illegal.html
imported/w3c/web-platform-tests/pointerevents/extension/pointerevent_touch-action-verification.html
css3/touch-action/touch-action-computed-style.html
imported/w3c/web-platform-tests/pointerevents/pointerevent_touch-action-verification.html

[EWS Watchlist]

Created attachment 358958 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Antoine Quint]

Committed r239978: <https://trac.webkit.org/changeset/239978>